Re: [PATCH net-next 08/15] net/mlx5: LAG: replace fixed MLX5_MAX_PORTS arrays with dynamic allocation

[Shay Drori <shayd@nvidia.com>]

On 03/11/2026 19:10, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
> 
> Converts two fixed-size arrays sized to MLX5_MAX_PORTS into
> dynamically allocated pointers:
> 
> - lag_mpesw.pf_metadata in mpesw.c/mpesw.h, sized by ldev->max_fns
> - mdev array in mlx5_lag_query_cong_counters(), sized by ldev->ports
> 
> The cong-counters array is sized by ldev->ports, which is fixed at ldev
> allocation, rather than by the LAG mode, so a mode change between the
> allocation and filling the array cannot overflow it. The query loop is
> bounded by the number of entries actually filled. A virt LAG is not
> aggregated and queries only the given device, as its members are not
> LAG ports.
> 
> Thus, removes the hard dependency on MLX5_MAX_PORTS.
> 
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 15 +++++++--------
>   .../net/ethernet/mellanox/mlx5/core/lag/mpesw.c   | 12 ++++++++++++
>   .../net/ethernet/mellanox/mlx5/core/lag/mpesw.h   |  2 +-
>   3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 2b76d119cfa6..b873eb91fd0d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -2917,7 +2917,9 @@ int mlx5_lag_query_cong_counters(struct mlx5_core_dev *dev,
>   	if (!out)
>   		return -ENOMEM;
>   
> -	mdev = kvzalloc(sizeof(mdev[0]) * MLX5_MAX_PORTS, GFP_KERNEL);
> +	ldev = mlx5_lag_dev(dev);
> +	num_ports = ldev ? max_t(int, ldev->ports, 1) : 1;
> +	mdev = kcalloc(num_ports, sizeof(mdev[0]), GFP_KERNEL);
>   	if (!mdev) {
>   		ret = -ENOMEM;
>   		goto free_out;
> @@ -2926,20 +2928,17 @@ int mlx5_lag_query_cong_counters(struct mlx5_core_dev *dev,
>   	memset(values, 0, sizeof(*values) * num_counters);
>   
>   	spin_lock_irqsave(&lag_lock, flags);
> -	ldev = mlx5_lag_dev(dev);
> -	if (ldev && __mlx5_lag_is_active(ldev)) {
> -		num_ports = ldev->ports;
> +	if (ldev && __mlx5_lag_is_active(ldev) && !ldev->virt_lag) {
>   		mlx5_ldev_for_each(i, 0, ldev) {
>   			fn = mlx5_lag_fn(ldev, i);
>   			mdev[idx++] = fn->dev;
>   		}


answers to net-dev sashiko

Sashiko (Medium):
Can this loop write past the end of mdev[]?
The array now has max(ldev->ports, 1) entries. mlx5_ldev_for_each()
writes one entry for each member that has MLX5_LAG_XA_MARK_PORT, and
nothing checks idx against num_ports.
mlx5_ldev_add_mdev() puts the PORT mark on every group_id == 0 member
and does not cap the count at ldev->ports:
     if (group_id)
         return 0;
     xa_set_mark(&ldev->fns, idx, MLX5_LAG_XA_MARK_PORT);
The member count is compared with ldev->ports only at activation time, in
mlx5_lag_check_prereq(). __mlx5_lag_dev_add_mdev() refuses a join only
when mode_changes_in_progress is set.
If an active non-virt LAG ever had more PORT-marked members than
ldev->ports, this loop would write struct mlx5_core_dev pointers past the
kcalloc'd buffer while holding lag_lock with IRQs off. I couldn't confirm
that such a configuration happens on real hardware. mapping_show() in
debugfs.c makes the same kcalloc(ldev->ports) assumption.
The commit message says:
     The cong-counters array is sized by ldev->ports, which is fixed at ldev
     allocation, rather than by the LAG mode, so a mode change between the
     allocation and filling the array cannot overflow it.
The old MLX5_MAX_PORTS sizing left headroom here. Should the loop have
something like "if (idx >= num_ports) break;", maybe with WARN_ON_ONCE(),
so the code enforces that guarantee?

[SD] It can't. The loop runs only for a non-virt LAG, and the
PORT-marked members of such an ldev are the PFs of one card, at most
num_lag_ports == ldev->ports of them (an SD secondary is not PORT-marked
and a virt member is excluded by the !ldev->virt_lag check right here).



>   	} else {
> -		num_ports = 1;
> -		mdev[MLX5_LAG_P1] = dev;
> +		mdev[idx++] = dev;
>   	}
>   	spin_unlock_irqrestore(&lag_lock, flags);
>   
> -	for (i = 0; i < num_ports; ++i) {
> +	for (i = 0; i < idx; ++i) {
>   		u32 in[MLX5_ST_SZ_DW(query_cong_statistics_in)] = {};
>   
>   		MLX5_SET(query_cong_statistics_in, in, opcode,
> @@ -2954,7 +2953,7 @@ int mlx5_lag_query_cong_counters(struct mlx5_core_dev *dev,
>   	}
>   
>   free_mdev:
> -	kvfree(mdev);
> +	kfree(mdev);
>   free_out:
>   	kvfree(out);
>   	return ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> index 6e3bef276611..584436668a5c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> @@ -15,6 +15,9 @@ static void mlx5_mpesw_metadata_cleanup(struct mlx5_lag *ldev)
>   	u32 pf_metadata;
>   	int i;
>   
> +	if (!ldev->lag_mpesw.pf_metadata)
> +		return;
> +
>   	mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
>   		dev = mlx5_lag_fn(ldev, i)->dev;
>   		esw = dev->priv.eswitch;
> @@ -27,6 +30,9 @@ static void mlx5_mpesw_metadata_cleanup(struct mlx5_lag *ldev)
>   		mlx5_esw_match_metadata_free(esw, pf_metadata);
>   		ldev->lag_mpesw.pf_metadata[i] = 0;
>   	}
> +
> +	kfree(ldev->lag_mpesw.pf_metadata);
> +	ldev->lag_mpesw.pf_metadata = NULL;
>   }
>   
>   static int mlx5_mpesw_metadata_set(struct mlx5_lag *ldev)
> @@ -36,6 +42,12 @@ static int mlx5_mpesw_metadata_set(struct mlx5_lag *ldev)
>   	u32 pf_metadata;
>   	int i, err;
>   
> +	ldev->lag_mpesw.pf_metadata =
> +		kcalloc(ldev->max_fns, sizeof(*ldev->lag_mpesw.pf_metadata),
> +			GFP_KERNEL);
> +	if (!ldev->lag_mpesw.pf_metadata)
> +		return -ENOMEM;
> +

Sashiko (High):
Can mlx5_mpesw_metadata_cleanup() index past the end of pf_metadata[]?
This buffer is sized by ldev->max_fns at the time MPESW is enabled, and
the length is not saved anywhere. The cleanup loop stops at the current
max_fns instead:
     mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
         ...
         pf_metadata = ldev->lag_mpesw.pf_metadata[i];
         ...
         mlx5_esw_match_metadata_free(esw, pf_metadata);
         ldev->lag_mpesw.pf_metadata[i] = 0;
     }
max_fns only ever grows, in mlx5_ldev_add_mdev():
     if (idx >= ldev->max_fns)
         ldev->max_fns = idx + 1;
sd_lag_init() adds SD secondaries without checking ldev->mode. These
have group_id != 0, so MLX5_LAG_FILTER_ALL includes them:
     mlx5_sd_for_each_secondary(i, primary, pos) {
         err = mlx5_ldev_add_mdev(ldev, pos, sd->group_id);
One possible sequence:
   esw_multiport devlink param set on a phys LAG before the SD group forms
     mlx5_devlink_esw_multiport_set()
       mlx5_lag_mpesw_enable()
         mlx5_mpesw_metadata_set()
           kcalloc(ldev->max_fns, ...)   /* e.g. 2 entries */
   SD secondary eth aux device probes
     mlx5e_probe()->mlx5_sd_init()->sd_lag_init()
       mlx5_ldev_add_mdev()   /* idx 2, 3, ..., max_fns grows */
   esw_multiport cleared, or eswitch mode change
     mlx5_lag_disable_mpesw()->mlx5_mpesw_metadata_cleanup()
       reads and writes pf_metadata[2..] out of bounds
If a stray value read there is non-zero, cleanup would also reprogram
the uplink ingress ACL of a device that never took part in MPESW and
send it a MULTIPORT_ESW event. It would also call ida_free() on an
arbitrary metadata id that another vport may still be using.
The old zeroed u32[MLX5_MAX_PORTS] array absorbed this growth for
indices below MLX5_MAX_PORTS.
Later in the series, "net/mlx5: LAG, do not add a device to an
already-active LAG" only rejects virt-LAG-capable devices in
__mlx5_lag_dev_add_mdev(). sd_lag_init() is unchanged, so this appears
to remain at the end of the series.
Could the allocated length be stored and used to bound the cleanup loop?
Alternatively, could membership growth be refused, or the buffer
reallocated, while MPESW is active?

[SD] this is pre-existing bug, MPESW should failed if SD Secondary
is down. this will be handle in separate patch.


>   	mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
>   		dev = mlx5_lag_fn(ldev, i)->dev;
>   		esw = dev->priv.eswitch;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.h
> index 5099723ba0f7..6296e3edd38c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.h
> @@ -9,7 +9,7 @@
>   
>   struct lag_mpesw {
>   	struct work_struct mpesw_work;
> -	u32 pf_metadata[MLX5_MAX_PORTS];
> +	u32 *pf_metadata;
>   };
>   
>   enum mpesw_op {