Re: [PATCH net-next 03/13] net/mlx5: LAG, allocate v2p_map dynamically

[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: 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).

- [High] v2p_map and the scratch `ports` buffer in mlx5_modify_lag() are
  now sized ldev->ports * MLX5_LAG_MAX_HASH_BUCKETS, where ldev->ports…
- [Medium] Before this patch, mlx5_modify_lag()
  (drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:861) used a zeroed…

--

net/mlx5: LAG, allocate v2p_map dynamically

This commit turns the fixed-size v2p_map array in struct mlx5_lag into a
pointer allocated in mlx5_lag_dev_alloc(). The new size uses ldev->ports
instead of MLX5_MAX_PORTS. The scratch ports[] buffer in mlx5_modify_lag()
moves from the stack to a kcalloc() of the same size.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index c655f6e32e9b0..00b0159cb422f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c

[ ... ]

> @@ -330,11 +331,16 @@ static struct mlx5_lag *mlx5_lag_dev_alloc(struct mlx5_core_dev *dev)
>  	if (!ldev)
>  		return NULL;
>  
> +	ldev->ports = MLX5_CAP_GEN(dev, num_lag_ports);
> +	ldev->buckets = 1;
> +	ldev->v2p_map = kcalloc(ldev->ports * MLX5_LAG_MAX_HASH_BUCKETS,
> +				sizeof(*ldev->v2p_map), GFP_KERNEL);

[Severity: High]
Is ldev->ports the right bound for this allocation?

The users of v2p_map index it by the PF's slot in the ldev->pfs xarray, not
by a dense port number. For example, in mlx5_infer_tx_affinity_mapping():

	mlx5_ldev_for_each(i, 0, ldev) {
		for (j = 0; j < buckets; j++) {
			idx = i * buckets + j;
			ports[idx] = mlx5_lag_xa_to_dev_idx(ldev, i) + 1;
		}
	}

The slot comes from mlx5_ldev_add_mdev(), which allocates it from the full
MLX5_MAX_PORTS range:

	err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, MLX5_MAX_PORTS - 1),
		       GFP_KERNEL);
	...
	if (group_id)
		return 0;

	xa_set_mark(&ldev->pfs, idx, MLX5_LAG_XA_MARK_PORT);

sd_lag_init() adds Socket-Direct secondaries to the same xarray. They take
slots but never get the PORT mark. Given that, can a port PF end up at a
slot >= ldev->ports?

Here is an example. With num_lag_ports = 4, both buffers are now 64 bytes.
Say the SD primary takes slot 0 and its secondary takes slot 1. The fourth
port PF then lands in slot 4. Because ports > 2,
mlx5_lag_set_port_sel_mode() sets ldev->buckets to
MLX5_LAG_MAX_HASH_BUCKETS, so that PF would use indices 64..79.

Would this write past the end of v2p_map through
mlx5_activate_lag()->mlx5_infer_tx_affinity_mapping()? Would it also write
past the new ports[] buffer through mlx5_modify_lag()?

These readers look like they would go past the end the same way:
mlx5_modify_lag(), mlx5_lag_print_mapping(),
__mlx5_lag_modify_definers_destinations(),
mlx5_lag_create_port_sel_table() and mlx5_lag_get_slave_port(). The
out-of-bounds bytes would then be passed to mlx5_lag_pf_by_dev_idx() as
1-based device indices, and also used to program the port-selection table
and tx_remap_affinity.

Before this patch, both buffers were
MLX5_MAX_PORTS * MLX5_LAG_MAX_HASH_BUCKETS bytes, which covers every
possible slot. The rules[] array in struct mlx5_lag_definer (port_sel.h) is
indexed the same way and is still that size.

With this in mind, is the "No functional change" statement in the commit
message accurate? Should the buffers be sized by the slot range, or should
the indexing use the dense port sequence instead? Later patches in the
series still size both buffers by ldev->ports, so this does not appear to
be fixed later on.

> +	if (!ldev->v2p_map)
> +		goto err_v2p_map;

[ ... ]

> @@ -864,6 +873,12 @@ void mlx5_modify_lag(struct mlx5_lag *ldev,
>  		return;
>  
>  	dev0 = mlx5_lag_pf(ldev, first_idx)->dev;
> +
> +	ports = kcalloc(ldev->ports * MLX5_LAG_MAX_HASH_BUCKETS,
> +			sizeof(*ports), GFP_KERNEL);
> +	if (!ports)
> +		return;

[Severity: Medium]
Before this patch, mlx5_modify_lag() could not fail once first_idx was
valid. Now, if this kcalloc() fails, the function returns without a
message and skips all of the following:

  _mlx5_modify_lag()
  the v2p_map update
  mlx5_lag_drop_rule_setup()
  the MLX5_DRIVER_EVENT_ACTIVE_BACKUP_LAG_CHANGE_LOWERSTATE notifier

The drop-rule setup and the notifier are skipped even when the mapping has
not changed.

mlx5_modify_lag() returns void, so its callers cannot see the failure or
retry. That covers mlx5_do_bond() (called from mlx5_do_bond_work()) and
mlx5_lag_set_port_affinity() in mp.c. By this point ldev->tracker already
holds the new lower state.

Could the hardware TX affinity then keep pointing at a failed port until
some unrelated bond event arrives?

The buffer is at most MLX5_MAX_PORTS * MLX5_LAG_MAX_HASH_BUCKETS = 128
bytes. Could it stay on the stack, or be preallocated with v2p_map, so this
path does not gain a new failure mode?

> +
>  	mlx5_infer_tx_affinity_mapping(tracker, ldev, ldev->buckets, ports);

[ ... ]

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