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

Message-ID: <CAOiHx=m6Dqo4r9eaSSHDy5Zo8RxBY4DpE-qNeZXTjQRDAZMmaA@mail.gmail.com>
Date: Fri, 25 Apr 2025 09:30:17 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>, 
	"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 Thu, Apr 24, 2025 at 11:50 AM Jonas Gorski <jonas.gorski@...il.com> wrote:
>
> Hi,
>
> On Thu, Apr 24, 2025 at 11:15 AM Paolo Abeni <pabeni@...hat.com> wrote:
> >
> >
> >
> > On 4/22/25 8:49 PM, Jonas Gorski wrote:
> > > When a net device has NETIF_F_HW_VLAN_CTAG_FILTER set, the 8021q code
> > > will add VLAN 0 when enabling the device, and remove it on disabling it
> > > again.
> > >
> > > But since we are changing NETIF_F_HW_VLAN_CTAG_FILTER during runtime in
> > > dsa_user_manage_vlan_filtering(), user ports that are already enabled
> > > may end up with no VLAN 0 configured, or VLAN 0 left configured.
> >
> > Why this is a problem specifically for dsa and not a generic one? others
> > devices allow flipping the NETIF_F_HW_VLAN_CTAG_FILTER feature at runtime.
> >
> > AFAICS dsa_user_manage_vlan_filtering() is currently missing a call to
> > netdev_update_features(), why is that not sufficient nor necessary?
>
> Good point, I missed that (looked for something like this, but
> obviously didn't look hard enough). But checking the flow of it in the
> kernel ...
>
> netdev_update_features() for NETIF_F_HW_VLAN_CTAG_FILTER triggers a
> NETDEV_CVLAN_FILTER_PUSH_INFO notification, which would then trigger
> vlan_filter_push_vids(), which then calls vlan_add_rx_filter_info()
> for all configured vlans.
>
> This is more or less identical to what dsa does with its
> vlan_for_each(user, dsa_user_restore_vlan, user); call.
>
> And AFAICT it also has the same issue I am trying to fix here, that it
> does not install a VLAN 0 filter for devices that are already up,
> which it would have if the device had NETIF_F_HW_VLAN_CTAG_FILTER set
> when was the device was enabled (and vice versa on on down/remove).
>
> So I guess the course of action for a V2 is fixing this in the core
> vlan code and make vlan_filter_push_vids() / vlan_filter_drop_vids()
> take care of the VLAN 0 filter as well, and then make dsa use
> netdev_update_features() to simplify the code as well.
>
> Does that sound reasonable?

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