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.