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

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.