Re: [PATCH RFC net 1/2] net: bridge: switchdev: do not notify new brentries as changed

[Jonas Gorski]

On Mon, Apr 14, 2025 at 1:39 PM Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
>
> On Sat, Apr 12, 2025 at 02:24:27PM +0200, Jonas Gorski wrote:
> > When adding a bridge vlan that is pvid or untagged after the vlan has
> > already been added to any other switchdev backed port, the vlan change
> > will be propagated as changed, since the flags change.
> >
> > This causes the vlan to not be added to the hardware for DSA switches,
> > since the DSA handler ignores any vlans for the CPU or DSA ports that
> > are changed.
> >
> > E.g. the following order of operations would work:
> >
> > $ ip link add swbridge type bridge vlan_filtering 1 vlan_default_pvid 1

as mentioned for the cover letter, I will fix the example to use
default_pvid 0 to have a "working" example.

> > $ ip link set lan1 master swbridge
> > $ bridge vlan add dev swbridge vid 1 pvid untagged self
> > $ bridge vlan add dev lan1 vid 1 pvid untagged
> >
> > but this order would brake:
>
> nitpick: s/brake/break/

Thanks, :set spell clearly didn't help here.

> > $ ip link add swbridge type bridge vlan_filtering 1 vlan_default_pvid 1
> > $ ip link set lan1 master swbridge
> > $ bridge vlan add dev lan1 vid 1 pvid untagged
> > $ bridge vlan add dev swbridge vid 1 pvid untagged self
> >
> > Additionally, the vlan on the bridge itself would become undeletable:
> >
> > $ bridge vlan
> > port              vlan-id
> > lan1              1 PVID Egress Untagged
> > swbridge          1 PVID Egress Untagged
> > $ bridge vlan del dev swbridge vid 1 self
> > $ bridge vlan
> > port              vlan-id
> > lan1              1 PVID Egress Untagged
> > swbridge          1 Egress Untagged
> >
> > since the vlan was never added to DSA's vlan list, so deleting it will
> > cause an error, causing the bridge code to not remove it.
> >
> > Fix this by checking if flags changed only for vlans that are already
> > brentry and pass changed as false for those that become brentries, as
> > these are a new vlan (member) from the switchdev point of view.
> >
> > Fixes: 8d23a54f5bee ("net: bridge: switchdev: differentiate new VLANs from changed ones")
> > Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx>
> > ---
> >  net/bridge/br_vlan.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> > index d9a69ec9affe..939a3aa78d5c 100644
> > --- a/net/bridge/br_vlan.c
> > +++ b/net/bridge/br_vlan.c
> > @@ -715,8 +715,8 @@ static int br_vlan_add_existing(struct net_bridge *br,
> >                               u16 flags, bool *changed,
> >                               struct netlink_ext_ack *extack)
> >  {
> > -     bool would_change = __vlan_flags_would_change(vlan, flags);
> >       bool becomes_brentry = false;
> > +     bool would_change = false;
> >       int err;
> >
> >       if (!br_vlan_is_brentry(vlan)) {
> > @@ -725,6 +725,8 @@ static int br_vlan_add_existing(struct net_bridge *br,
> >                       return -EINVAL;
> >
> >               becomes_brentry = true;
> > +     } else {
> > +             would_change = __vlan_flags_would_change(vlan, flags);
> >       }
> >
> >       /* Master VLANs that aren't brentries weren't notified before,
> > --
> > 2.43.0
> >
>
> You might want to mention that "bool *changed" is used later in
> br_process_vlan_info() to make a decision whether to call
> br_vlan_notify(RTM_NEWVLAN) or not. We want to notify switchdev with
> changed=false, but we want to keep notifying the change to rtnetlink.
>
> The rtnetlink notification still happens even if we don't set
> would_change here, because it depends on this code snippet:
>
>         if (becomes_brentry) {
>                 ...
>                 *changed = true;
>                 ...
>         }
>
> and not on this one:
>
>         if (would_change)
>                 *changed = true;
>
> That was my only concern with this change (I had missed the first snippet
> when initially reading the code), and I didn't see in the commit log
> some sort of attention paid to this detail.

Will add it. And I did check that, even considered shortly to merge this to

        if (becomes_brentry || would_change)
                *changed = true;

but then considered that a refactoring too much.

>
> With that:
>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

Thank you!