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

[Daniel Borkmann]

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