[PATCH net] net/mlx5: Bridge, fix bogus MDB offload errors

[PATCH net] net/mlx5: Bridge, fix bogus MDB offload errors

[Tariq Toukan]

From: Gal Pressman <gal@nvidia.com>

MDB add operations are queued on the switchdev deferred queue
(SWITCHDEV_F_DEFER), while vlan object deletions are handled
synchronously.
A 'bridge vlan del' can overtake an MDB add that is still queued, and
the vlan is already gone by the time the driver is called.

The two operations are independent in practice: in my test, the MDB add
is triggered by assigning the VF an IPv6 address (hence joining a
solicited-node multicast group), while another process deletes the vlan
from its representor.

Racing 'bridge mdb add' against concurrent 'bridge vlan del' could
result in:

[ 5390.967651] mlx5_core 0000:08:00.0: E-Switch: Failed to lookup bridge port vlan metadata (vport=1)
[ 5390.969120] mlx5_core 0000:08:00.1: E-Switch: Failed to lookup bridge port vlan metadata to create MDB (MAC=33:33:ff:12:00:05,vid=2204,vport=1)
[ 5390.971676] mlx5_core 0000:08:00.1 eth4: Failed to add Port Multicast Database entry (object id=2) with error: -EINVAL (-22).
               Failure in updating the port's Multicast Database could lead to
               multicast forwarding issues.

The error cannot reach anyone that could do anything with it, switchdev
returned success to the bridge as soon as the MDB object was queued, and
the errno only flags the entry as offload failed.
The end result is correct either way, if the MDB has been offloaded
before the vlan was deleted, mlx5_esw_bridge_port_mdb_vlan_flush() would
have removed it.

The FDB path already treats this as a harmless race,
mlx5_esw_bridge_port_vlan_lookup() reports it on 'info' logging level
and skips the offload. mlxsw did the same in [1].

Skip the MDB offload and report on 'info' logging level instead.

[1] Commit 17b334a876fe ("mlxsw: spectrum_switchdev: Don't warn about valid situations")

