Re: [PATCH v6 bpf 1/3] xdp: introduce xdp_convert_skb_to_buff()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xdp: shared xdp_rxq.mem.type mutation in xdp_convert_skb_to_buff()
--

commit cc30acfe8407351451e1a0085dbc31d3b426757f
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xdp: introduce xdp_convert_skb_to_buff()

This commit introduces the xdp_convert_skb_to_buff() helper to initialize an
xdp_buff from an sk_buff for generic XDP and sets xdp->rxq->mem.type to
MEM_TYPE_PAGE_POOL so page_pool-backed fragments are properly released when
shrinking tail data.

> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 07231adfb5f82..28bd78dbef207 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -389,6 +389,42 @@ struct sk_buff *xdp_build_skb_from_frame(struct xdp_frame *xdpf,
>  					 struct net_device *dev);
>  struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf);
>  
> +/**
> + * xdp_convert_skb_to_buff - build an xdp_buff on top of an skb
> + * @skb: skb to describe, packet data is taken from its MAC header onwards
> + * @xdp: xdp_buff to initialize
> + * @xdp_rxq: Rx queue info to attach to @xdp
> + *
> + * The memory type set on @xdp_rxq is only used to release fragments that
> + * the XDP program consumes; the head can not be freed from under the skb.
> + * A non-linear @skb is expected to have gone through skb_pp_cow_data(), so
> + * all of its fragments are page_pool backed.
> + */
> +static inline
> +void xdp_convert_skb_to_buff(struct sk_buff *skb, struct xdp_buff *xdp,
> +			     struct xdp_rxq_info *xdp_rxq)
> +{
> +	u32 frame_sz, pkt_len;
> +
> +	/* SKB "head" area always have tailroom for skb_shared_info */
> +	frame_sz = skb_end_pointer(skb) - skb->head;
> +	frame_sz += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	pkt_len = skb_tail_pointer(skb) - skb_mac_header(skb);
> +
> +	xdp_init_buff(xdp, frame_sz, xdp_rxq);
> +	xdp_prepare_buff(xdp, skb->head, skb->mac_header, pkt_len, true);
> +
> +	if (skb_is_nonlinear(skb)) {
> +		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
> +		xdp_buff_set_frags_flag(xdp);
> +	} else {
> +		xdp_buff_clear_frags_flag(xdp);
> +	}
> +
> +	DEBUG_NET_WARN_ON_ONCE(skb_is_nonlinear(skb) && !skb->pp_recycle);
> +	xdp->rxq->mem.type = MEM_TYPE_PAGE_POOL;
> +}

[Severity: High]
Does mutating xdp->rxq->mem.type in place on the shared netdev_rx_queue::xdp_rxq
cause a regression for bpf_prog_test_run_xdp()?

In net/core/dev.c, bpf_prog_run_generic_xdp() looks up the persistent device
queue via netif_get_rxqueue(skb) and passes &rxqueue->xdp_rxq to
xdp_convert_skb_to_buff(), which permanently sets rxqueue->xdp_rxq.mem.type to
MEM_TYPE_PAGE_POOL.

In net/bpf/test_run.c, bpf_prog_test_run_xdp() (and xdp_convert_md_to_buff()
when ingress_ifindex is specified) attaches the device's &rxqueue->xdp_rxq
without resetting mem.type:

net/bpf/test_run.c:bpf_prog_test_run_xdp() {
	...
	rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
	rxqueue->xdp_rxq.frag_size = PAGE_SIZE;
	xdp_init_buff(&xdp, rxqueue->xdp_rxq.frag_size, &rxqueue->xdp_rxq);
	...
			page = alloc_page(GFP_KERNEL);
			if (!page) {
				ret = -ENOMEM;
				goto out_put_dev;
			}

			frag = &sinfo->frags[sinfo->nr_frags++];

			data_len = min_t(u32, kattr->test.data_size_in - size,
					 PAGE_SIZE);
			skb_frag_fill_page_desc(frag, page, 0, data_len);
	...
}

When a multi-buffer BPF_PROG_TEST_RUN runs on that queue after (or concurrently
with) generic XDP and the program shrinks and releases a fragment via
bpf_xdp_adjust_tail() or bpf_xdp_pull_data(), the call chain is:

bpf_xdp_shrink_data()
  -> __xdp_return(netmem, MEM_TYPE_PAGE_POOL, false, NULL)
    -> page_pool_put_full_netmem()
      -> page_pool_unref_netmem()

Because the fragment page in bpf_prog_test_run_xdp() was allocated with
alloc_page(GFP_KERNEL) rather than a page_pool, atomic_long_sub_return() in
page_pool_unref_netmem() underflows pp_ref_count from 0 to -1, triggering
WARN_ON(ret < 0) and leaking the page since sinfo->nr_frags has already been
decremented. If the buddy page previously came from a page_pool where
page_pool_return_netmem() cleared page->pp to NULL while leaving
pp_ref_count == 1, page_pool_put_unrefed_netmem() will also dereference a NULL
pool pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104121316.1155680-1-sst@poczta.fm?part=1