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 - vlan_filtering=0 means absolutely no vlan processing, that is what everyone expects and breaking that expectation would break various use cases Less important, but still: - it is in the fast path for everyone - it can already be fixed by a tc action/8021q device 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
