Re: [PATCH net-next 04/13] net/mlx5: LAG, allocate port-indexed scratch buffers dynamically

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (1)

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

- [Medium] _mlx5_modify_lag()
  (drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c) can now leave a…
- [Medium] net/mlx5: heap buffer overflow in MLX5 LAG configuration
  arrays

Pre-existing issues:
- [Low] Concern: lag_active_port_bits() builds a bitmask of up to 8 bits
  (MLX5_MAX_PORTS = 8), but mlx5_ifc_lagc_bits.active_port is only 4…

--

net/mlx5: LAG, allocate port-indexed scratch buffers dynamically

This patch replaces the MLX5_MAX_PORTS-sized stack arrays in
lag_active_port_bits(), mlx5_lag_print_mapping() and
mlx5_infer_tx_affinity_mapping() with kcalloc(ldev->ports, ...)
allocations. lag_active_port_bits() and mlx5_infer_tx_affinity_mapping()
now return int, and their callers get -ENOMEM on allocation failure.

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

[ ... ]

> @@ -105,13 +111,21 @@ static int mlx5_cmd_create_lag(struct mlx5_core_dev *dev, struct mlx5_lag *ldev,
>  		MLX5_SET(lagc, lag_ctx, tx_remap_affinity_1, ports[idx0]);
>  		MLX5_SET(lagc, lag_ctx, tx_remap_affinity_2, ports[idx1]);
>  		break;
> -	case MLX5_LAG_PORT_SELECT_MODE_PORT_SELECT_FT:
> +	case MLX5_LAG_PORT_SELECT_MODE_PORT_SELECT_FT: {
> +		u8 active_port;
> +		int ret;
> +
>  		if (!MLX5_CAP_PORT_SELECTION(dev, port_select_flow_table_bypass))
>  			break;
>  
> -		MLX5_SET(lagc, lag_ctx, active_port,
> -			 lag_active_port_bits(ldev, tracker));
> +		ret = lag_active_port_bits(ldev, tracker);
> +		if (ret < 0)
> +			return ret;
> +
> +		active_port = ret;
> +		MLX5_SET(lagc, lag_ctx, active_port, active_port);

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch.
lag_active_port_bits() can build a mask up to 8 bits wide
(MLX5_MAX_PORTS is 8), but the lagc active_port field is only 4 bits wide:

include/linux/mlx5/mlx5_ifc.h:mlx5_ifc_lagc_bits {
	...
	u8         reserved_at_20[0xc];
	u8         active_port[0x4];
	...
}

MLX5_SET() masks the value to the field width. Bits 4..7 would be
silently dropped here, and also in mlx5_cmd_modify_active_port().

In hash LAG with port_select_flow_table_bypass, would ports 5..8 ever be
reported to FW as active? It's not clear whether FW offers bypass on
devices with more than 4 LAG ports.

>  		break;
> +	}
>  	default:
>  		break;
>  	}

[ ... ]

> @@ -816,7 +857,10 @@ static int _mlx5_modify_lag(struct mlx5_lag *ldev,
>  		    !MLX5_CAP_PORT_SELECTION(dev0, port_select_flow_table_bypass))
>  			return ret;
>  
> -		active_ports = lag_active_port_bits(ldev, tracker);
> +		ret = lag_active_port_bits(ldev, tracker);
> +		if (ret < 0)
> +			return ret;

[Severity: Medium]
Can a failed kcalloc() in lag_active_port_bits() leave a hash-based LAG
only partly reprogrammed?

By the time lag_active_port_bits() runs, mlx5_lag_port_sel_modify() has
already rewritten the FT rule destinations in hardware:

_mlx5_modify_lag()
  mlx5_lag_port_sel_modify(ldev, ports)
    mlx5_lag_modify_definers_destinations()
      __mlx5_lag_modify_definers_destinations()
        mlx5_modify_rule_destination()
  lag_active_port_bits()
    kcalloc() fails -> return -ENOMEM

If that happens, mlx5_cmd_modify_active_port() is never sent. The FW
active_port bitmask used by bypass traffic still lists the old active
uplinks, which could include a port whose link just went down.

