Re: [PATCH bpf] net: return page_pool frags to the pool in bpf_xdp_shrink_data()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: bpf_xdp_shrink_data() unrefs page_pool on non-recycled shared frags
- [Low] bpf: multi-line comment style violation in bpf_xdp_shrink_data()
--

commit a0cb1ce1fd03b9aff34c23b2b43d38801d791b8d
Author: Dudu Lu <phx0fer@gmail.com>

net: return page_pool frags to the pool in bpf_xdp_shrink_data()

This commit updates bpf_xdp_shrink_data() in net/core/filter.c to check
whether a released fragment with MEM_TYPE_PAGE_SHARED is backed by a
page_pool and recycle it via napi_pp_put_page() before falling back to
__xdp_return(). This avoids freeing page_pool-backed fragments allocated
by skb_pp_cow_data() through page_frag_free() under generic xdp when the
rxq has no registered memory model.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index b7ac145b68b1f..f17113ff29874 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -4408,6 +4409,18 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
>  	}
>  
>  	if (release) {
> +		/* The frag may be page_pool-backed even when the rxq's
> +		 * xdp_rxq_info carries no mem model (generic XDP on a
> +		 * driver that never registers one, e.g. loopback).  In that
> +		 * case mem_type is MEM_TYPE_PAGE_SHARED and __xdp_return()
> +		 * would free the page through page_frag_free(), leaving the
> +		 * page_pool's accounting dangling -- "page_pool leak".  Route
> +		 * page_pool pages back through the pool instead.
> +		 */

[Severity: Low]
This isn't a bug, but in bpf subsystem files such as net/core/filter.c,
should multi-line comments in bpf_xdp_shrink_data() have the opening /* on
its own line with the comment text starting on the following line?

> +		if (IS_ENABLED(CONFIG_PAGE_POOL) &&
> +		    mem_type == MEM_TYPE_PAGE_SHARED && napi_pp_put_page(netmem))
> +			return release;
> +
>  		__xdp_return(netmem, mem_type, false, zc_frag);

[Severity: High]
Can calling napi_pp_put_page(netmem) in bpf_xdp_shrink_data() when
mem_type == MEM_TYPE_PAGE_SHARED corrupt reference counts if the xdp_buff
owns a page->_refcount reference rather than a pp_ref_count reference?

On a veth interface with native xdp enabled, veth_xdp_rcv_skb() calls
veth_convert_skb_to_xdp_buff() -> skb_pp_cow_data(). For any trailing
fragment with size <= PAGE_SIZE / 2 (such as a 4000-byte packet),
page_pool_dev_alloc() -> page_pool_alloc_netmem() calls
page_pool_alloc_frag_netmem(), which fragments the page with BIAS_MAX:

net/core/page_pool.c:page_pool_alloc_frag_netmem() {
    ...
		pool->frag_page = netmem;

frag_reset:
		pool->frag_users = 1;
		*offset = 0;
		pool->frag_offset = size;
		page_pool_fragment_netmem(netmem, BIAS_MAX);
		return netmem;
}

When bpf_prog_run_xdp() returns XDP_TX or XDP_REDIRECT, veth_xdp_rcv_skb()
transfers ownership from the skb to the xdp_buff and switches the memory
model to rq->xdp_mem (which has type MEM_TYPE_PAGE_SHARED):

drivers/net/veth.c:veth_xdp_rcv_skb() {
    ...
	case XDP_TX:
		veth_xdp_get(xdp);
		consume_skb(skb);
		xdp->rxq->mem = rq->xdp_mem;
		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
    ...
}

Here, veth_xdp_get(xdp) increments page->_refcount via __skb_frag_ref(),
and consume_skb(skb) releases the skb's pp_ref_count reference via
napi_pp_put_page() (decrementing pp_ref_count from BIAS_MAX to
BIAS_MAX - 1). Because rq->page_pool->frag_page (or another skb sharing
the fragmented page) still holds the remaining pp_ref_count references,
pp_ref_count does not reach 0 and netmem_is_pp(netmem) remains true.

When a downstream program in veth_xdp_rcv_one(), dev_map_bpf_prog_run(),
or cpu_map_bpf_prog_run_xdp() shrinks and releases that fragment via
bpf_xdp_adjust_tail() or bpf_xdp_pull_data(), bpf_xdp_shrink_data() sees
mem_type == MEM_TYPE_PAGE_SHARED and netmem_is_pp(netmem) == true. Calling
napi_pp_put_page(netmem) decrements pp_ref_count a second time instead of
calling __xdp_return() -> page_frag_free() to drop the page->_refcount
reference.

Wouldn't this leak page->_refcount and cause page_pool_unref_netmem() to
underflow pp_ref_count (or prematurely recycle the page while other skbs
still hold fragments of it) when rq->page_pool drains frag_page?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021155645.69120-1-phx0fer@gmail.com?part=1