diff mbox series

[net-next,1/2] net: dsa: untag the bridge pvid from rx skbs

Message ID 20200923031155.2832348-2-f.fainelli@gmail.com
State New
Headers show
Series net: dsa: b53: Configure VLANs while not filtering | expand

Commit Message

Florian Fainelli Sept. 23, 2020, 3:11 a.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently the bridge untags VLANs present in its VLAN groups in
__allowed_ingress() only when VLAN filtering is enabled.

But when a skb is seen on the RX path as tagged with the bridge's pvid,
and that bridge has vlan_filtering=0, and there isn't any 8021q upper
with that VLAN either, then we have a problem. The bridge will not untag
it (since it is supposed to remain VLAN-unaware), and pvid-tagged
communication will be broken.

There are 2 situations where we can end up like that:

1. When installing a pvid in egress-tagged mode, like this:

ip link add dev br0 type bridge vlan_filtering 0
ip link set swp0 master br0
bridge vlan del dev swp0 vid 1
bridge vlan add dev swp0 vid 1 pvid

This happens because DSA configures the VLAN membership of the CPU port
using the same flags as swp0 (in this case "pvid and not untagged"), in
an attempt to copy the frame as-is from ingress to the CPU.

However, in this case, the packet may arrive untagged on ingress, it
will be pvid-tagged by the ingress port, and will be sent as
egress-tagged towards the CPU. Otherwise stated, the CPU will see a VLAN
tag where there was none to speak of on ingress.

When vlan_filtering is 1, this is not a problem, as stated in the first
paragraph, because __allowed_ingress() will pop it. But currently, when
vlan_filtering is 0 and we have such a VLAN configuration, we need an
8021q upper (br0.1) to be able to ping over that VLAN, which is not
symmetrical with the vlan_filtering=1 case, and therefore, confusing for
users.

Basically what DSA attempts to do is simply an approximation: try to
copy the skb with (or without) the same VLAN all the way up to the CPU.
But DSA drivers treat CPU port VLAN membership in various ways (which is
a good segue into situation 2). And some of those drivers simply tell
the CPU port to copy the frame unmodified, which is the golden standard
when it comes to VLAN processing (therefore, any driver which can
configure the hardware to do that, should do that, and discard the VLAN
flags requested by DSA on the CPU port).

2. Some DSA drivers always configure the CPU port as egress-tagged, in
an attempt to recover the classified VLAN from the skb. These drivers
cannot work at all with untagged traffic when bridged in
vlan_filtering=0 mode. And they can't go for the easy "just keep the
pvid as egress-untagged towards the CPU" route, because each front port
can have its own pvid, and that might require conflicting VLAN
membership settings on the CPU port (swp1 is pvid for VID 1 and
egress-tagged for VID 2; swp2 is egress-taggeed for VID 1 and pvid for
VID 2; with this simplistic approach, the CPU port, which is really a
separate hardware entity and has its own VLAN membership settings, would
end up being egress-untagged in both VID 1 and VID 2, therefore losing
the VLAN tags of ingress traffic).

So the only thing we can do is to create a helper function for resolving
the problematic case (that is, a function which untags the bridge pvid
when that is in vlan_filtering=0 mode), which taggers in need should
call. It isn't called from the generic DSA receive path because there
are drivers that fall neither in the first nor second category.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h  |  8 ++++++
 net/dsa/dsa.c      |  9 +++++++
 net/dsa/dsa_priv.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)

Comments

Vladimir Oltean Sept. 23, 2020, 8:14 a.m. UTC | #1 
On Tue, Sep 22, 2020 at 08:11:54PM -0700, Florian Fainelli wrote:
> diff --git a/include/net/dsa.h b/include/net/dsa.h

> index d16057c5987a..b539241a7533 100644

> --- a/include/net/dsa.h

> +++ b/include/net/dsa.h

