Re: [PATCH bpf v2] xsk: Do not free ZC Rx buffer when the Rx ring is full

[bot+bpf-ci]

> xsk: Do not free ZC Rx buffer when the Rx ring is full

TL;DR: Commit message only: the code change looks correct, but the "freed
twice" claim seems inaccurate for drivers that free immediately (i40e, ice,
etc.); only deferred-release drivers like mlx5e are affected.

>     xsk: Do not free ZC Rx buffer when the Rx ring is full
>
>     When an XDP program redirects a zero-copy buffer to an XSK and the
>     socket's Rx ring is full, xsk_rcv_zc() frees the buffer and returns
>     -ENOBUFS to xdp_do_redirect().
>
>     A failed xdp_do_redirect() leaves the buffer owned by the caller, and
>     most drivers rely on that: mlx5e treats the failure as XDP_DROP and
>     releases the buffer when the Rx WQE is refilled, while i40e, ice,
>     ixgbe, igb, igc, stmmac, idpf and others free it themselves. The
>     buffer is therefore freed twice. It gets reused for another Rx
>     descriptor, the second free puts an in-use buffer back on the pool
>     free list, and the same UMEM frame is later handed to user space
>     twice. ixgbe and igb keep the buffer in the ring on their need_wakeup
>     -ENOBUFS (XDP_EXIT) path and run it through XDP again on the next poll,
>     after the core has freed it.
>
>     Leave the buffer with the caller on both the single-buffer and the
>     multi-buffer -ENOBUFS paths, as xsk_rcv() and xsk_rcv_check() already
>     do. A few drivers relied on the core freeing the buffer, so make them
>     free it:
>
>     - i40e and ice consume the descriptor on XDP_EXIT without freeing it.
>     - gve DQO counts the redirect error without freeing the buffer.
>     - octeontx2 returns false on a failed redirect, so the caller goes on
>       to build an skb from the already freed buffer. Free the buffer and
>       report it as consumed instead.
>
>     Seen on Mellanox ConnectX-6 DX MT2892 (FW 22.47.1026) with AF_XDP
>     zero-copy on 7.3.0-rc6 when the user-space consumer could not keep up
>     with the Rx ring. With this change no duplicate frames were seen with
>     striding RQ on or off.
>
>     Fixes: 269009893146 ("xsk: recycle buffer in case Rx queue was full")
>     Signed-off-by: Marcin Knap <marcin.knap@xxxxxxxxxx>

The changelog says that for i40e, ice, ixgbe, igb, igc, stmmac, idpf and the
others the buffer is "freed twice" and that "the second free puts an in-use
buffer back on the pool free list".  Is this accurate for those drivers?

All of them call xsk_buff_free() on the redirect failure path straight after
xdp_do_redirect() returns, before any xp_alloc() can run.  xp_free() starts
with:

    void xp_free(struct xdp_buff_xsk *xskb)
    {
        if (!list_empty(&xskb->list_node))
            return;
        ...

and the xsk_buff_free() from the old xsk_rcv_zc() err label has already put
list_node on pool->free_list.  So wouldn't the second free simply return
early here?

For a multi-buffer frame, xskb_list is already emptied by then, and the head
hits the same early return.

The corruption described seems to happen only where the second release comes
later, after the buffer may have been reallocated:

- mlx5e releases the buffer on WQE refill.
- ixgbe and igb keep the buffer in the ring on XDP_EXIT and run XDP on it
  again.
- dpaa2 calls dpaa2_eth_recycle_buf(), which hands the address back to the
  hardware pool while the buffer is also on free_list.
- octeontx2 builds an skb from the freed buffer.

The code change itself looks correct.  Could the changelog be reworded to
name the drivers with a deferred second release, or the reuse after free, as
the ones affected, instead of the drivers that free the buffer immediately?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41025962641