Re: [PATCH net-next] net: bridge: pop vlan from skb if filtering is disabled but it's a pvid

[Florian Fainelli <f.fainelli@gmail.com>]

On 9/14/2020 12:51 AM, Nikolay Aleksandrov wrote:
> On Sat, 2020-09-12 at 12:38 -0700, Florian Fainelli wrote:
>>
>> On 9/11/2020 11:56 PM, Nikolay Aleksandrov wrote:
>>> On Sat, 2020-09-12 at 02:16 +0300, Vladimir Oltean wrote:
>>>> Currently the bridge untags VLANs from its VLAN group in
>>>> __allowed_ingress() only when VLAN filtering is enabled.
>>>>
>>>> When installing a pvid in egress-tagged mode, DSA switches have a
>>>> problem:
>>>>
>>>> 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
>>>>
>>>> When adding a VLAN on a DSA switch interface, 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.
>>>>
>>>> Make the 2 cases (vlan_filtering 0 and 1) behave the same way by popping
>>>> the pvid, if the skb happens to be tagged with it, when vlan_filtering
>>>> is 0.
>>>>
>>>> There was an attempt to resolve this issue locally within the DSA
>>>> receive data path, but even though we can determine that we are under a
>>>> bridge with vlan_filtering=0, there are still some challenges:
>>>> - we cannot be certain that the skb will end up in the software bridge's
>>>>     data path, and for that reason, we may be popping the VLAN for
>>>>     nothing. Example: there might exist an 8021q upper with the same VLAN,
>>>>     or this interface might be a DSA master for another switch. In that
>>>>     case, the VLAN should definitely not be popped even if it is equal to
>>>>     the default_pvid of the bridge, because it will be consumed about the
>>>>     DSA layer below.
>>>
>>> Could you point me to a thread where these problems were discussed and why
>>> they couldn't be resolved within DSA in detail ?
>>>
>>>> - the bridge API only offers a race-free API for determining the pvid of
>>>>     a port, br_vlan_get_pvid(), under RTNL.
>>>>
>>>
>>> The API can be easily extended.
>>>
>>>> And in fact this might not even be a situation unique to DSA. Any driver
>>>> that receives untagged frames as pvid-tagged is now able to communicate
>>>> without needing an 8021q upper for the pvid.
>>>>
>>>
>>> I would prefer we don't add hardware/driver-specific fixes in the bridge, when
>>> vlan filtering is disabled there should be no vlan manipulation/filtering done
>>> by the bridge. This could potentially break users who have added 8021q devices
>>> as bridge ports. At the very least this needs to be hidden behind a new option,
>>> but I would like to find a way to actually push it back to DSA. But again adding
>>> hardware/driver-specific options should be avoided.
>>>
>>> Can you use tc to pop the vlan on ingress ? I mean the cases above are visible
>>> to the user, so they might decide to add the ingress vlan rule.
>>
>> We had discussed various options with Vladimir in the threads he points
>> out but this one is by far the cleanest and basically aligns the data
>> path when the bridge is configured with vlan_filtering=0 or 1.
>>
>> Some Ethernet switches supported via DSA either insist on or the driver
>> has been written in such a way that the default_pvid of the bridge which
>> is egress untagged for the user-facing ports is configured as egress
>> tagged for the CPU port. That CPU port is ultimately responsible for
>> bringing the Ethernet frames into the Linux host and the bridge data
>> path which is how we got VLAN tagged frames into the bridge, even if a
>> vlan_filtering=0 bridge is not supposed to.
>>
> 
> I read the thread and see your problem, but I still think that it must not be
> fixed in the bridge device, more below.
> 
>> We can solve this today by having a 802.1Q upper on top of the bridge
>> default to pop/push the default_pvid VLAN tag, but this is not obvious,
>> represents a divergence from a pure software bridge, and leads to
>> support questions. What is also utterly confusing for instance is that a
>> raw socket like one opened by a DHCP client will successfully allow br0
>> to obtain an IP address, but the data path still is not functional, as
>> you would need to use br0.1 to have a working data path taking care of
>> the VLAN tag.
>>
> 
> With this patch br0.1 won't work anymore for anyone having vlan_filtering=0.
> 
>> Vladimir had offered a DSA centric solution to this problem, which was
>> not that bad after all, but this one is also my favorite.
>>
> 
> I saw it, and it looks good. I saw the one of the main issues is not having an
> RCU get pvid helper. I can provide you with that to simplify the patch.
> 
>> Let us know when you are caught up on the thread and we can see how to
>> solve that the best way.
> 
> I'll start with why this patch is a non-starter, I'm guessing most of us already
> have guessed, but just to have them:
>   - the fix is DSA-specific, it must not reside in the bridge

Not necessarily any switch that forces egress tagging of a given PVID 
would fall in that category.

>   - vlan_filtering=0 means absolutely no vlan processing, that is what everyone
>     expects and breaking that expectation would break various use cases

OK

> 
> Less important, but still:
>   - it is in the fast path for everyone
>   - it can already be fixed by a tc action/8021q device

Sure, but the point is that it should be fixed in a way that is 
transparent to the user, as much as possible.

> 
> We can go into details but that would be a waste of time, instead I think we
> should focus on Vladimir's proposed DSA change.
> 
> Vladimir, I think with the right pvid helper the patch would reduce to
> dsa_untag_bridge_pvid() on the Rx path only. One thing that I'm curious about
> is shouldn't dsa_untag_bridge_pvid() check if the bridge pvid is == to the skb
> tag and the port's pvid?
> Since we can have the port's pvid different from the bridge's. That's for the
> case of vlan_filtering=1 and the port having that vlan, but not as pvid.
> 
> Thanks,
>   Nik
> 

-- 
Florian