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

On 8/17/26 10:15 PM, Daniel Borkmann wrote:

On 8/16/26 7:16 AM, Jiayuan Chen wrote:

quoted

On 8/16/26 11:57 AM, bot+bpf-ci@kernel.org wrote:

quoted

quoted

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.

Could you look into this as well, and make this a small series along 
with a BPF selftest
which exercises/tests this bug?

Thanks,
Daniel

Thanks Daniel. I'm still thinking of a unified fix( for example 
__xdp_return() since that's the one place 
bpf_xdp_shrink_data/xdp_return_buff/xdp_convert_buff_to_frame go
through), deciding by the page itself there handles both directions at once.

I'll work on it.