[PATCH net-next v3 0/2] net: dsa: b53: Configure VLANs while not filtering

[PATCH net-next v3 0/2] net: dsa: b53: Configure VLANs while not filtering

[Florian Fainelli]

Hi David, Jakub,

These two patches allow the b53 driver which always configures its CPU
port as egress tagged to behave correctly with VLANs being always
configured whenever a port is added to a bridge.

Vladimir provides a patch that aligns the bridge with vlan_filtering=0
receive path to behave the same as vlan_filtering=1. Per discussion with
Nikolay, this behavior is deemed to be too DSA specific to be done in
the bridge proper.

This is a preliminary series for Vladimir to make
configure_vlan_while_filtering the default behavior for all DSA drivers
in the future.

Thanks!

Changes in v3:

- added Vladimir's Acked-by tag to patch #2
- removed unnecessary if_vlan.h inclusion in patch #2
- reworded commit message to be accurate with the code changes

Changes in v2:

- moved the call to dsa_untag_bridge_pvid() into net/dsa/tag_brcm.c
  since we have a single user for now

Florian Fainelli (1):
  net: dsa: b53: Configure VLANs while not filtering

Vladimir Oltean (1):
  net: dsa: untag the bridge pvid from rx skbs

 drivers/net/dsa/b53/b53_common.c | 19 +--------
 drivers/net/dsa/b53/b53_priv.h   |  1 -
 net/dsa/dsa_priv.h               | 66 ++++++++++++++++++++++++++++++++
 net/dsa/tag_brcm.c               | 15 +++++++-
 4 files changed, 81 insertions(+), 20 deletions(-)

-- 
2.25.1

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

[Florian Fainelli]

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>
---
 net/dsa/dsa_priv.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

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);
-- 
2.25.1

[PATCH net-next v3 2/2] net: dsa: b53: Configure VLANs while not filtering

[Florian Fainelli]

Update the B53 driver to support VLANs while not filtering. This
requires us to enable VLAN globally within the switch upon driver
initial configuration (dev->vlan_enabled).

We also need to remove the code that dealt with PVID re-configuration in
b53_vlan_filtering() since that function worked under the assumption
that it would only be called to make a bridge VLAN filtering, or not
filtering, and we would attempt to move the port's PVID accordingly.

Now that VLANs are programmed all the time, even in the case of a
non-VLAN filtering bridge, we would be programming a default_pvid for
the bridged switch ports.

We need the DSA receive path to pop the VLAN tag if it is the bridge's
default_pvid because the CPU port is always programmed tagged in the
programmed VLANs. In order to do so we utilize the
dsa_untag_bridge_pvid() helper introduced in the commit before within
net/dsa/tag_brcm.c.

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 19 ++-----------------
 drivers/net/dsa/b53/b53_priv.h   |  1 -
 net/dsa/tag_brcm.c               | 15 +++++++++++++--
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 6a5796c32721..73507cff3bc4 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1377,23 +1377,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up);
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
 {
 	struct b53_device *dev = ds->priv;
-	u16 pvid, new_pvid;
-
-	b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
-	if (!vlan_filtering) {
-		/* Filtering is currently enabled, use the default PVID since
-		 * the bridge does not expect tagging anymore
-		 */
-		dev->ports[port].pvid = pvid;
-		new_pvid = b53_default_pvid(dev);
-	} else {
-		/* Filtering is currently disabled, restore the previous PVID */
-		new_pvid = dev->ports[port].pvid;
-	}
-
-	if (pvid != new_pvid)
-		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
-			    new_pvid);
 
 	b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering);
 