> @@ -301,6 +301,14 @@ struct dsa_switch {

>  	 */

>  	bool			configure_vlan_while_not_filtering;

>  

> +	/* If the switch driver always programs the CPU port as egress tagged

> +	 * despite the VLAN configuration indicating otherwise, then setting

> +	 * @untag_bridge_pvid will force the DSA receive path to pop the bridge's

> +	 * default_pvid VLAN tagged frames to offer a consistent behavior

> +	 * between a vlan_filtering=0 and vlan_filtering=1 bridge device.

> +	 */

> +	bool			untag_bridge_pvid;

> +

>  	/* In case vlan_filtering_is_global is set, the VLAN awareness state

>  	 * should be retrieved from here and not from the per-port settings.

>  	 */

> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c

> index 5c18c0214aac..dec4ab59b7c4 100644

> --- a/net/dsa/dsa.c

> +++ b/net/dsa/dsa.c

> @@ -225,6 +225,15 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,

>  	skb->pkt_type = PACKET_HOST;

>  	skb->protocol = eth_type_trans(skb, skb->dev);

>  

> +	if (unlikely(cpu_dp->ds->untag_bridge_pvid)) {

> +		nskb = dsa_untag_bridge_pvid(skb);

> +		if (!nskb) {

> +			kfree_skb(skb);

> +			return 0;

> +		}

> +		skb = nskb;

> +	}

> +

>  	s = this_cpu_ptr(p->stats64);

>  	u64_stats_update_begin(&s->syncp);

>  	s->rx_packets++;


I was thinking a lot simpler. Maybe you could just tail-call
dsa_untag_bridge_pvid(skb) at the end of your .rcv function instead of
putting it in the common receive path. I specifically wrote it to look
at hdr->h_vlan_proto instead of skb->protocol, so it wouldn't depend on
eth_type_trans().

Something like:

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 9c6c30649d13..118d253af5a7 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -192,7 +192,7 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 		nskb->data - ETH_HLEN - BRCM_TAG_LEN,
 		2 * ETH_ALEN);
 
