Re: [PATCH net v2 1/3] net/mlx5: Bridge, don't fail switchdev events of sibling eswitch ports

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] In this commit, mlx5_esw_bridge_lower_rep_port_lookup()
  (drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c:116-126)…

--

net/mlx5: Bridge, don't fail switchdev events of sibling eswitch ports

The mlx5 bridge offload registers switchdev notifiers per eswitch instance
while the notifier chains are global, so each instance must filter events
that aren't its own. This patch replaces the same-HW filter in the port
object and attribute handlers with a check that the instance actually
tracks the port, so sibling eswitch events no longer return -EINVAL to
user space.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> index baac38bece14a..56592e2e6b9b8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> @@ -104,6 +104,28 @@ mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(struct net_device *dev, struct m
>  	return NULL;
>  }
>  
> +static bool mlx5_esw_bridge_rep_port_lookup(struct net_device *dev,
> +					    struct mlx5_esw_bridge_offloads *br_offloads,
> +					    u16 *vport_num, u16 *esw_owner_vhca_id)
> +{
> +	if (!mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, br_offloads->esw, vport_num,
> +						       esw_owner_vhca_id))
> +		return false;
> +
> +	return mlx5_esw_bridge_port_exists(*vport_num, *esw_owner_vhca_id, br_offloads);
> +}
> +
> +static bool mlx5_esw_bridge_lower_rep_port_lookup(struct net_device *dev,
> +						  struct mlx5_esw_bridge_offloads *br_offloads,
> +						  u16 *vport_num, u16 *esw_owner_vhca_id)
> +{
> +	if (!mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(dev, br_offloads->esw, vport_num,
> +							     esw_owner_vhca_id))
> +		return false;
> +
> +	return mlx5_esw_bridge_port_exists(*vport_num, *esw_owner_vhca_id, br_offloads);
> +}
> +

[Severity: Low]
Should the tracked-port test happen inside the lower device walk rather
than after it?

mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() stops at the first
same-HCA representor it finds while walking the bridge's lower devices:

	netdev_for_each_lower_dev(dev, lower_dev, iter) {
		struct net_device *rep;

		if (netif_is_bridge_master(lower_dev))
			continue;

		rep = mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(lower_dev, esw, vport_num,
								      esw_owner_vhca_id);
		if (rep)
			return rep;
	}

Bridge-level attributes are notified with dev == the bridge netdevice, so
on a merged-eswitch HCA whose bridge holds representors of two eswitch
instances, the walk can return the sibling's representor first. If the
calling instance has no peer port for that representor - the late-start
case described in the commit message - mlx5_esw_bridge_port_exists()
fails and the whole lookup returns false, even though a later lower
device is a port this instance does track.

> @@ -283,8 +303,8 @@ mlx5_esw_bridge_port_obj_attr_set(struct net_device *dev,
>  	u16 vport_num, esw_owner_vhca_id;
>  	int err = 0;
>  
> -	if (!mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(dev, br_offloads->esw, &vport_num,
> -							     &esw_owner_vhca_id))
> +	if (!mlx5_esw_bridge_lower_rep_port_lookup(dev, br_offloads, &vport_num,
> +						   &esw_owner_vhca_id))
>  		return 0;
>  
>  	port_attr_info->handled = true;

Can this turn the previous -EINVAL into a silently dropped attribute
update for that instance?  mlx5_esw_bridge_port_obj_attr_set() returns 0
with port_attr_info->handled left false, so the ageing time, VLAN
filtering, VLAN protocol and MC-disabled updates never reach that
instance's own bridge object.  Each instance resolves its bridge through
the port, for example:

drivers/net/ethernet/mellanox/mlx5/core/esw/bridge.c:mlx5_esw_bridge_vlan_filtering_set() {
	bridge = mlx5_esw_bridge_from_port_lookup(vport_num, esw_owner_vhca_id, br_offloads);
	if (!bridge)
		return -EINVAL;
	...
}

so after e.g. "ip link set br0 type bridge vlan_filtering 1" the software
bridge would have filtering enabled while that instance's hardware bridge
keeps MLX5_ESW_BRIDGE_VLAN_FILTERING_FLAG clear, with no error reported.

The follow-up patch in this series, "net/mlx5: LAG, check if vport exists
on br_offloads", moves the mlx5_esw_bridge_port_exists() test into the
leaf of the recursive walk so an untracked representor returns NULL and
netdev_for_each_lower_dev() continues to the next lower device, which
removes this behaviour at the end of the series.  Would it make sense to
fold that change into this patch, or to note that this patch alone is not
suitable for backporting without it?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914085430.18694-1-bersoare%40isovalent.com