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

[Shay Drori <shayd@nvidia.com>]

On 24/09/2026 20:45, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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().

good find.
will push as a separate patch.

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

In that case I think this is more of pre-existing bug which can be
handled in a separate patch.

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

Yes. mlx5_lag_check_prereq() will return false if the number of PFs in
ldev->pfs is different than ldev->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