-	return nskb;
+	return dsa_untag_bridge_pvid(nskb);
 }
 
 static const struct dsa_device_ops brcm_netdev_ops = {
@@ -220,8 +220,14 @@ static struct sk_buff *brcm_tag_rcv_prepend(struct sk_buff *skb,
 					    struct net_device *dev,
 					    struct packet_type *pt)
 {
+	struct sk_buff *nskb;
+
 	/* tag is prepended to the packet */
-	return brcm_tag_rcv_ll(skb, dev, pt, ETH_HLEN);
+	nskb = brcm_tag_rcv_ll(skb, dev, pt, ETH_HLEN);
+	if (!nskb)
+		return nskb;
+
+	return dsa_untag_bridge_pvid(nskb);
 }
 
 static const struct dsa_device_ops brcm_prepend_netdev_ops = {

Thanks,
-Vladimir
Florian Fainelli Sept. 23, 2020, 5:06 p.m. UTC | #2 
On 9/23/20 1:14 AM, Vladimir Oltean wrote:
> On Tue, Sep 22, 2020 at 08:11:54PM -0700, Florian Fainelli wrote:

>> diff --git a/include/net/dsa.h b/include/net/dsa.h

>> index d16057c5987a..b539241a7533 100644

>> --- a/include/net/dsa.h

>> +++ b/include/net/dsa.h

>> @@ -301,6 +301,14 @@ struct dsa_switch {

>>  	 */

>>  	bool			configure_vlan_while_not_filtering;

>>  

>> +	/* If the switch driver always programs the CPU port as egress tagged

>> +	 * despite the VLAN configuration indicating otherwise, then setting

>> +	 * @untag_bridge_pvid will force the DSA receive path to pop the bridge's

>> +	 * default_pvid VLAN tagged frames to offer a consistent behavior

>> +	 * between a vlan_filtering=0 and vlan_filtering=1 bridge device.

>> +	 */

>> +	bool			untag_bridge_pvid;

>> +

>>  	/* In case vlan_filtering_is_global is set, the VLAN awareness state

>>  	 * should be retrieved from here and not from the per-port settings.

>>  	 */

>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c

>> index 5c18c0214aac..dec4ab59b7c4 100644

>> --- a/net/dsa/dsa.c

>> +++ b/net/dsa/dsa.c

>> @@ -225,6 +225,15 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,

>>  	skb->pkt_type = PACKET_HOST;

>>  	skb->protocol = eth_type_trans(skb, skb->dev);

>>  

>> +	if (unlikely(cpu_dp->ds->untag_bridge_pvid)) {

>> +		nskb = dsa_untag_bridge_pvid(skb);

>> +		if (!nskb) {

>> +			kfree_skb(skb);

>> +			return 0;

>> +		}

>> +		skb = nskb;

>> +	}

>> +

>>  	s = this_cpu_ptr(p->stats64);

>>  	u64_stats_update_begin(&s->syncp);

>>  	s->rx_packets++;

> 

> I was thinking a lot simpler. Maybe you could just tail-call

> dsa_untag_bridge_pvid(skb) at the end of your .rcv function instead of

> putting it in the common receive path. I specifically wrote it to look

> at hdr->h_vlan_proto instead of skb->protocol, so it wouldn't depend on

> eth_type_trans().


Yes, good point, there is no point in promoting this to the main receive
path until we have a second user of that facility, v2 coming shortly.
-- 
Florian
diff mbox series

Patch

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d16057c5987a..b539241a7533 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -301,6 +301,14 @@  struct dsa_switch {
 	 */
 	bool			configure_vlan_while_not_filtering;
 
+	/* If the switch driver always programs the CPU port as egress tagged
+	 * despite the VLAN configuration indicating otherwise, then setting
+	 * @untag_bridge_pvid will force the DSA receive path to pop the bridge's
+	 * default_pvid VLAN tagged frames to offer a consistent behavior
+	 * between a vlan_filtering=0 and vlan_filtering=1 bridge device.
+	 */
+	bool			untag_bridge_pvid;
+
 	/* In case vlan_filtering_is_global is set, the VLAN awareness state
 	 * should be retrieved from here and not from the per-port settings.
 	 */
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5c18c0214aac..dec4ab59b7c4 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -225,6 +225,15 @@  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, skb->dev);
 
+	if (unlikely(cpu_dp->ds->untag_bridge_pvid)) {
+		nskb = dsa_untag_bridge_pvid(skb);
+		if (!nskb) {
+			kfree_skb(skb);
+			return 0;
+		}
+		skb = nskb;
+	}
+
 	s = this_cpu_ptr(p->stats64);
 	u64_stats_update_begin(&s->syncp);
 	s->rx_packets++;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2da656d984ef..0348dbab4131 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -7,6 +7,7 @@ 
 #ifndef __DSA_PRIV_H
 #define __DSA_PRIV_H
 
+#include <linux/if_bridge.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
 #include <linux/netpoll.h>
@@ -194,6 +195,71 @@  dsa_slave_to_master(const struct net_device *dev)
 	return dp->cpu_dp->master;
 }
 
+/* If under a bridge with vlan_filtering=0, make sure to send pvid-tagged
+ * frames as untagged, since the bridge will not untag them.
+ */
+static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
+{
+	struct dsa_port *dp = dsa_slave_to_port(skb->dev);
+	struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
+	struct net_device *br = dp->bridge_dev;
+	struct net_device *dev = skb->dev;
+	struct net_device *upper_dev;
+	struct list_head *iter;
+	u16 vid, pvid, proto;
+	int err;
+
+	if (!br || br_vlan_enabled(br))
+		return skb;
+
+	err = br_vlan_get_proto(br, &proto);
+	if (err)
+		return skb;
+
+	/* Move VLAN tag from data to hwaccel */
+	if (!skb_vlan_tag_present(skb) && hdr->h_vlan_proto == htons(proto)) {
+		skb = skb_vlan_untag(skb);
+		if (!skb)
+			return NULL;
+	}
+
+	if (!skb_vlan_tag_present(skb))
+		return skb;
+
+	vid = skb_vlan_tag_get_id(skb);
+
+	/* We already run under an RCU read-side critical section since
+	 * we are called from netif_receive_skb_list_internal().
+	 */
+	err = br_vlan_get_pvid_rcu(dev, &pvid);
+	if (err)
+		return skb;
+
+	if (vid != pvid)
+		return skb;
+
+	/* The sad part about attempting to untag from DSA is that we
+	 * don't know, unless we check, if the skb will end up in
+	 * the bridge's data path - br_allowed_ingress() - or not.
+	 * For example, there might be an 8021q upper for the
+	 * default_pvid of the bridge, which will steal VLAN-tagged traffic
+	 * from the bridge's data path. This is a configuration that DSA
+	 * supports because vlan_filtering is 0. In that case, we should
+	 * definitely keep the tag, to make sure it keeps working.
+	 */
+	netdev_for_each_upper_dev_rcu(dev, upper_dev, iter) {
+		if (!is_vlan_dev(upper_dev))
+			continue;
+
+		if (vid == vlan_dev_vlan_id(upper_dev))
+			return skb;
+	}
+
+	__vlan_hwaccel_clear_tag(skb);
+
+	return skb;
+}
+
 /* switch.c */
 int dsa_switch_register_notifier(struct dsa_switch *ds);
 void dsa_switch_unregister_notifier(struct dsa_switch *ds);