mlx5_modify_lag() then takes the error path and skips the v2p_map update:

	err = _mlx5_modify_lag(ldev, tracker, ports);
	if (err) {
		...
		goto out;
	}
	memcpy(ldev->v2p_map, ports,
	       ldev->ports * MLX5_LAG_MAX_HASH_BUCKETS);

Both mlx5_modify_lag() and __mlx5_lag_modify_definers_destinations()
skip entries where ports[idx] == v2p_map[idx]. Suppose the tracker later
returns to the state that matches the stale v2p_map. Would the FT rules
then stay on the intermediate mapping, with nothing to reprogram them?

A FW failure in mlx5_cmd_modify_active_port() could already cause this
before the patch. This patch adds a software failure point between the
two hardware steps.

The bitmask doesn't seem to need a scratch buffer. Could it be built
directly while walking mlx5_ldev_for_each()? Alternatively, could it be
computed before mlx5_lag_port_sel_modify() touches the hardware?

> +		active_ports = ret;
>  
>  		return mlx5_cmd_modify_active_port(dev0, active_ports);
>  	}

[ ... ]

> @@ -668,20 +696,28 @@ static bool __mlx5_lag_is_sd_active(struct mlx5_lag *ldev,
>   * If there are ports that are disabled fill the relevant slots
>   * with mapping that points to active ports.
>   */
> -static void mlx5_infer_tx_affinity_mapping(struct lag_tracker *tracker,
> -					   struct mlx5_lag *ldev,
> -					   u8 buckets,
> -					   u8 *ports)
> +static int mlx5_infer_tx_affinity_mapping(struct lag_tracker *tracker,
> +					  struct mlx5_lag *ldev,
> +					  u8 buckets,
> +					  u8 *ports)
>  {
> -	int disabled[MLX5_MAX_PORTS] = {};
> -	int enabled[MLX5_MAX_PORTS] = {};
>  	int disabled_ports_num = 0;
>  	int enabled_ports_num = 0;
> +	int *disabled;
> +	int *enabled;
> +	int err = 0;
>  	int idx;
>  	u32 rand;
>  	int i;
>  	int j;
>  
> +	enabled = kcalloc(ldev->ports, sizeof(*enabled), GFP_KERNEL);
> +	disabled = kcalloc(ldev->ports, sizeof(*disabled), GFP_KERNEL);
> +	if (!enabled || !disabled) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
>  	mlx5_ldev_for_each(i, 0, ldev) {
>  		if (tracker->netdev_state[i].tx_enabled &&
>  		    tracker->netdev_state[i].link_up)

[Severity: Medium]
The old stack arrays were sized by MLX5_MAX_PORTS, which is also the
limit on how many entries ldev->pfs can hold. The new buffers are sized
by ldev->ports instead. However, the loops that fill them still walk
the xarray with mlx5_ldev_for_each() and index by a running count:

mlx5_infer_tx_affinity_mapping()
  mlx5_ldev_for_each(i, 0, ldev)
    enabled[enabled_ports_num++] = i;
    or disabled[disabled_ports_num++] = i;

mlx5_infer_tx_enabled() / mlx5_infer_tx_disabled()
  ports[(*num_enabled)++] = i;

None of these helpers checks the count against ldev->ports.

Is there anything that guarantees the number of PFs added to ldev->pfs
never exceeds ldev->ports (num_lag_ports)? Suppose devcom pairs more
functions sharing the same system image GUID than FW reported in
num_lag_ports. Then these buffers would be written past the end of
their kcalloc() allocations:

  enabled[] and disabled[] in mlx5_infer_tx_affinity_mapping()
  enabled_ports in lag_active_port_bits()
  enabled_ports in mlx5_lag_print_mapping()

Before this patch the same situation stayed inside the MLX5_MAX_PORTS
stack arrays.

The same applies to buf in mlx5_lag_print_mapping(). It is now sized
ldev->ports * 10 + 1, but the non-hash branch emits one " port %d:%d"
per iterated port and bucket, so an extra PF would push the
scnprintf() writes past the allocation.

If the invariant holds, could it be enforced where a PF is added to
ldev->pfs? Alternatively, could these loops bound the count by
ldev->ports, so an unexpected extra PF can't turn into a heap
overwrite?

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