@@ -2619,6 +2602,8 @@ struct b53_device *b53_switch_alloc(struct device *base,
 	dev->priv = priv;
 	dev->ops = ops;
 	ds->ops = &b53_switch_ops;
+	ds->configure_vlan_while_not_filtering = true;
+	dev->vlan_enabled = ds->configure_vlan_while_not_filtering;
 	mutex_init(&dev->reg_mutex);
 	mutex_init(&dev->stats_mutex);
 
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index c55c0a9f1b47..24893b592216 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -91,7 +91,6 @@ enum {
 struct b53_port {
 	u16		vlan_ctl_mask;
 	struct ethtool_eee eee;
-	u16		pvid;
 };
 
 struct b53_vlan {
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index cc8512b5f9e2..1dab212a294f 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -140,6 +140,11 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
 	/* Remove Broadcom tag and update checksum */
 	skb_pull_rcsum(skb, BRCM_TAG_LEN);
 
+	/* Set the MAC header to where it should point for
+	 * dsa_untag_bridge_pvid() to parse the correct VLAN header.
+	 */
+	skb_set_mac_header(skb, -ETH_HLEN);
+
 	skb->offload_fwd_mark = 1;
 
 	return skb;
@@ -191,7 +196,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 = {
@@ -219,8 +224,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 = {
-- 
2.25.1

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

[Vladimir Oltean]

On Wed, Sep 23, 2020 at 02:40:37PM -0700, Florian Fainelli wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> +	/* 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;
> +	}

Argh...
So I wanted to ask you how's performance with a few 8021q uppers, then I
remembered that vlan_do_receive() probably does something more efficient
here than a complete lookup, like hashing or something, then I found the
vlan_find_dev() helper function.... Sorry for not noticing it in the
first place.

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

[Florian Fainelli]

On 9/23/20 2:48 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 02:40:37PM -0700, Florian Fainelli wrote:
>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>> +	/* 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;
>> +	}
> 
> Argh...
> So I wanted to ask you how's performance with a few 8021q uppers, then I
> remembered that vlan_do_receive() probably does something more efficient
> here than a complete lookup, like hashing or something, then I found the
> vlan_find_dev() helper function.... Sorry for not noticing it in the
> first place.

Given the platforms I am using this is not even noticeable, but I did
not test with more than 10 uppers being added to the switch port.

Speaking of that part of the code, I was also wondering whether you
wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
catch a bridge device upper as opposed to a switch port upper? Either
way is fine and there are possibly use cases for either.
-- 
Florian

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

[Vladimir Oltean]

On Wed, Sep 23, 2020 at 02:51:09PM -0700, Florian Fainelli wrote:
> Speaking of that part of the code, I was also wondering whether you
> wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
> catch a bridge device upper as opposed to a switch port upper? Either
> way is fine and there are possibly use cases for either.

So, yeah, both use cases are valid, and I did in fact mean uppers of the
bridge, but now that you're raising the point, do we actually support
properly the use case with an 8021q upper of a bridged port? My
understanding is that this VLAN-tagged traffic should not be switched on
RX. So without some ACL rule on ingress that the driver must install, I
don't see how that can work properly.

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

[Florian Fainelli]

On 9/23/20 3:01 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 02:51:09PM -0700, Florian Fainelli wrote:
>> Speaking of that part of the code, I was also wondering whether you
>> wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
>> catch a bridge device upper as opposed to a switch port upper? Either
>> way is fine and there are possibly use cases for either.
> 
> So, yeah, both use cases are valid, and I did in fact mean uppers of the
> bridge, but now that you're raising the point, do we actually support
> properly the use case with an 8021q upper of a bridged port? My
> understanding is that this VLAN-tagged traffic should not be switched on
> RX. So without some ACL rule on ingress that the driver must install, I
> don't see how that can work properly.

Is not this a problem only if the DSA master does VLAN receive filtering
though? In a bridge with vlan_filtering=0 the switch port is supposed to
accept any VLAN tagged frames because it does not do ingress VLAN ID
checking.

Prior to your patch, I would always install a br0.1 upper to pop the
default_pvid and that would work fine because the underlying DSA master
does not do VLAN filtering.
-- 
Florian

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

[Florian Fainelli]

On 9/23/20 3:06 PM, Florian Fainelli wrote:
> On 9/23/20 3:01 PM, Vladimir Oltean wrote:
>> On Wed, Sep 23, 2020 at 02:51:09PM -0700, Florian Fainelli wrote:
>>> Speaking of that part of the code, I was also wondering whether you
>>> wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
>>> catch a bridge device upper as opposed to a switch port upper? Either
>>> way is fine and there are possibly use cases for either.
>>
>> So, yeah, both use cases are valid, and I did in fact mean uppers of the
>> bridge, but now that you're raising the point, do we actually support
>> properly the use case with an 8021q upper of a bridged port? My
>> understanding is that this VLAN-tagged traffic should not be switched on
>> RX. So without some ACL rule on ingress that the driver must install, I
>> don't see how that can work properly.
> 
> Is not this a problem only if the DSA master does VLAN receive filtering
> though? In a bridge with vlan_filtering=0 the switch port is supposed to
> accept any VLAN tagged frames because it does not do ingress VLAN ID
> checking.
> 
> Prior to your patch, I would always install a br0.1 upper to pop the
> default_pvid and that would work fine because the underlying DSA master
> does not do VLAN filtering.

This is kind of a bad example, because the switch port has been added to
the default_pvid VLAN entry, but I believe the rest to be correct though.
-- 
Florian

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

[Vladimir Oltean]

On Wed, Sep 23, 2020 at 03:08:49PM -0700, Florian Fainelli wrote:
> On 9/23/20 3:06 PM, Florian Fainelli wrote:
> > On 9/23/20 3:01 PM, Vladimir Oltean wrote:
> >> On Wed, Sep 23, 2020 at 02:51:09PM -0700, Florian Fainelli wrote:
> >>> Speaking of that part of the code, I was also wondering whether you
> >>> wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
> >>> catch a bridge device upper as opposed to a switch port upper? Either
> >>> way is fine and there are possibly use cases for either.
> >>
> >> So, yeah, both use cases are valid, and I did in fact mean uppers of the
> >> bridge, but now that you're raising the point, do we actually support
> >> properly the use case with an 8021q upper of a bridged port? My
> >> understanding is that this VLAN-tagged traffic should not be switched on
> >> RX. So without some ACL rule on ingress that the driver must install, I
> >> don't see how that can work properly.
> >
> > Is not this a problem only if the DSA master does VLAN receive filtering
> > though?

I don't understand how the DSA master is involved here, sorry.

> > In a bridge with vlan_filtering=0 the switch port is supposed to
> > accept any VLAN tagged frames because it does not do ingress VLAN ID
> > checking.
> >
> > Prior to your patch, I would always install a br0.1 upper to pop the
> > default_pvid and that would work fine because the underlying DSA master
> > does not do VLAN filtering.

Yes, but on both your Broadcom tags, the VLAN header is shifted to the
right, so the master's hardware parser shouldn't figure out it's looking
at VLAN (unless your master is DSA-aware). So again, I don't see how
that makes a difference.

>
> This is kind of a bad example, because the switch port has been added to
> the default_pvid VLAN entry, but I believe the rest to be correct though.

I don't think it's a bad example, and I think that we should try to keep
br0.1 working.

Given the fact that all skbs are received as VLAN-tagged, the
dsa_untag_bridge_pvid function tries to guess what is the intention of
the user, in order to figure out when it should strip that tag and when
it shouldn't. When there is a swp0.1 upper, it is clear (to me, at
least) that the intention of the user is to terminate some traffic on
it, so the VLAN tag should be kept. Same should apply to br0.1. The only
difference is that swp0.1 might not work correctly today due to other,
unrelated reasons (like I said, the 8021q upper should 'steal' traffic
from the bridge inside the actual hardware datapath, but without
explicit configuration, which we don't have, it isn't really doing
that). Lastly, in absence of any 8021q upper, the function should untag
the skb to allow VLAN-unaware networking to be performed through the
bridge, because, presumably, that VLAN was added only as a side effect
of driver internal configuration, and is not desirable to any upper
layer.

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

[Florian Fainelli]

On 9/23/20 3:25 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 03:08:49PM -0700, Florian Fainelli wrote:
>> On 9/23/20 3:06 PM, Florian Fainelli wrote:
>>> On 9/23/20 3:01 PM, Vladimir Oltean wrote:
>>>> On Wed, Sep 23, 2020 at 02:51:09PM -0700, Florian Fainelli wrote:
>>>>> Speaking of that part of the code, I was also wondering whether you
>>>>> wanted this to be netdev_for_each_upper_dev_rcu(br, upper_dev, iter) and
>>>>> catch a bridge device upper as opposed to a switch port upper? Either
>>>>> way is fine and there are possibly use cases for either.
>>>>
>>>> So, yeah, both use cases are valid, and I did in fact mean uppers of the
>>>> bridge, but now that you're raising the point, do we actually support
>>>> properly the use case with an 8021q upper of a bridged port? My
>>>> understanding is that this VLAN-tagged traffic should not be switched on
>>>> RX. So without some ACL rule on ingress that the driver must install, I
>>>> don't see how that can work properly.
>>>
>>> Is not this a problem only if the DSA master does VLAN receive filtering
>>> though?
> 
> I don't understand how the DSA master is involved here, sorry.

I do not have a VLAN filtering DSA master at hand so maybe I am
fantasizing on something that is not a problem, but if the switch send
tagged traffic towards the DSA master and that DSA master is VLAN
filtering on receive and today we are not making sure that those VLANs
are programmed into the filter (regardless of a bridge existing), how do
we deliver these VLAN tagged frames to the DSA master?

> 
>>> In a bridge with vlan_filtering=0 the switch port is supposed to
>>> accept any VLAN tagged frames because it does not do ingress VLAN ID
>>> checking.
>>>
>>> Prior to your patch, I would always install a br0.1 upper to pop the
>>> default_pvid and that would work fine because the underlying DSA master
>>> does not do VLAN filtering.
> 
> Yes, but on both your Broadcom tags, the VLAN header is shifted to the
> right, so the master's hardware parser shouldn't figure out it's looking
> at VLAN (unless your master is DSA-aware). So again, I don't see how
> that makes a difference.

The NICs are all Broadcom tag aware but it only seems to matter to them
for checksum purposes, as none support VLAN extraction or filtering. I
get your point now.

> 
>>
>> This is kind of a bad example, because the switch port has been added to
>> the default_pvid VLAN entry, but I believe the rest to be correct though.
> 
> I don't think it's a bad example, and I think that we should try to keep
> br0.1 working.
> 
> Given the fact that all skbs are received as VLAN-tagged, the
> dsa_untag_bridge_pvid function tries to guess what is the intention of
> the user, in order to figure out when it should strip that tag and when
> it shouldn't. When there is a swp0.1 upper, it is clear (to me, at
> least) that the intention of the user is to terminate some traffic on
> it, so the VLAN tag should be kept. Same should apply to br0.1. The only
> difference is that swp0.1 might not work correctly today due to other,
> unrelated reasons (like I said, the 8021q upper should 'steal' traffic
> from the bridge inside the actual hardware datapath, but without
> explicit configuration, which we don't have, it isn't really doing
> that). Lastly, in absence of any 8021q upper, the function should untag
> the skb to allow VLAN-unaware networking to be performed through the
> bridge, because, presumably, that VLAN was added only as a side effect
> of driver internal configuration, and is not desirable to any upper
> layer.
> 

I don't think it would be making much sense to add an 802.1Q upper for
the bridge's default_pvid to the switch port, and add that upper as a
bridge port. Maybe we should make it work, maybe not.
-- 
Florian

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

[Florian Fainelli]

On 9/23/20 2:48 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 02:40:37PM -0700, Florian Fainelli wrote:
>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>> +	/* 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;
>> +	}
> 
> Argh...
> So I wanted to ask you how's performance with a few 8021q uppers, then I
> remembered that vlan_do_receive() probably does something more efficient
> here than a complete lookup, like hashing or something, then I found the
> vlan_find_dev() helper function.... Sorry for not noticing it in the
> first place.

Not having much luck with using  __vlan_find_dev_deep_rcu() for a reason
I don't understand we trip over the proto value being neither of the two
support Ethertype and hit the BUG().

+       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
+       if (upper_dev)
+               return skb;

Any ideas?
-- 
Florian

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

[Vladimir Oltean]

On Wed, Sep 23, 2020 at 03:54:59PM -0700, Florian Fainelli wrote:
> Not having much luck with using  __vlan_find_dev_deep_rcu() for a reason
> I don't understand we trip over the proto value being neither of the two
> support Ethertype and hit the BUG().
> 
> +       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
> +       if (upper_dev)
> +               return skb;
> 
> Any ideas?

Damn...
Yes, of course, the skb->protocol is still ETH_P_XDSA which is where
eth_type_trans() on the master left it.

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

[Florian Fainelli]

On 9/23/20 3:58 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 03:54:59PM -0700, Florian Fainelli wrote:
>> Not having much luck with using  __vlan_find_dev_deep_rcu() for a reason
>> I don't understand we trip over the proto value being neither of the two
>> support Ethertype and hit the BUG().
>>
>> +       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
>> +       if (upper_dev)
>> +               return skb;
>>
>> Any ideas?
> 
> Damn...
> Yes, of course, the skb->protocol is still ETH_P_XDSA which is where
> eth_type_trans() on the master left it.

proto was obtained from br_vlan_get_proto() a few lines above, and
br_vlan_get_proto() just returns br->vlan_proto which defaults to
htons(ETH_P_8021Q) from br_vlan_init().

This is not skb->protocol that we are looking at AFAICT.
-- 
Florian

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

[Vladimir Oltean]

On Wed, Sep 23, 2020 at 03:59:46PM -0700, Florian Fainelli wrote:
> On 9/23/20 3:58 PM, Vladimir Oltean wrote:
> > On Wed, Sep 23, 2020 at 03:54:59PM -0700, Florian Fainelli wrote:
> >> Not having much luck with using  __vlan_find_dev_deep_rcu() for a reason
> >> I don't understand we trip over the proto value being neither of the two
> >> support Ethertype and hit the BUG().
> >>
> >> +       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
> >> +       if (upper_dev)
> >> +               return skb;
> >>
> >> Any ideas?
> > 
> > Damn...
> > Yes, of course, the skb->protocol is still ETH_P_XDSA which is where
> > eth_type_trans() on the master left it.
> 
> proto was obtained from br_vlan_get_proto() a few lines above, and
> br_vlan_get_proto() just returns br->vlan_proto which defaults to
> htons(ETH_P_8021Q) from br_vlan_init().
> 
> This is not skb->protocol that we are looking at AFAICT.

Ok, my mistake. So what is the value of proto in vlan_proto_idx when it
fails? To me, the call path looks pretty pass-through for vlan_proto.

Re: [PATCH net-next v3 0/2] net: dsa: b53: Configure VLANs while not filtering

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 23 Sep 2020 14:40:36 -0700

> These two patches allow the b53 driver which always configures its CPU
> port as egress tagged to behave correctly with VLANs being always
> configured whenever a port is added to a bridge.
> 
> Vladimir provides a patch that aligns the bridge with vlan_filtering=0
> receive path to behave the same as vlan_filtering=1. Per discussion with
> Nikolay, this behavior is deemed to be too DSA specific to be done in
> the bridge proper.
> 
> This is a preliminary series for Vladimir to make
> configure_vlan_while_filtering the default behavior for all DSA drivers
> in the future.
 ...

Series applied, thanks Florian.

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

[Florian Fainelli]

On 9/23/2020 4:08 PM, Vladimir Oltean wrote:
> On Wed, Sep 23, 2020 at 03:59:46PM -0700, Florian Fainelli wrote:
>> On 9/23/20 3:58 PM, Vladimir Oltean wrote:
>>> On Wed, Sep 23, 2020 at 03:54:59PM -0700, Florian Fainelli wrote:
>>>> Not having much luck with using  __vlan_find_dev_deep_rcu() for a reason
>>>> I don't understand we trip over the proto value being neither of the two
>>>> support Ethertype and hit the BUG().
>>>>
>>>> +       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
>>>> +       if (upper_dev)
>>>> +               return skb;
>>>>
>>>> Any ideas?
>>>
>>> Damn...
>>> Yes, of course, the skb->protocol is still ETH_P_XDSA which is where
>>> eth_type_trans() on the master left it.
>>
>> proto was obtained from br_vlan_get_proto() a few lines above, and
>> br_vlan_get_proto() just returns br->vlan_proto which defaults to
>> htons(ETH_P_8021Q) from br_vlan_init().
>>
>> This is not skb->protocol that we are looking at AFAICT.
> 
> Ok, my mistake. So what is the value of proto in vlan_proto_idx when it
> fails? To me, the call path looks pretty pass-through for vlan_proto.

At the time we crash the proto value is indeed ETH_P_XDSA, but it is not 
because of the __vlan_find_dev_deep_rcu() call as I was mislead by the 
traces I was looking it (on ARMv7 the LR was pointing not where I was 
expecting it to), it is because of the following call trace:

netif_receive_skb_list_internal
   -> __netif_receive_skb_list_core
     -> __netif_receive_skb_core
       -> vlan_do_receive()

That function does use skb->vlan_proto to determine the VLAN group, at 
that point we have not set it but we did inherit skb->protocol instead 
which is ETH_P_XDSA.

The following does work though, tested with both br0 and a br0.1 upper:

+       upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
+       if (upper_dev) {
+               skb->vlan_proto = vlan_dev_vlan_proto(upper_dev);
+               return skb;
         }

I should have re-tested v2 and v3 with a bridge upper but I did not 
otherwise I would have caught that. If that sounds acceptable to you as 
well, I will submit that tomorrow.

Let me know what you think about the 802.1Q upper of a physical switch 
port in the other email.
-- 
Florian