Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

On 8/24/26 6:31 PM, Lorenzo Bianconi wrote:
>> bpf_xdp_shrink_data() frees a released frag via __xdp_return() using
>> xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is
>> cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED,
>> so the page_pool page is freed with page_frag_free() and we hit
>> "Bad page state ... page_pool leak".
>>
>> Both generic XDP and veth are affected. A non-linear skb is cow'd into
>> page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic
>> XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become
>> page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED.
>>
>> We can't just fix rxq->mem.type in place:
>> - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see
>>    bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in
>>    parallel, so we must not write to it.
>> - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP
>>    teardown, and with GRO that reset runs without stopping in-flight NAPI,
>>    so a type stashed there can be clobbered under a packet still in flight.
>>
>> Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an
>> option either: without recording it somewhere, both can only guess the
>> frag's memory type, which quickly gets confusing.
>>
>> So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
>> skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the
>> page_pool when it is set, otherwise it keeps falling back to
>> xdp->rxq->mem.type unchanged. No other path changes behaviour.
>>
>> Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
>> Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
> Hi Jiayuan Chen,
>
> thx for fixing it. Can we do something like the patch below instead?
>
> Regards,
> Lorenzo
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 1d679e8fd649..4ed659b58141 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -433,16 +433,16 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_attach_page_pool);
>   void __xdp_return(netmem_ref netmem, enum xdp_mem_type mem_type,
>   		  bool napi_direct, struct xdp_buff *xdp)
>   {
> +	netmem_ref head_netmem = netmem_compound_head(netmem);
> +	if (netmem_is_pp(head_netmem))
> +		mem_type = MEM_TYPE_PAGE_POOL;
> +
>   	switch (mem_type) {
>   	case MEM_TYPE_PAGE_POOL:
> -		netmem = netmem_compound_head(netmem);
>   		if (napi_direct && xdp_return_frame_no_direct())
>   			napi_direct = false;
> -		/* 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);
> +		page_pool_put_full_netmem(netmem_get_pp(head_netmem),
> +					  head_netmem, napi_direct);
>   		break;
>   	case MEM_TYPE_PAGE_SHARED:
>   		page_frag_free(__netmem_address(netmem));


Hi Lorenzo,

I tried this, but it regresses the bpf selftest with a page_pool ref 
underflow (0 warns on master, 45 with the patch):

     WARNING: include/net/page_pool/helpers.h:297 at 
page_pool_alloc_frag_netmem
     skb_pp_cow_data
     veth_xdp_rcv_skb


On XDP_TX/XDP_REDIRECT veth has to take plain page refs via get_page() 
(veth_xdp_get()) and then
consume_skb(): the skb itself must be freed while the data pages stay 
alive for the frame. consume_skb()
already returns the skb's page_pool ref, so what the frame holds 
afterwards is a plain page ref, to be
dropped with page_frag_free().

netmem_is_pp() can't see that: it only says the page still belongs to a 
pool (other users may still hold pool refs on the same page),
not what kind of ref we're dropping. So __xdp_return() turns those 
plain-ref drops into a second pool
put and pp_ref goes negative.

That's why I kept the type in the xdp_buff and only override it in the 
shrink path, where we know the
frag ref is the cow'd page_pool one.

Regards,
Jiayuan