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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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
> $ 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>