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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 4729 bytes --]

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

Right. I can see the point now :). IIUC we are currently able to trigger
the issue just on xdp fragments running bpf_xdp_shrink_data() but the problem
theoretically occurs even for the xdp->data, right? (it is rallocated using the
page_pool in skb_pp_cow_data()). Is it better to always set this new flag when
the buffers are reallocated via skb_pp_cow_data()? (Maybe renaming it in
something like XDP_FLAGS_DATA_FROM_PP).

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]