bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

[kernel-patches-daemon-bpf]

Pull request for series with
subject: bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146649

[kernel-patches-daemon-bpf]

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146649
version: 1

[kernel-patches-review-bot]

commit bed4b6e655c503643e1537f4aeeb17a068e5eefa
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

This commit fixes a page_pool leak reported by syzbot when generic XDP
programs shrink skb frags. The fix makes bpf_xdp_shrink_data() check the
frag itself with netmem_is_pp() rather than trusting the shared rxq mem type.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 16845987b244..3aab5556abe2 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -86,6 +86,7 @@
>  #include <net/inet_dscp.h>
>
>  #include "dev.h"
> +#include "netmem_priv.h"
>
>  /* Keep the struct bpf_fib_lookup small so that it fits into a cacheline */
>  static_assert(sizeof(struct bpf_fib_lookup) == 64, "struct bpf_fib_lookup size check");
> @@ -4288,6 +4289,14 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
>  	if (mem_type == MEM_TYPE_XSK_BUFF_POOL) {
>  		netmem = 0;
>  		zc_frag = bpf_xdp_shrink_data_zc(xdp, shrink, tail, release);
> +	} else if (netmem_is_pp(netmem)) {
> +		/* The rxq mem model does not always describe how a released
> +		 * frag must be freed: the generic-XDP and veth paths run the
> +		 * program on a shared rxq while the frag has been cow'd into
> +		 * page_pool memory. Trust the page itself and return a
> +		 * page_pool frag to its pool regardless of rxq->mem.type.
> +		 */
> +		mem_type = MEM_TYPE_PAGE_POOL;
>  	}
>
>  	if (release) {

The comment says the fix handles rxq->mem.type "regardless" of its value,
but the implementation still checks MEM_TYPE_XSK_BUFF_POOL first. When the
shared rxq has been left with mem_type == MEM_TYPE_XSK_BUFF_POOL by
veth_xdp_rcv_one() (which writes frame->mem_type into the shared rxq on
XDP_TX/XDP_REDIRECT and never restores it), a page_pool frag skips the new
netmem_is_pp() branch entirely and goes to bpf_xdp_shrink_data_zc() instead.

Should the comment say "regardless of rxq->mem.type, unless the rxq claims
XSK memory" or should the netmem_is_pp() test be unconditional so the stale
MEM_TYPE_XSK_BUFF_POOL case is also handled?

The commit message states: "A single rxq->mem.type can be wrong in either
direction." The code, however, only corrects one direction: it upgrades
mem_type to MEM_TYPE_PAGE_POOL when the frag is a page_pool page, but it
never downgrades when rxq->mem.type already says MEM_TYPE_PAGE_POOL and the
frag is not a page_pool page.

That second direction is reachable on the very path the commit message
cites. veth_xdp_rcv_one() (drivers/net/veth.c:638) points the buff at the
shared rxq and then, on XDP_TX/XDP_REDIRECT, writes the frame's type into
it:

    xdp->rxq = &rq->xdp_rxq;
    ...
    case XDP_TX:
            xdp->rxq->mem.type = frame->mem_type;
    case XDP_REDIRECT:
            xdp->rxq->mem.type = frame->mem_type;

Once one redirected frame with mem_type == MEM_TYPE_PAGE_POOL is TX'd or
redirected, rq->xdp_rxq.mem.type stays MEM_TYPE_PAGE_POOL for every
subsequent packet on that queue.

A later frame whose frags are plain pages then reaches:

    } else if (netmem_is_pp(netmem)) {   /* false, mem_type left as PAGE_POOL */
    ...
    __xdp_return(netmem, mem_type, false, zc_frag);

and __xdp_return()'s MEM_TYPE_PAGE_POOL arm (net/core/xdp.c:436-446) skips
the check and dereferences a garbage pool pointer:

    /* No need to check netmem_is_pp() as mem->type knows this a page_pool page */
    page_pool_put_full_netmem(netmem_get_pp(netmem), netmem, napi_direct);

netmem_get_pp() reads page->pp, which for a non-page_pool page holds
whatever the allocator left there.

This specific crash is pre-existing rather than introduced by the patch, but
the patch is the change that claims to make the frag itself authoritative,
and it had the netmem in hand. Could the decision be unconditional:

    } else {
            mem_type = netmem_is_pp(netmem) ? MEM_TYPE_PAGE_POOL : mem_type;

plus a guard for the stale-MEM_TYPE_PAGE_POOL case, or should the "wrong in
either direction" claim be dropped from the commit message so the remaining
hole is not reported as fixed?

This isn't a bug, but a subsystem pattern flags this as potentially
concerning: the new multi-line block comment added at net/core/filter.c
starts its text on the same line as the opening:

    /* The rxq mem model does not always describe how a released
     * frag must be freed: ...
     */

The BPF subsystem style required for net/core/filter.c puts the opening on
a line of its own:

    /*
     * The rxq mem model does not always describe how a released
     * frag must be freed: ...
     */

Most existing multi-line comments in net/core/filter.c use the old style, so
the new comment matches its immediate neighbours and checkpatch will not
complain. The guide says the rule applies even where surrounding code uses
the old style, but the decision is yours.

---

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

In-Reply-To-Subject: bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31924332756

[kernel-patches-daemon-bpf]

Forwarding comment 5305619814 via email
In-Reply-To: 20260816031245.268898-1-jiayuan.chen@linux.dev
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260816031245.268898-1-jiayuan.chen@linux.dev/

[kernel-patches-daemon-bpf]

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146649
version: 1