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
$ 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:
$ 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@gmail.com>
---
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.0On 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
> $ 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/
>
> $ 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@gmail.com>
> ---
> 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.
With that:
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
On Mon, Apr 14, 2025 at 1:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> 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@gmail.com>
> > ---
> > 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@nxp.com>
Thank you!
On Mon, Apr 14, 2025 at 02:11:18PM +0200, Jonas Gorski wrote: > On Mon, Apr 14, 2025 at 1:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > $ 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. Please do so. You have consistently given examples with vlan_default_pvid 1 throughout the patch set. > 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. I agree that it would be an unnecessary change for 'net'. You can send a refactoring patch for net-next after Thursday, though.
© 2016 - 2025 Red Hat, Inc.