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

On 8/16/26 7:16 AM, Jiayuan Chen wrote:
> On 8/16/26 11:57 AM, [email protected] 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