| Message ID | 20200923214038.3671566-2-f.fainelli@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: b53: Configure VLANs while not filtering | expand |
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.
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.
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.
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.
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.
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.
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.
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?
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.
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.
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.
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.
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);