Re: [PATCH net 3/3] net/mlx5e: TC, skip peer flow cleanup when LAG seq is unavailable

[Shay Drori <shayd@nvidia.com>]

On 18/06/2026 15:48, Simon Horman wrote:
> External email: Use caution opening links or attachments
> 
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> 
> Simon says: I think the first two issues flagged below warrant
>              consideration within the context of this patch.
> 
>              But the third and final issue flagged below as pre-existing
>              seems to be something to consider in the context of
>              possible follow-up.
> ---
> net/mlx5e: TC, skip peer flow cleanup when LAG seq is unavailable
> 
> This commit addresses a bad memory access crash by skipping peer flows when
> mlx5_lag_get_dev_seq() returns an error. This occurs when the peer isn't in
> the LAG or when no device is marked as master.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index a9001d1c902ff..c6e6534a5e23b 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -2155,6 +2155,9 @@ static void mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
>>        devcom = flow->priv->mdev->priv.eswitch->devcom;
>>        mlx5_devcom_for_each_peer_entry(devcom, peer_esw, pos) {
>>                i = mlx5_lag_get_dev_seq(peer_esw->dev);
>> +             if (i < 0)
>> +                     continue;
>> +
> 
> [Severity: Critical]
> Does skipping the remainder of this loop prevent the flow from being
> unlinked from the doubly-linked list?
> 
> By using continue here in mlx5e_tc_del_fdb_peers_flow(), we skip calling
> mlx5e_tc_del_fdb_peer_flow(), which means flow->peer[i] is never
> unlinked from the esw->offloads.peer_flows[original_i] list.

only in case a LAG member is removed from ldev, mlx5_lag_get_dev_seq()
will return error.
before LAG member is removed, esw->devcom is cleanup, which invoke
mlx5e_tc_clean_fdb_peer_flows(), which remove all peer flows.
Hence, no flow remains.

> 
> If the flow is subsequently freed via mlx5e_flow_put(), will this leave a
> freed list node resident in the active list and guarantee a use-after-free
> list corruption during the next list traversal?
> 
>>                mlx5e_tc_del_fdb_peer_flow(flow, i);
>>        }
>>   }
> 
> [ ... ]
> 
>> @@ -5526,6 +5529,9 @@ void mlx5e_tc_clean_fdb_peer_flows(struct mlx5_eswitch *esw)
>>
>>        mlx5_devcom_for_each_peer_entry(devcom, peer_esw, pos) {
>>                i = mlx5_lag_get_dev_seq(peer_esw->dev);
>> +             if (i < 0)
>> +                     continue;
>> +
> 
> [Severity: Critical]
> Similarly to the above, does this skip unlinking during flow cleanup when
> the LAG sequence is unavailable?

By the time get_dev_seq() can fail, the member is already out of ldev
- and clean_fdb_peer_flows() (devcom unpair) ran before that, while seq
was still valid, so the flows are already unlinked.
The guard just covers that later window; nothing remains to leak.


> 
>>                list_for_each_entry_safe(flow, tmp, &esw->offloads.peer_flows[i], peer[i])
>>                        mlx5e_tc_del_fdb_peers_flow(flow);
>>        }
> 
> 
> [Severity: Critical]
> This isn't a bug introduced by this patch, but is there a similar missing
> bounds check during flow addition in mlx5e_tc_add_fdb_peer_flow() in this
> same file?
> 
> If a flow is added concurrently with LAG teardown, mlx5_lag_get_dev_seq()
> can return -ENOENT (-2):
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:mlx5e_tc_add_fdb_peer_flow() {
>      ...
>      int i = mlx5_lag_get_dev_seq(peer_esw->dev);
>      ...
>      list_add_tail(&flow->peer[i], &esw->offloads.peer_flows[i]);
>      ...
> }
> 
> Does this result in an out-of-bounds write to a negative array index?


No, if LAG is destroyed, than is_peer_flow_needed will return false and
we won't enter here.
the whole peer loop runs under the devcom read lock
(mlx5_devcom_for_each_peer_begin), while devcom unpair - which is what
precedes LAG member removal and runs clean_fdb_peer_flows - takes the
write lock. The read lock therefore blocks teardown for the duration, so
mlx5_lag_get_dev_seq() can't go negative here.