Re: [PATCH net-next 06/13] net/mlx5: LAG, size debugfs buffers by port count

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] mapping_show() in
  drivers/net/ethernet/mellanox/mlx5/core/lag/debugfs.c used a stack…

--

net/mlx5: LAG, size debugfs buffers by port count

This commit replaces the fixed-size ports[MLX5_MAX_PORTS] stack array in
mapping_show() with a buffer allocated by kcalloc(), sized by
ldev->ports.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/debugfs.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/debugfs.c
> index e9b8d79d2d21e..44296789d6567 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/debugfs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/debugfs.c
> @@ -101,14 +101,18 @@ static int flags_show(struct seq_file *file, void *priv)
>  static int mapping_show(struct seq_file *file, void *priv)
>  {
>  	struct mlx5_core_dev *dev = file->private;
> -	u8 ports[MLX5_MAX_PORTS] = {};
>  	struct mlx5_lag *ldev;
>  	bool hash = false;
>  	bool lag_active;
>  	int i, idx = 0;
>  	int num_ports;
> +	u8 *ports;
>  
>  	ldev = mlx5_lag_dev(dev);
> +	ports = kcalloc(ldev->ports, sizeof(*ports), GFP_KERNEL);

[Severity: Low]
Can the loops that fill this buffer write past ldev->ports entries?

Neither fill path is limited by ldev->ports. The non-hash path is:

	mlx5_ldev_for_each(i, 0, ldev)
		ports[idx++] = ldev->v2p_map[i];

and idx is never compared to the allocation size. The hash path passes
the buffer to mlx5_infer_tx_enabled(), which takes no length:

	mlx5_ldev_for_each(i, 0, ldev)
		if (tracker->netdev_state[i].tx_enabled &&
		    tracker->netdev_state[i].link_up)
			ports[(*num_enabled)++] = i;

If no port is enabled, it falls back to mlx5_infer_tx_disabled(), which
also writes one entry per port. mlx5_ldev_for_each() goes through
mlx5_lag_for_each(), and that loop stops at tmp < MLX5_MAX_PORTS, not
at ldev->ports.

The two bounds come from different places:

  mlx5_lag_dev_alloc():
	ldev->ports = MLX5_CAP_GEN(dev, num_lag_ports);

  mlx5_ldev_add_mdev():
	err = xa_alloc(&ldev->pfs, &idx, pf, XA_LIMIT(0, MLX5_MAX_PORTS - 1),
		       GFP_KERNEL);
	...
	xa_set_mark(&ldev->pfs, idx, MLX5_LAG_XA_MARK_PORT);

ldev->ports is read from the first device's capability. The number of
PORT-marked entries in ldev->pfs is capped only by MLX5_MAX_PORTS. As far
as I can see, the only comparison against ldev->ports is
mlx5_lag_num_devs(ldev) != ldev->ports in mlx5_lag_check_prereq(), and
that runs only when LAG is activated.

The old stack array was safe by construction, because it and the xarray
limit were both MLX5_MAX_PORTS. The new buffer relies on an unstated
invariant: ldev->pfs never holds more port PFs than num_lag_ports.

If more port PFs than ldev->ports join the same ldev while LAG is
active, reading the debugfs mapping file would write past the requested
size. One such window is after a new PF is added and before bond work
tears LAG down. Since the count is capped at 8 and the smallest kmalloc
object is 8 bytes, the extra bytes stay inside the slab object. On
debug kernels this would still show up as a KASAN slab-out-of-bounds or
slub_debug redzone report.

The sibling commit "net/mlx5: LAG, allocate port-indexed scratch buffers
dynamically" makes the same assumption in lag_active_port_bits() and
mlx5_lag_print_mapping(). At the end of the series, mapping_show()
still uses kcalloc(ldev->ports) with the same fill loops.

Would it make sense to state the invariant, clamp the writes to
ldev->ports, or keep sizing the buffer by MLX5_MAX_PORTS?

> +	if (!ports)
> +		return -ENOMEM;
> +
>  	mutex_lock(&ldev->lock);
>  	lag_active = __mlx5_lag_is_active(ldev);
>  	if (lag_active) {

[ ... ]

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