Re: [PATCH net v3] xsk: fix NULL pointer dereference in __xsk_rcv()

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260806204757.47817-1-blbllhy@gmail.com?part=1
---
--- Patch [1]: [PATCH net v3] xsk: fix NULL pointer dereference in __xsk_rcv() ---
commit ee824acf57213f0a5f7afb287ff22c8b8eefdc3c
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

xsk: fix NULL pointer dereference in __xsk_rcv()

The patch converts the multi-buffer copy path in __xsk_rcv() into a
two-stage transaction: all buffers for a packet are allocated and staged
on the pool's xskb_list first, and only once that succeeds are the data
copied and the RX descriptors reserved. On an allocation failure the
staged buffers are recycled with xsk_buff_free().

Link: https://lore.kernel.org/netdev/20260724164719.99563-1-blbllhy@gmail.com

A couple of questions below, starting with the changelog.

The commit message ends at:

  "Only after the allocation stage succeeds, copy the data, reserve the
   RX descriptors, and release the buffers in an error-free loop using
   xsk_buff_get_frag()."

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
Should the message also describe the two new drop conditions the patch
adds?

The first is the new WARN_ON_ONCE(!list_empty(&xs->pool->xskb_list)),
which drops the packet and bumps rx_dropped based on pool-wide state that
is also written by xsk_buff_add_frag() and drained by xsk_rcv_zc().

The second is the new handling of a Fill Ring that hands out the same
chunk twice ("xsk_xdp == first || !list_empty(&xskb->list_node)"), which
is entirely under userspace control. Before this patch, an aligned-mode
Fill Ring containing a duplicate address made __xp_alloc() return
&pool->heads[xp_aligned_extract_idx(addr)] twice and the old loop emitted
two RX descriptors both pointing at that chunk; now the packet is dropped
and the Fill Ring entries are consumed with no descriptor produced.

Both are explained in the v3 changelog below the "---" separator, which is
dropped when the patch is applied, so neither git history nor a stable
backporter (the patch carries a Fixes: tag) sees them.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 7855ee09c4b64..02ff3ad97410f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -343,11 +344,34 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  		frag =  &sinfo->frags[0];
>  	}
>  
> +	if (WARN_ON_ONCE(!list_empty(&xs->pool->xskb_list)))
> +		goto err_alloc;

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
Can this condition ever clear itself once it fires? The err_alloc label
only does:

	err_alloc:
		xs->rx_dropped++;
		return -ENOMEM;

so pool->xskb_list is neither drained nor repaired. pool->xskb_list is a
field of the long-lived struct xsk_buff_pool, initialized once by
INIT_LIST_HEAD(&pool->xskb_list) in xp_create_and_assign_umem(), and it is
never reset per packet.

If a stale entry is ever left on that list, doesn't every subsequent
multi-buffer copy-mode packet for every socket sharing the pool take this
branch and get dropped, with the stranded buffers never returned to
pool->free_list? Since WARN_ON_ONCE only prints once, the drops after that
are silent.

Would draining the list here, or keeping the staging list call-local, be
preferable to a bailout for a condition that persists?

> +
> +	first = xsk_buff_alloc(xs->pool);
> +	if (!first)
> +		goto err_alloc;
> +
> +	xdp_buff_set_frags_flag(first);
> +	for (i = 1; i < num_desc; i++) {
> +		xsk_xdp = xsk_buff_alloc(xs->pool);
> +		if (!xsk_xdp)
> +			goto err_free;
> +
> +		xskb = container_of(xsk_xdp, struct xdp_buff_xsk, xdp);
> +		if (unlikely(xsk_xdp == first ||
> +			     !list_empty(&xskb->list_node)))
> +			goto err_free;
> +
> +		list_add_tail(&xskb->list_node, &xs->pool->xskb_list);
> +	}

[Severity: High]
[Finding: main-5-7]
[Sources: opus-5]
What serializes access to xs->pool->xskb_list here? The two callers of
__xsk_rcv() do not share a lock:

