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

Hi,

Thanks for fixing this! Just some commit message nits.

On 19.08.26 17:13, Jerome Tollet wrote:
> When an XDP redirect to an AF_XDP socket fails because the RX ring is
> full, the XSK core frees the buffer. mlx5e later visits the cyclic WQE
> and frees its XSK buffer before trying to refill the slot.
> 
Is this necessary? You explain very well the circumstances in the last
paragraph.

> If a batched refill succeeds only partially, a missing WQE keeps its
> old buffer pointer. The buffer may meanwhile be allocated to another
> WQE, so a later refill retry can free a live buffer through the stale
> pointer and publish the same UMEM frame twice.
>
This should be the first paragraph. With a bit of extra context added
(XDP redirect with AF_XDP).

> Mark the WQE as released immediately after the driver-side free. The
> flag is already cleared when a replacement buffer is assigned, so
> refill retries no longer release stale pointers.
> 
> A standalone legacy cyclic-RQ zero-copy libxsk reproducer, using
> 64-byte UDP traffic offered at 12 Mpps, stopped on stock after
> 2,854,914 packets in 4.094 seconds, with 4,542 xdp_rx_ring_full events
> and 64 ownership/double-publication errors. With this change it
> processed 356,904,225 packets in 30 seconds despite 571,405
> xdp_rx_ring_full events, with no ownership or data errors.
>
There's no splat, right?

> Fixes: 3f93f82988bc ("net/mlx5e: RX, Defer page release in legacy rq for better recycling")
> Cc: [email protected]
> Suggested-by: Daniel Borkmann <[email protected]>
> Signed-off-by: Jerome Tollet <[email protected]>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 6fc6605d2..7967a2737 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);
>  }
>

When you send the v2, feel free to add:

Reviewed-by: Dragos Tatulea <[email protected]>

Thanks,
Dragos