netdev - Re: [PATCH v2 net] net: dsa: fix switchdev objects on bridge master mistakenly being applied on ports

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 for Android: free password hash cracker in your pocket

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

Message-ID: <874khnq9hh.fsf@waldekranz.com>
Date:   Sun, 07 Mar 2021 16:17:14 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v2 net] net: dsa: fix switchdev objects on bridge master mistakenly being applied on ports

On Sun, Mar 07, 2021 at 12:21, Vladimir Oltean <olteanv@...il.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> Tobias reports that after the blamed patch, VLAN objects being added to
> a bridge device are being added to all slave ports instead (swp2, swp3).
>
> ip link add br0 type bridge vlan_filtering 1
> ip link set swp2 master br0
> ip link set swp3 master br0
> bridge vlan add dev br0 vid 100 self
>
> This is because the fix was too broad: we made dsa_port_offloads_netdev
> say "yes, I offload the br0 bridge" for all slave ports, but we didn't
> add the checks whether the switchdev object was in fact meant for the
> physical port or for the bridge itself. So we are reacting on events in
> a way in which we shouldn't.
>
> The reason why the fix was too broad is because the question itself,
> "does this DSA port offload this netdev", was too broad in the first
> place. The solution is to disambiguate the question and separate it into
> two different functions, one to be called for each switchdev attribute /
> object that has an orig_dev == net_bridge (dsa_port_offloads_bridge),
> and the other for orig_dev == net_bridge_port (*_offloads_bridge_port).
>
> In the case of VLAN objects on the bridge interface, this solves the
> problem because we know that VLAN objects are per bridge port and not
> per bridge. And when orig_dev is equal to the net_bridge, we offload it
> as a bridge, but not as a bridge port; that's how we are able to skip
> reacting on those events. Note that this is compatible with future plans
> to have explicit offloading of VLAN objects on the bridge interface as a
> bridge port (in DSA, this signifies that we should add that VLAN towards
> the CPU port).
>
> Fixes: 99b8202b179f ("net: dsa: fix SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING getting ignored")
> Reported-by: Tobias Waldekranz <tobias@...dekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

Please wait before applying.

I need to do some more testing later (possibly tomorrow). But I am
pretty sure that this patch does not work with the (admittedly somewhat
exotic) combination of:

- Non-offloaded LAG
- Bridge with VLAN filtering enabled.

When adding the LAG to the bridge, I get an error because mv88e6xxx
tries to add VLAN 1 to the ports (which it should not do as the LAG is
not offloaded).