[PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

[PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

[Jiayuan Chen]

syzbot reported a "Bad page state ... page_pool leak" when a generic XDP
program shrinks an skb into its frags on the tun write() path:

  page_frag_free
  __xdp_return
  bpf_xdp_shrink_data
  bpf_xdp_frags_shrink_tail
  bpf_xdp_adjust_tail

When a program shrinks a whole frag, bpf_xdp_shrink_data() frees it via
__xdp_return() using xdp->rxq->mem.type. For skb-backed XDP the skb is
first rebuilt into page_pool memory (skb_cow_data_for_xdp() for generic
XDP, skb_pp_cow_data() for veth), so the frag is a page_pool page. But the
rxq was registered as MEM_TYPE_PAGE_SHARED, so __xdp_return() calls
page_frag_free() on a page_pool page: its base refcount drops to 0 and the
page is freed to the buddy allocator with pp_magic still set.

The rxq mem model cannot be relied on here because the rxq is shared and
does not describe the frag's real memory. The netdev generic rxq is used
both by generic XDP (page_pool frags) and by bpf_prog_test_run_xdp(), which
borrows the loopback rxq for plain alloc_page() frags; veth uses one rxq
for cow'd (page_pool) skbs and for redirected frames of any memory type. A
single rxq->mem.type can be wrong in either direction.

The memory type is really a property of the page. Check the frag itself
with netmem_is_pp() and return page_pool frags to their pool, keeping the
rxq mem type only for non page_pool pages. The page_pool is taken from the
page (netmem_get_pp()), so no rxq registration is involved.

Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
Reported-by: syzbot+237bbeed8dfe0699b7f5@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/core/filter.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 3423734124a5..4536bd147c25 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -89,6 +89,7 @@
 #include <net/ip6_route.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");
@@ -4293,6 +4294,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) {
-- 
2.43.0

Re: [PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4721 bytes --]

> 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

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31924332756

Re: [PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

[Jiayuan Chen]

On 8/16/26 11:57 AM, bot+bpf-ci@kernel.org wrote:
>> 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;



This is a non-pool page being mistreated as a pool page, and I think 
it's a separate issue, xdp_return_buff and xdp_convert_buff_to_frame hit 
it too, not just this path.