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

On 21.08.26 23:37, Jerome Tollet wrote:
> With AF_XDP on a striding RQ, mlx5e defers releasing XSK buffers until
> an MPWQE is refilled. If XSK allocation then returns -ENOMEM,
> actual_wq_head is not advanced and a later NAPI poll retries the same
> WQE.
> 
> mlx5e_free_rx_mpwqe() currently leaves each released slot marked as
> releasable. On retry it can therefore call xsk_buff_free() again through
> stale pointers after the frames have returned to the XSK pool and been
> reallocated.
> 
> Mark each slot as released immediately after xsk_buff_free(). The
> successful allocation path already clears the bitmap after replacing
> all buffers, so retries become idempotent without changing the success
> path.
> 
> Fault injection forced three consecutive allocation failures for one
> selected MPWQE in AF_XDP zero-copy mode. Stock freed the same 16 XSK
> buffer pointers on all three attempts, for 48 frees. With this change,
> only the initial 16 frees occurred; retries observed a full bitmap, and
> a later successful allocation cleared it.
> 
> Fixes: 4c2a13236807 ("net/mlx5e: RX, Defer page release in striding rq for better recycling")
> Cc: [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 7bd0606a5..5fe17d295 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -609,9 +609,12 @@ mlx5e_free_rx_mpwqe(struct mlx5e_rq *rq, struct mlx5e_mpw_info *wi)
>  		 * is no way to return the page to userspace when the interface
>  		 * goes down.
>  		 */
> -		for (i = 0; i < rq->mpwqe.pages_per_wqe; i++)
> -			if (no_xdp_xmit || !test_bit(i, wi->skip_release_bitmap))
> +		for (i = 0; i < rq->mpwqe.pages_per_wqe; i++) {
> +			if (no_xdp_xmit || !test_bit(i, wi->skip_release_bitmap)) {
>  				xsk_buff_free(xsk_buffs[i]);
> +				__set_bit(i, wi->skip_release_bitmap);
> +			}
> +		}
>  	} else {
>  		for (i = 0; i < rq->mpwqe.pages_per_wqe; i++) {
>  			if (no_xdp_xmit || !test_bit(i, wi->skip_release_bitmap)) {
I'd change this to be consistent with mlx5e_alloc_rx_mpwqe(): set skip_release_bitmap
bits in error path of mlx5e_xsk_alloc_rx_mpwqe.

Thanks,
Dragos