Fixes: 70f0302b3f20 ("net/mlx5: Bridge, implement mdb offload")
Signed-off-by: Gal Pressman <gal@nvidia.com>
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/esw/bridge.c  | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
index b4cf3c5ac0dd..7c32dc9db098 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
@@ -1867,26 +1867,27 @@ int mlx5_esw_bridge_port_mdb_add(struct net_device *dev, u16 vport_num, u16 esw_
 
 	port = mlx5_esw_bridge_port_lookup(vport_num, esw_owner_vhca_id, br_offloads);
 	if (!port) {
-		esw_warn(br_offloads->esw->dev,
+		/* MDB is added asynchronously from the switchdev deferred
+		 * queue while port might have been deleted concurrently.
+		 */
+		esw_info(br_offloads->esw->dev,
 			 "Failed to lookup bridge port to add MDB (MAC=%pM,vport=%u)\n",
 			 addr, vport_num);
-		NL_SET_ERR_MSG_FMT_MOD(extack,
-				       "Failed to lookup bridge port to add MDB (MAC=%pM,vport=%u)",
-				       addr, vport_num);
-		return -EINVAL;
+		return 0;
 	}
 
 	bridge = port->bridge;
 	if (bridge->flags & MLX5_ESW_BRIDGE_VLAN_FILTERING_FLAG && vid) {
 		vlan = mlx5_esw_bridge_vlan_lookup(vid, port);
 		if (!vlan) {
-			esw_warn(br_offloads->esw->dev,
+			/* MDB is added asynchronously from the switchdev
+			 * deferred queue while vlan might have been deleted
+			 * concurrently.
+			 */
+			esw_info(br_offloads->esw->dev,
 				 "Failed to lookup bridge port vlan metadata to create MDB (MAC=%pM,vid=%u,vport=%u)\n",
 				 addr, vid, vport_num);
-			NL_SET_ERR_MSG_FMT_MOD(extack,
-					       "Failed to lookup vlan metadata for MDB (MAC=%pM,vid=%u,vport=%u)",
-					       addr, vid, vport_num);
-			return -EINVAL;
+			return 0;
 		}
 	}
 

base-commit: b6ef0d965fe8ce0ee4b4dcb58b3511f83003e42d
-- 
2.44.0

Re: [PATCH net] net/mlx5: Bridge, fix bogus MDB offload errors

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c,
  mlx5_esw_bridge_port_mdb_add() now returns 0 when the bridge port…

--

net/mlx5: Bridge, fix bogus MDB offload errors

This patch changes mlx5_esw_bridge_port_mdb_add() so that it returns 0
instead of -EINVAL when the bridge port or the port vlan cannot be found.
It also lowers the log level from warn to info. The goal is to stop the
error messages that appear when a 'bridge vlan del' overtakes a deferred
MDB add.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
> index b4cf3c5ac0dde..7c32dc9db098e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c
> @@ -1867,26 +1867,27 @@ int mlx5_esw_bridge_port_mdb_add(struct net_device *dev, u16 vport_num, u16 esw_
>  
>  	port = mlx5_esw_bridge_port_lookup(vport_num, esw_owner_vhca_id, br_offloads);
>  	if (!port) {
> -		esw_warn(br_offloads->esw->dev,
> +		/* MDB is added asynchronously from the switchdev deferred
> +		 * queue while port might have been deleted concurrently.
> +		 */
> +		esw_info(br_offloads->esw->dev,
>  			 "Failed to lookup bridge port to add MDB (MAC=%pM,vport=%u)\n",
>  			 addr, vport_num);
> -		NL_SET_ERR_MSG_FMT_MOD(extack,
> -				       "Failed to lookup bridge port to add MDB (MAC=%pM,vport=%u)",
> -				       addr, vport_num);
> -		return -EINVAL;
> +		return 0;
>  	}
>  
>  	bridge = port->bridge;
>  	if (bridge->flags & MLX5_ESW_BRIDGE_VLAN_FILTERING_FLAG && vid) {
>  		vlan = mlx5_esw_bridge_vlan_lookup(vid, port);
>  		if (!vlan) {
> -			esw_warn(br_offloads->esw->dev,
> +			/* MDB is added asynchronously from the switchdev
> +			 * deferred queue while vlan might have been deleted
> +			 * concurrently.
> +			 */
> +			esw_info(br_offloads->esw->dev,
>  				 "Failed to lookup bridge port vlan metadata to create MDB (MAC=%pM,vid=%u,vport=%u)\n",
>  				 addr, vid, vport_num);
> -			NL_SET_ERR_MSG_FMT_MOD(extack,
> -					       "Failed to lookup vlan metadata for MDB (MAC=%pM,vid=%u,vport=%u)",
> -					       addr, vid, vport_num);
> -			return -EINVAL;
> +			return 0;
>  		}
>  	}

[Severity: Medium]
Both of these early exits now return 0. Will the bridge then report the
MDB entry as offloaded, even though nothing was programmed in the
eswitch?

The return value passes unchanged through mlx5_esw_bridge_port_obj_add()
and switchdev_port_obj_add_deferred() to the completion callback:

net/bridge/br_switchdev.c:br_switchdev_mdb_complete() {
	if (err == -EOPNOTSUPP)
		goto out_free;
    ...
		old_flags = p->flags;
		br_multicast_set_pg_offload_flags(p, !err);
    ...
}

When err == 0, br_multicast_set_pg_offload_flags() sets
MDB_PG_FLAGS_OFFLOAD, and __mdb_entry_fill_flags() exports it to user
space as MDB_FLAGS_OFFLOAD. 'bridge mdb show' and RTM_NEWMDB dumps would
then show 'offload' for an entry that has no hardware rule. Before this
patch, the same entry showed 'offload_failed'.

The entry also stays in the bridge MDB after the vlan del race.
__vlan_del() removes port groups only through
br_multicast_toggle_one_vlan(). That function does nothing unless
per-vlan snooping (BR_VLFLAG_MCAST_ENABLED) is enabled. So the wrong flag
stays until the entry times out, and for a permanent entry it never goes
away.

The !vlan branch can also be hit without any race:

- With per-vlan snooping off, br_mdb_add() accepts 'bridge mdb add ...
  vid X' for a VID that is not configured on the port.

- mlx5_esw_bridge_port_vlan_add() can return -EOPNOTSUPP when the
  reformat caps are missing. __vlan_vid_add() then falls back to
  vlan_vid_add(), and the bridge keeps a vlan that mlx5 never created.

The result also differs from mlx5_esw_bridge_port_mdb_attach(). That
function already uses -EOPNOTSUPP for the not-offloaded case:

	if (!(bridge->flags & MLX5_ESW_BRIDGE_MCAST_FLAG))
		return -EOPNOTSUPP;

The new return 0 runs before that check. With multicast offload disabled,
the same unoffloaded entry would be marked offloaded when the vlan is
missing, and left untouched when the vlan is present.

The commit message says:

    the errno only flags the entry as offload failed.
    The end result is correct either way, ...

That does not cover the case where the entry is now flagged as offloaded.

The FDB precedent also seems to work differently. FDB offload state is
set only by an explicit SWITCHDEV_FDB_OFFLOADED notification, so skipping
an FDB offload does not leave a flag behind.

Would returning -EOPNOTSUPP here work instead?
switchdev_port_obj_add_deferred() does not log -EOPNOTSUPP, so the bogus
netdev_err would still go away. br_switchdev_mdb_complete() would also
leave the flags untouched.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021122115.3737284-1-tariqt%40nvidia.com