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