Re: [PATCH net v2] net/mlx5e: Prevent stale XSK buffer release on refill retry

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

Pre-existing issues:
- [High] Incomplete Fix: Double Free in MPWQE path on XSK ring full
--

--- Patch [1]: [PATCH net v2] net/mlx5e: Prevent stale XSK buffer release on refill retry ---
commit c24da150351198b044b52cb55d5ba341b752ae81
Author: Jerome Tollet <[email protected]>

net/mlx5e: Prevent stale XSK buffer release on refill retry
    
When an XDP redirect to an AF_XDP socket fails because its RX ring is
full, the XSK core frees the buffer. During the subsequent batched refill
of a legacy cyclic RQ, mlx5e also releases the WQE's XSK buffer before
allocating a replacement. If that refill succeeds only partially, a WQE
left without a replacement retains its old buffer pointer.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 6fc6605d20549..7967a27372981 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -410,8 +410,11 @@ static inline void mlx5e_free_rx_wqe(struct mlx5e_rq *rq,
>  
>  static void mlx5e_xsk_free_rx_wqe(struct mlx5e_wqe_frag_info *wi)
>  {
> -	if (!(wi->flags & BIT(MLX5E_WQE_FRAG_SKIP_RELEASE)))
> -		xsk_buff_free(*wi->xskp);
> +	if (wi->flags & BIT(MLX5E_WQE_FRAG_SKIP_RELEASE))
> +		return;
> +
> +	xsk_buff_free(*wi->xskp);
> +	wi->flags |= BIT(MLX5E_WQE_FRAG_SKIP_RELEASE);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does a similar double-free 
vulnerability exist in the MPWQE path?

It looks like the same sequence of events can happen for multi-packet WQEs
when the XSK RX ring is full. In mlx5e_post_rx_mpwqes() (in en_rx.c):

mlx5e_post_rx_mpwqes() {
        ...
        mlx5e_free_rx_mpwqe();
        ...
        alloc_err = mlx5e_xsk_alloc_rx_mpwqe();
        if (unlikely(alloc_err)) {
                ...
                break;
        }
}

If mlx5e_xsk_alloc_rx_mpwqe() fails (e.g., returning -ENOMEM), it returns
without filling the skip_release_bitmap with 1s. This differs from the
standard mlx5e_alloc_rx_mpwqe() path which correctly calls bitmap_fill()
on error.

When the allocation fails, the loop breaks early and actual_wq_head is not
advanced.

On the next NAPI poll, won't mlx5e_post_rx_mpwqes() process this same WQE
head again? Since skip_release_bitmap is still 0, mlx5e_free_rx_mpwqe()
will execute xsk_buff_free() on the same pointers, causing a double-free.

Should mlx5e_xsk_alloc_rx_mpwqe() also fill the skip_release_bitmap on
error, or should mlx5e_free_rx_mpwqe() update the bitmap immediately after
freeing the buffers, similar to your fix here for the legacy cyclic RQ?

>  
>  static void mlx5e_dealloc_rx_wqe(struct mlx5e_rq *rq, u16 ix)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1