linux-kernel - Re: [PATCH net] net: dsa: fix VLAN 0 filter imbalance when toggling filtering

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] [day] [month] [year] [list]

Message-ID: <CAOiHx=keOAWqF4Atzqx4VZW+xAccO=WtWCOoVoEPR9iFrDf_zw@mail.gmail.com>
Date: Sat, 26 Apr 2025 17:48:26 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>, 
	Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: fix VLAN 0 filter imbalance when toggling filtering

On Fri, Apr 25, 2025 at 9:51 AM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Fri, Apr 25, 2025 at 09:30:17AM +0200, Jonas Gorski wrote:
> > After looking into it a bit more, netdev_update_features() does not
> > relay any success or failure, so there is no way for DSA to know if it
> > succeded or not. And there are places where we temporarily want to
> > undo all configured vlans, which makes it hard to do via
> > netdev_update_features().
> >
> > Not sure anymore if this is a good way forward, especially if it is
> > just meant to fix a corner case. @Vladimir, what do you think?
> >
> > I'd probably rather go forward with the current fix (+ apply it as
> > well for the vlan core code), and do the conversion to
> > netdev_update_features() at later time, since I see potential for
> > unexpected breakage.
> >
> > Best regards,
> > Jonas
>
> I see the inconsistency you're trying to fix, but I'm still wondering
> whether it is the fix that b53 requires, given the fact that it doesn't
> seem to otherwise depend on 8021q to set up or modify VID 0. I would say
> I don't yet have a fully developed opinion and I am waiting for you to
> provide the result of the modified bridge_vlan_aware selftest,
> specifically drop_untagged().

It does need a lot more fixes on top of that. With this patch applied:

TEST: Reception of 802.1p-tagged traffic                            [ OK ]
TEST: Dropping of untagged and 802.1p-tagged traffic with no PVID   [FAIL]
        802.1p-tagged reception succeeded, but should have failed

The latter is no surprise, since b53 does not handle non filtering
bridges correctly, or toggling filtering at runtime.

I fixed most issues I found in b53 and it now succeeds in WIP code I
have (and most other tests from there).

One thing I struggled a bit is that the second test tests four
different scenarios, but only has one generic failure message, so a
failure does not tell which of the four setups failed.

The issues I fixed so far locally:

1. b53 programs the vlan table based on bridge vlans regardless if
filtering is on or not
2. b53 allows vlan 0 to be modified from
dsa_switch_ops::port_vlan_{add,remove} for bridged ports
3. b53 adds vlan 0 to a port when it leaves a bridge, but does not
remove it on join
4. b53 does not handle switching a vlan from pvid to non-pvid
5. stp (and other reserved multicast) requires a PVID vlan.

This makes especially non-filtering bridges not work as expected, or
the switch in any way after adding and then removing a filtering
bridge.

Best regards,
Jonas