Re: [PATCH net-next 08/13] net/mlx5e: TC, track peer flows in a vhca_id xarray

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/mlx5e: NULL pointer dereference in TC flow error cleanup
- [High] net/mlx5e: UAF of dup_peer_flows due to race with unpair
--

commit 92fc8821722bceca321f4825f3199f7c0d234a70
Author: Shay Drory <shayd@nvidia.com>

net/mlx5e: TC, track peer flows in a vhca_id xarray

The commit replaces a fixed-size esw->offloads.peer_flows[] array with an
xarray keyed by vhca_id to lift the MLX5_MAX_PORTS cap. A dup_peer_flows
list_head is created when eswitches pair over devcom and freed when they
unpair.

> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -4634,8 +4635,11 @@ static int mlx5e_tc_add_fdb_peer_flow(struct flow_cls_offload *f,
>  	peer_flow->peer_orig = flow;
>  	list_add_tail(&peer_flow->peer_flows, &flow->peer_flows);
>  	flow_flag_set(flow, DUP);
> +	dup_peer_flows = xa_load(&esw->offloads.peer_flows, peer_vhca_id);
> +	if (!dup_peer_flows)
> +		return -ENODEV;

[Severity: High]
Can this early return lead to a NULL pointer dereference during error
unwinding? 

If xa_load() returns NULL, we return -ENODEV, leaving peer_flow->peer
completely uninitialized.

When the caller unwinds and calls mlx5e_tc_del_fdb_peer_flow(peer_flow),
it executes list_del(&peer_flow->peer) which will access NULL->next and
NULL->prev, causing a kernel panic.

>  	mutex_lock(&esw->offloads.peer_mutex);
> -	list_add_tail(&peer_flow->peer, &esw->offloads.peer_flows[i]);
> +	list_add_tail(&peer_flow->peer, dup_peer_flows);
>  	mutex_unlock(&esw->offloads.peer_mutex);
>  
>  out:

[ ... ]

> @@ -5530,20 +5534,14 @@ int mlx5e_tc_num_filters(struct mlx5e_priv *priv, unsigned long flags)
>  
>  void mlx5e_tc_clean_fdb_peer_flows(struct mlx5_eswitch *esw)
>  {
[ ... ]
> +	xa_for_each(&esw->offloads.peer_flows, index, dup_peer_flows)
>  		list_for_each_entry_safe(peer_flow, tmp_peer_flow,
> -					 &esw->offloads.peer_flows[i], peer)
> +					 dup_peer_flows, peer)
>  			mlx5e_tc_del_fdb_peer_flow(peer_flow);

[Severity: Low]
This isn't a bug introduced by this patch, but does flushing the entire xarray
here wipe flows for all active peers when only a single peer unpairs?

When a devcom unpair event happens for a single peer, iterating over the
entire esw->offloads.peer_flows xarray unconditionally drops TC flows
duplicated to all active peers, rather than just the one disconnecting.
This behavior existed prior to the patch, but the patch carries it forward
without resolving the multi-peer limitation it claimed to lift.

> -	}
>  }

[ ... ]

> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -3383,10 +3383,15 @@ static void mlx5_esw_offloads_rep_event_unpair(struct mlx5_eswitch *esw,
>  static void mlx5_esw_offloads_unpair(struct mlx5_eswitch *esw,
>  				     struct mlx5_eswitch *peer_esw)
>  {
> +	struct list_head *dup_peer_flows;
> +
>  #if IS_ENABLED(CONFIG_MLX5_CLS_ACT)
>  	mlx5e_tc_clean_fdb_peer_flows(esw);
>  #endif
>  	mlx5_esw_offloads_rep_event_unpair(esw, peer_esw);
> +	dup_peer_flows = xa_erase(&esw->offloads.peer_flows,
> +				  MLX5_CAP_GEN(peer_esw->dev, vhca_id));
> +	kfree(dup_peer_flows);

[Severity: High]
Does this kfree() race with concurrent TC flow additions, leading to a
use-after-free?

In mlx5e_tc_add_fdb_peer_flow(), dup_peer_flows is retrieved locklessly
via xa_load(), and then usage is delayed behind acquiring peer_mutex:

mlx5e_tc_add_fdb_peer_flow() {
    dup_peer_flows = xa_load(&esw->offloads.peer_flows, peer_vhca_id);
    ...
    mutex_lock(&esw->offloads.peer_mutex);
    list_add_tail(&peer_flow->peer, dup_peer_flows);
}

Meanwhile, this unpair path does not hold peer_mutex before removing the
entry via xa_erase() and freeing it. If unpair runs concurrently with a
filter addition, can the addition thread wake up and call list_add_tail()
on the already freed dup_peer_flows list head?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923103830.1183-1-tariqt@nvidia.com?part=8