netdev - Re: [PATCH RFC net 0/2] net: dsa: fix handling brentry vlans with flags

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=mchprjNi8qQKsYzf18rE0NZxt-SjBRs9xnJEppcXUe7Q@mail.gmail.com>
Date: Sun, 13 Apr 2025 11:25:12 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Nikolay Aleksandrov <razor@...ckwall.org>, Ido Schimmel <idosch@...dia.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <vladimir.oltean@....com>, bridge@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net 0/2] net: dsa: fix handling brentry vlans with flags

On Sat, Apr 12, 2025 at 3:50 PM Jonas Gorski <jonas.gorski@...il.com> wrote:
>
> On Sat, Apr 12, 2025 at 3:34 PM Vladimir Oltean <olteanv@...il.com> wrote:
> >
> > On Sat, Apr 12, 2025 at 02:24:26PM +0200, Jonas Gorski wrote:
> > > While trying to figure out the hardware behavior of a DSA supported
> > > switch chip and printing various internal vlan state changes, I noticed
> > > that some flows never triggered adding the cpu port to vlans, preventing
> > > it from receiving any of the VLANs traffic.
> > >
> > > E.g. the following sequence would cause the cpu port not being member of
> > > the vlan, despite the bridge vlan output looking correct:
> > >
> > > $ ip link add swbridge type bridge vlan_filtering 1 vlan_default_pvid 1
> > > $ ip link set lan1 master swbridge
> >
> > At this step, dsa_port_bridge_join() -> switchdev_bridge_port_offload()
> > -> ... -> br_switchdev_port_offload() -> nbp_switchdev_sync_objs() ->
> > br_switchdev_vlan_replay() -> br_switchdev_vlan_replay_group(br_vlan_group(br))
> > -> br_switchdev_vlan_replay_one() should have notified DSA, with changed=false.
> > It should be processed by dsa_user_host_vlan_add() -> dsa_port_host_vlan_add().
> >
> > You make it sound like that doesn't happen.
>
> Yes, because I messed up writing down what I did. That should have
> been vlan_default_pvid 0 so there are no VLANs configured by default.
> >
> > I notice you didn't mention which "DSA supported chip" you are using.
> > By any chance, does its driver set ds->configure_vlan_while_not_filtering = false?
> > That would be my prime suspect, making dsa_port_skip_vlan_configuration() ignore
> > the code path above, because the bridge port is not yet VLAN filtering.
> > It becomes VLAN filtering only a bit later in dsa_port_bridge_join(),
> > with the dsa_port_switchdev_sync_attrs() -> dsa_port_vlan_filtering(br_vlan_enabled(br))
> > call.
> >
> > If that is the case, the only thing that is slightly confusing to me is
> > why you haven't seen the "skipping configuration of VLAN" extack message.
> > But anyway, if the theory above is true, you should instead be looking
> > at adding proper VLAN support to said driver, and drop this set instead,
> > because VLAN replay isn't working properly.
>
> It's b53, and on first glance it does not set it. But as I just
> noticed, I messed up the example.

So I had the chance to properly check and b53 does not unset it, so it
should be true.

> So adding the lan1 vid is supposed to be the very first vlan that is configured.
>
> >
> > > $ bridge vlan add dev lan1 vid 1 pvid untagged
> > > $ bridge vlan add dev swbridge vid 1 pvid untagged self
> > >
> > > Adding more printk debugging, I traced it br_vlan_add_existing() setting
> > > changed to true (since the vlan "gained" the pvid untagged flags), and
> > > then the dsa code ignoring the vlan notification, since it is a vlan for
> > > the cpu port that is updated.
> >
> > Yes, this part and everything that follows should be correct.

So as an addendum that I probably should have included in the cover
letter and commit message:

Starting with no vlans configured on the bridge:

$ bridge vlan add dev lan1 vid 1 pvid untagged
$ bridge vlan add dev swbridge vid 1 pvid untagged self

does not work, but

$ bridge vlan add dev lan1 vid 1 pvid untagged
$ bridge vlan add dev swbridge vid 1 self

does properly trigger a configuration of the vlan on the cpu port.

And the difference is that in the former, would_change =>
vlan->changed is true (due to "pvid untagged"), and in the latter it
is not.

Best regards,
Jonas