Re: [PATCH net-next v1 07/11] xsk: Deliver provider chunks without copying

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch weakens a documented AF_XDP UAPI guarantee, and
  userspace cannot opt in or detect it.
- [Low] The commit message gets wrong what the old copy path did.
- [Low] The patch adds a new core page_pool memory-provider API,
  net_mp_release_page_pool_bulk(), in net/core/page_pool.c and…
- [Low] The new drop paths for provider sockets in xsk_rcv() do a plain
  non-atomic `xs->rx_dropped++` on a u64 from contexts outside the queue…

--

xsk: Deliver provider chunks without copying

This commit puts packets whose buffers all belong to the socket's memory
provider directly on the AF_XDP RX ring, and copies other packets into
chunks claimed from the provider. Generic XDP and CPUMAP redirects to
provider sockets are dropped. TX descriptor validation now reserves
tx_metadata_len only for descriptors with XDP_TX_METADATA.

A provider socket still receives through the copy path, which takes
FILL entries without the provider's ownership checks: a chunk that a
page pool holds could be handed out twice. No driver opts in yet.

[Severity: Low]
Does this describe what the old copy path actually does for a provider
socket?

Before this patch, xsk_rcv() calls __xsk_rcv() for provider sockets,
because their mem.type is MEM_TYPE_PAGE_POOL. That leads to:

xsk_rcv()->__xsk_rcv()->xsk_buff_alloc()->xp_alloc()->__xp_alloc()
    ->xp_get_xskb()

For a provider bind, pool->free_heads_cnt is umem->chunks and pool->fq is
still set. So __xp_alloc() peeks a FILL entry, and in aligned mode (the
only mode xsk_pp_bind() accepts) xp_get_xskb() returns &pool->heads[idx].

pool->heads is never allocated here. xp_assign_dev() returns right after
a successful xsk_pp_bind() and never calls xp_alloc_heads():

net/xdp/xsk_buff_pool.c:xp_assign_dev() {
    ...
	if (xsk_pp_supported(netdev)) {
		err = xsk_pp_bind(pool, netdev, queue_id);
		if (err)
			goto err_unreg_pool;

		pool->umem->zc = true;
		return 0;
	}
    ...
}

xp_alloc() then writes xskb->xdp.data through a NULL-based pointer.

So the failure would be a NULL or wild pointer dereference in NAPI, not a
chunk handed out twice. No driver opts in yet, so there is no runtime
impact. Could the commit message describe the actual failure mode?

[ ... ]

[Severity: Low]
This isn't a bug, but this adds a new core page_pool memory provider API,
declared in include/net/page_pool/memory_provider.h. The xsk commit
message doesn't mention it.

The helper clears pp_info and advances pool->pages_state_release_cnt.
That counter feeds the inflight accounting that decides when a destroyed
pool is freed. The helper also documents that @pool may be freed on
return.

Other core changes in this series, such as "page_pool: Coordinate refill
with memory providers", got their own patches. Could this go in a
separate page_pool patch, so the page_pool maintainers can review the
new contract?

[ ... ]

[ ... ]

[Severity: Low]
Can these xs->rx_dropped++ updates race with the queue NAPI?

The CPUMAP kthread reaches the unregistered rxq branch.
cpu_map_bpf_prog_run_xdp() uses an on-stack rxq:

kernel/bpf/cpumap.c:cpu_map_bpf_prog_run_xdp() {
    ...
	struct xdp_rxq_info rxq = {};
    ...
}

It sets rxq.dev = xdpf->dev_rx and leaves queue_index at 0, so
xsk_rcv_check() passes for a socket bound to queue 0.

A BPF_F_TEST_XDP_LIVE_FRAMES test run reaches the xsk_pp_queue_buf()
branch. xdp_test_run_setup() registers a fake rxq on the real device with
queue_index 0 and page-backed MEM_TYPE_PAGE_POOL buffers. xsk_pp_rcv_zc()
returns -EXDEV, then xsk_pp_queue_buf() returns false, all in process
context.

At the same time, the queue NAPI on another CPU can increment
xs->rx_dropped and xs->rx_queue_full in xsk_pp_rcv_copy(),
xsk_pp_rcv_zc() and xsk_rcv_check(). No lock is shared between these
contexts.

Would concurrent increments get lost, and could the u64 counter tear on
32-bit, so that XDP_STATISTICS reports wrong drop counts? xsk_rcv_check()
already has the same unsynchronized pattern, and the effect is limited to
these counters.

[ ... ]

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
Does this weaken the documented AF_XDP headroom guarantee for XDP_USE_SG
applications, with no way to opt in or detect it?

af_xdp.rst says a UMEM headroom of N means the packet "will start N bytes
into the buffer leaving the first N bytes for the application to use".
Before this patch, every RX buffer kept this headroom:

- copy mode, in __xsk_rcv()
- driver zero copy, in xp_alloc()
- the new xsk_pp_rcv_copy() path, which uses addrs[i] + headroom

Here the frag descriptors are published at skb_frag_address(frag) - umem,
wherever the device placed the payload. On fbnic that can be chunk
offset 0. The device has then DMA-written into the first N bytes of
those chunks. Any per-chunk application data kept in the headroom is
overwritten.

xp_assign_dev() picks xsk_pp_bind() automatically whenever
xsk_pp_supported(netdev) is true. That covers both XDP_ZEROCOPY and a
bind without flags. No bind or UMEM flag lets an application accept or
refuse the weaker rule. The af_xdp.rst hunk changes the documented rule
for all drivers:

  In a multi-buffer packet, only the first buffer is guaranteed this
  headroom; the others may start at offset 0 of their chunk.

At the end of the series only fbnic opts in, and fbnic had no AF_XDP RX
before, so no existing deployment changes today. An application that
relies on per-buffer headroom could still lose it, with no change on its
side, if another driver later moves to xsk_page_pool.

Should this be gated by a flag? Or should the commit message describe it
as a UAPI change rather than as a device property?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org