netdev - Re: [RFC PATCH net-next 0/5] Replay and offload host VLAN entries in DSA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 13 Feb 2022 20:54:50 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, <netdev@...r.kernel.org>
CC:     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>,
        Vladimir Oltean <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Ido Schimmel <idosch@...dia.com>,
        Rafael Richter <rafael.richter@....de>,
        Daniel Klauer <daniel.klauer@....de>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [RFC PATCH net-next 0/5] Replay and offload host VLAN entries in
 DSA

On 09/02/2022 23:30, Vladimir Oltean wrote:
> The motivation behind these patches is that Rafael reported the
> following error with mv88e6xxx when the first switch port joins a
> bridge:
> 
> mv88e6085 0x0000000008b96000:00: port 0 failed to add a6:ef:77:c8:5f:3d vid 1 to fdb: -95 (-EOPNOTSUPP)
> 
> The FDB entry that's added is the MAC address of the bridge, in VID 1
> (the default_pvid), being replayed as part of br_add_if() -> ... ->
> nbp_switchdev_sync_objs().
> 
> -EOPNOTSUPP is the mv88e6xxx driver's way of saying that VID 1 doesn't
> exist in the VTU, so it can't program the ATU with a FID, something
> which it needs.
> 
> It appears to be a race, but it isn't, since we only end up installing
> VID 1 in the VTU by coincidence. DSA's approximation of programming
> VLANs on the CPU port together with the user ports breaks down with
> host FDB entries on mv88e6xxx, since that strictly requires the VTU to
> contain the VID. But the user may freely add VLANs pointing just towards
> the bridge, and FDB entries in those VLANs, and DSA will not be aware of
> them, because it only listens for VLANs on user ports.
> 
> To create a solution that scales properly to cross-chip setups and
> doesn't leak entries behind, some changes in the bridge driver are
> required. I believe that these are for the better overall, but I may be
> wrong. Namely, the same refcounting procedure that DSA has in place for
> host FDB and MDB entries can be replicated for VLANs, except that it's
> garbage in, garbage out: the VLAN addition and removal notifications
> from switchdev aren't balanced. So the first 3 patches attempt to deal
> with that.
> 
> This patch set has been superficially tested on a board with 3 mv88e6xxx
> switches in a daisy chain and appears to produce the primary desired
> effect - the driver no longer returns -EOPNOTSUPP when the first port
> joins a bridge, and is successful in performing local termination under
> a VLAN-aware bridge.
> As an additional side effect, it silences the annoying "p%d: already a
> member of VLAN %d\n" warning messages that the mv88e6xxx driver produces
> when coupled with systemd-networkd, and a few VLANs are configured.
> Furthermore, it advances Florian's idea from a few years back, which
> never got merged:
> https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/
> 

Hi,
I don't like the VLAN delete on simple flags change to workaround some devices'
broken behaviour, in general I'd like to avoid adding driver workarounds in the bridge.
Either those drivers should be fixed (best approach IMO), or the workaround should only
affect them, and not everyone. The point is that a vlan has much more state than a simple
fdb, and deleting it could result in a lot more unnecessary churn which can be avoided
if these flags can be changed properly. The host replay sounds good, but please re-work
the flags change logic and push the changes down where they're needed.

Thanks,
 Nik