Date:   Mon, 4 Oct 2021 11:16:22 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net 1/2] net: dsa: tag_dsa: send packets with TX fwd
 offload from VLAN-unaware bridges using VID 0

On Mon, Oct 04, 2021 at 12:55:27PM +0200, Tobias Waldekranz wrote:
> On Mon, Oct 04, 2021 at 01:23, Vladimir Oltean <vladimir.oltean@....com> wrote:
> > The present code is structured this way due to an incomplete thought
> > process. In Documentation/networking/switchdev.rst we document that if a
> > bridge is VLAN-unaware, then the presence or lack of a pvid on a bridge
> > port (or on the bridge itself, for that matter) should not affect the
> > ability to receive and transmit tagged or untagged packets.
> >
> > If the bridge on behalf of which we are sending this packet is
> > VLAN-aware, then the TX forwarding offload API ensures that the skb will
> > be VLAN-tagged (if the packet was sent by user space as untagged, it
> > will get transmitted town to the driver as tagged with the bridge
> > device's pvid). But if the bridge is VLAN-unaware, it may or may not be
> > VLAN-tagged. In fact the logic to insert the bridge's PVID came from the
> > idea that we should emulate what is being done in the VLAN-aware case.
> > But we shouldn't.
> 
> IMO, the problem here stems from a discrepancy between LinkStreet
> devices and the bridge, in how PVID is interpreted. For the bridge, when
> VLAN filtering is disabled, ingressing traffic will be assigned to VID
> 0. This is true even if the port's PVID is set. A mv88e6xxx port who's
> QMode bits are set to 00 (802.1Q disabled) OTOH, will assign ingressing
> traffic to its PVID.
> 
> So, in order to match the bridge's behavior, I think we need to rethink
> how mv88e6xxx deals with non-filtering bridges. At first, one might be
> tempted to simply leave the hardware PVID at 0. The PVT can then be used
> to create isolation barriers between different bridges. ATU isolation is
> really what kills this approach. Since there is no VLAN information in
> the tag, there is no way to separate flows from different bridges into
> different FIDs. This is the issue I discovered with the forward
> offloading series.
> 
> > It appears that injecting packets using a VLAN ID of 0 serves the
> > purpose of forwarding the packets to the egress port with no VLAN tag
> > added or stripped by the hardware, and no filtering being performed.
> > So we can simply remove the superfluous logic.
> 
> The problem with this patch is that return traffic from the CPU is sent
> asymmetrically over a different VLAN, which in turn means that it will
> perform the DA lookup in a different FID (0). The result is that traffic
> does flow, but for the wrong reason. CPU -> port traffic is now flooded
> as unknown unicast. An example:
> 
> (:aa / 10.1)
>     br0
>    /   \
> sw0p1 sw0p2
> \         /
>  \       /
>   \     /
>     CPU
>      |
>   .--0--.
>   | sw0 |
>   '-1-2-'
>     | '-- sniffer
>     '---- host (:bb / 10.2)
> 
> br0 is created using the default settings. sw0 will have (among others)
> static entries for the CPU:
> 
>     fid:0 addr:aa type:static port:0
>     fid:1 addr:aa type:static port:0
> 
> 1. host sends an ARP for 10.1.
> 
> 2. sw0 will add this entry (since vlan_default_pvid is 1):
> 
>     fid:1 addr:bb type:age-7 port:1

Well, that's precisely mv88e6xxx's problem, it should not make its
ports' pvid inherit that of the bridge if the bridge is not VLAN aware.
Other drivers inherit the bridge pvid only when VLAN filtering is turned
on. See sja1105, ocelot, mt7530 at the very least. So the entry should
have been learned in FID 0 here.

> 3. CPU replies with a FORWARD (VID 0).
> 
> 4. sw0 will perform a DA lookup in FID 0, missing the entry learned in
>    step 2.
> 
> 5. sw0 floods the frame as unknown unicast to both host and sniffer.
> 
> Conversely, if flooding of unknown unicast is disabled on sw0p1:
> 
>     $ bridge link set dev sw0p1 flood off
> 
> host can no longer communicate with the CPU.
> 
> As I alluded to in the forward offloading thread, I think we need to
> move a scheme where:
> 
> 1. mv88e6xxx clears ds->configure_vlan_while_not_filtering.

No, that's the wrong answer, nobody should clear ds->configure_vlan_while_not_filtering.
mv88e6xxx should leave the pvid at zero* when joining a bridge that is
not VLAN-aware. It should inherit the bridge pvid when that bridge
becomes VLAN-aware, and it should reset the pvid to zero* when that
bridge becomes VLAN-unaware.

> 2. Assigns a free VID (and by extension a FID) in the VTU to each
>    non-filtering bridge.

*with the mention that the pvid of zero will only solve the first half
of the problem, the discrepancy between the VLAN classified on xmit and
the VLAN classified on rcv.

It will not solve the ATU (FDB) isolation problem. But to solve the FDB
isolation problem you need this:
https://patchwork.kernel.org/project/netdevbpf/cover/20210818120150.892647-1-vladimir.oltean@nxp.com/

> With this in place, the tagger could use the VID associated with the
> egressing port's bridge in the tag.

So the patch is not incorrect, it is incomplete. And there's nothing
further I can add to the tagger logic to make it more complete, at least
not now.

That's one of the reasons why this is merely a "part 1".

Powered by blists - more mailing lists