Date:   Mon, 14 Sep 2020 07:51:33 +0000
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "olteanv@...il.com" <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net-next] net: bridge: pop vlan from skb if filtering is
 disabled but it's a pvid

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

Powered by blists - more mailing lists