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:
>> 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


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.