Re: [PATCH net v2 2/3] net/mlx5: LAG, check if vport exists on br_offloads

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The commit message describes a bug that the code cannot exhibit,
  and the patch is a functional no-op for the scenario it names.
- [Low] The commit asserts a user-visible functional defect ('a bridge
  attribute change on a bonded uplink can silently no-op on the right…

--

net/mlx5: LAG, check if vport exists on br_offloads

This adds a mlx5_esw_bridge_port_exists() check to the LAG-master/rep
branch of mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(), so that a
rep which is not tracked by the calling instance's br_offloads is
rejected instead of returned.

> 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 56592e2e6b9b8..4b7b0a0fc2b2f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
> @@ -85,9 +85,16 @@ mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get(struct net_device *dev, struct m
>  	struct net_device *lower_dev;
>  	struct list_head *iter;
>  
> -	if (netif_is_lag_master(dev) || mlx5e_eswitch_rep(dev))
> -		return mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, esw, vport_num,
> -								 esw_owner_vhca_id);
> +	if (netif_is_lag_master(dev) || mlx5e_eswitch_rep(dev)) {
> +		struct net_device *rep;
> +
> +		rep = mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, esw, vport_num,
> +								esw_owner_vhca_id);
> +		if (rep && !mlx5_esw_bridge_port_exists(*vport_num, *esw_owner_vhca_id,
> +							esw->br_offloads))
> +			return NULL;
> +		return rep;
> +	}

[Severity: High]
Does this hunk change anything for the bonded-uplink case the changelog
describes?

The changelog says:

    mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() walks a LAG bond's
    lower devices and returns the first rep that's structurally eligible
    (same HCA, is a rep), without checking it's tracked by the calling
    instance's br_offloads. On a merged-eswitch HCA where a bond spans
    reps of more than one eswitch instance, this can return a sibling's
    rep instead of continuing to the one this instance actually owns

For dev being the LAG master, the bond walk happens in
mlx5_esw_bridge_lag_rep_get(), reached via
mlx5_esw_bridge_rep_vport_num_vhca_id_get():

	if (netif_is_lag_master(dev))
		dev = mlx5_esw_bridge_lag_rep_get(dev, esw);

and that walk already filters on the eswitch instance:

	netdev_for_each_lower_dev(dev, lower, iter) {
		if (!mlx5e_eswitch_rep(lower))
			continue;

		if (mlx5_esw_bridge_dev_same_esw(lower, esw))
			return lower;
	}

So can it actually return a sibling instance's rep here?  It looks like
only lowers satisfying mlx5_esw_bridge_dev_same_esw(lower, esw) are
accepted, which would make the "same HCA, is a rep" description apply to
mlx5_esw_bridge_rep_vport_num_vhca_id_get()'s non-LAG path rather than to
the bond walk.

The new branch is also terminal - on rejection it returns NULL and no
other bond member is examined - so what is "continued to" in the LAG case?

There is a second question about the result being identical before and
after.  The only caller is mlx5_esw_bridge_lower_rep_port_lookup(), which
repeats the same test on the same br_offloads:

	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);

With esw->br_offloads and br_offloads being the same object, a dev that is
a LAG master ends up with the same verdict either way, and
mlx5_esw_bridge_port_obj_attr_set() still returns 0 without applying the
attribute:

	if (!mlx5_esw_bridge_lower_rep_port_lookup(dev, br_offloads, &vport_num,
						   &esw_owner_vhca_id))
		return 0;

The behaviour that does change is the recursive lower-device walk further
down, for a dev that is neither a LAG master nor a rep (the bridge
netdevice itself, for bridge-wide attributes such as ageing time, VLAN
filtering, VLAN protocol and MC-disabled): an untracked sibling lower rep
no longer terminates the walk, so the walk can reach the lower rep this
instance does track.  Would it be clearer to describe that path in the
subject and changelog instead of the LAG bond?

[Severity: Low]
Should this patch carry a Fixes: tag?

The changelog claims a user-visible effect ("a bridge attribute change on
a bonded uplink can silently no-op on the right instance"), but there is
no Fixes: line, while the two sibling patches in the series both tag
Fixes: c358ea1741bc ("net/mlx5: Bridge, allow merged eswitch
connectivity").

The behaviour actually changed here (the recursive lower-device walk in
mlx5_esw_bridge_lower_rep_vport_num_vhca_id_get() stopping on an untracked
sibling rep) is a merged-eswitch bug too, so either adding the matching
Fixes: tag or rewording the message to say this is follow-up hardening of
the preceding patch with no independent user-visible effect would remove
the ambiguity.

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