net/xdp/xsk.c:xsk_generic_rcv() {
	err = xsk_rcv_check(xs, xdp, len);
	if (!err) {
		spin_lock_bh(&xs->pool->rx_lock);
		err = __xsk_rcv(xs, xdp, len);
		...
}

net/xdp/xsk.c:xsk_rcv() {
	...
	err = __xsk_rcv(xs, xdp, len);
	if (!err)
		xdp_return_buff(xdp);
	...
}

xsk_rcv() (reached from __xsk_map_redirect()) holds no pool lock, so the
per-packet staging state now lives in pool-global list state with an empty
lockset. Previously each buffer was a local variable in this function.

A concrete unlocked, non-NAPI caller is cpumap:

kernel/bpf/cpumap.c:cpu_map_bpf_prog_run_xdp() {
	struct xdp_rxq_info rxq = {};
	...
		rxq.dev = xdpf->dev_rx;
		rxq.mem.type = xdpf->mem_type;
		/* TODO: report queue_index to xdp_rxq_info */
	...
			err = xdp_do_redirect(xdpf->dev_rx, &xdp, rcpu->prog);
}

queue_index defaults to 0 and mem.type is page-based, so an XDP_REDIRECT
into an xskmap from that program passes xsk_dev_queue_valid(), takes the
copy path, and runs from a cpumap kthread that can execute concurrently
with the NIC NAPI, another cpumap kthread, or a generic-XDP
xsk_generic_rcv() on another CPU that only holds rx_lock.

Can that lead to concurrent list_add_tail() on the same list_head, i.e.
list corruption? And can xsk_buff_get_frag() in the copy loop pop a buffer
staged by the other context, so the same umem chunk ends up published in
two different RX descriptors?

Note also that the helpers touching this list in
include/net/xdp_sock_drv.h have no internal locking:

include/net/xdp_sock_drv.h:xsk_buff_get_frag() {
	frag = list_first_entry_or_null(&xskb->pool->xskb_list,
					struct xdp_buff_xsk, list_node);
	if (frag) {
		list_del_init(&frag->list_node);
		ret = &frag->xdp;
	}
}

They were previously reached only from the NAPI/zero-copy driver context.

> +
> +	xdp_buff_clear_frags_flag(first);
> +	xsk_xdp = first;
>  	do {
>  		u32 to_len = frame_size + meta_len;
>  		u32 copied;
>  
> -		xsk_xdp = xsk_buff_alloc(xs->pool);
> +		next = xsk_buff_get_frag(xsk_xdp);

[Severity: Low]
[Finding: main-2-3]
[Sources: opus-5]
This isn't a bug today, but the producer/consumer protocol for
pool->xskb_list is now implemented two different ways in-tree.

The canonical producer is:

include/net/xdp_sock_drv.h:xsk_buff_add_frag() {
	if (!__xdp_buff_add_frag(head, virt_to_netmem(data),
				 offset_in_page(data), xdp->data_end - data,
				 xdp->frame_sz, false))
		return false;

	frag = container_of(xdp, struct xdp_buff_xsk, xdp);
	list_add_tail(&frag->list_node, &frag->pool->xskb_list);
}

__xdp_buff_add_frag() sets the head's XDP_FLAGS_HAS_FRAGS and updates the
head's skb_shared_info nr_frags/len. The new code open-codes the producer
side with a bare list_add_tail() plus xdp_buff_set_frags_flag(first) and no
skb_shared_info initialization, while using the official consumer
xsk_buff_get_frag(). That yields a frags-flagged xdp_buff whose shared info
at xdp_data_hard_end() is uninitialized. Today nothing reads it
(xsk_buff_free() only writes nr_frags = 0), but would it be safer to reuse
xsk_buff_add_frag() here?

Related: xsk_buff_get_frag() is declared as

	static inline struct xdp_buff *xsk_buff_get_frag(const struct xdp_buff *first)

and every other caller passes the head buffer, whereas this loop passes the
fragment returned by the previous iteration. That only works because the
body uses the argument solely to reach xskb->pool.

>  		copy_to = xsk_xdp->data - meta_len;
>  
>  		copied = xsk_copy_xdp(copy_to, &copy_from, to_len, &from_len, &frag, rem);
> @@ -356,10 +380,21 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  		xskb = container_of(xsk_xdp, struct xdp_buff_xsk, xdp);
>  		__xsk_rcv_zc_safe(xs, xskb, copied - meta_len,
>  				  rem ? XDP_PKT_CONTD : 0);
> +		xsk_xdp = next;
>  		meta_len = 0;
>  	} while (rem);
>  
>  	return 0;
> +
> +err_free:
> +	/* Re-set frags flag; xsk_buff_alloc() may have cleared first->flags
> +	 * if the same Fill Ring address aliased back to first.
> +	 */
> +	xdp_buff_set_frags_flag(first);
> +	xsk_buff_free(first);
> +err_alloc:
> +	xs->rx_dropped++;
> +	return -ENOMEM;
>  }

Following on from the locking question above: with the frags flag set,
xsk_buff_free() walks the whole pool list:

include/net/xdp_sock_drv.h:xsk_buff_free() {
	list_for_each_entry_safe(pos, tmp, xskb_list, list_node) {
		list_del_init(&pos->list_node);
		xp_free(pos);
	}
	...
}

so if another context has entries staged on pool->xskb_list at that moment,
does this error path xp_free() buffers that are still in flight there,
skewing pool->free_list/free_heads accounting?