Re: [PATCH v2 bpf 1/2] xdp: update xdp_rxq_info's mem type in XDP generic hook

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Oct 20, 2025 at 05:36:06PM +0200, Alexander Lobakin wrote:
> From: Jesper Dangaard Brouer <hawk@kernel.org>
> Date: Mon, 20 Oct 2025 13:20:57 +0200
> 
> > 
> > 
> > On 17/10/2025 16.31, Maciej Fijalkowski wrote:
> >> Currently, generic XDP hook uses xdp_rxq_info from netstack Rx queues
> >> which do not have its XDP memory model registered. There is a case when
> >> XDP program calls bpf_xdp_adjust_tail() BPF helper, which in turn
> >> releases underlying memory. This happens when it consumes enough amount
> >> of bytes and when XDP buffer has fragments. For this action the memory
> >> model knowledge passed to XDP program is crucial so that core can call
> >> suitable function for freeing/recycling the page.
> >>
> >> For netstack queues it defaults to MEM_TYPE_PAGE_SHARED (0) due to lack
> >> of mem model registration. The problem we're fixing here is when kernel
> >> copied the skb to new buffer backed by system's page_pool and XDP buffer
> >> is built around it. Then when bpf_xdp_adjust_tail() calls
> >> __xdp_return(), it acts incorrectly due to mem type not being set to
> >> MEM_TYPE_PAGE_POOL and causes a page leak.
> >>
> > 
> > Does the code not set the skb->pp_recycle ?

Hi Jesper,

yes it does and I based on this my initial solution, however Jakub had
concerns:

https://lore.kernel.org/bpf/20251001082737.23f5037f@kernel.org/

> 
> You mean this CoW code which replaces the buffers in the skb with system
> PP-backed ones?

skb_pp_cow_data() takes arbitrary page_pool as an input, it does not imply
we're gonna be dealing with system pp only. veth provides its own and
generic xdp uses system pp (the two existing skb_pp_cow_data() callsites).

> Maybe that's the problem (I don't remember the details of the function)?
> 
> > 
> >> Pull out the existing code from bpf_prog_run_generic_xdp() that
> >> init/prepares xdp_buff onto new helper xdp_convert_skb_to_buff() and
> >> embed there rxq's mem_type initialization that is assigned to xdp_buff.
> 
> [...]
> 
> >> +    if (skb_is_nonlinear(skb)) {
> >> +        skb_shinfo(skb)->xdp_frags_size = skb->data_len;
> >> +        xdp_buff_set_frags_flag(xdp);
> >> +    } else {
> >> +        xdp_buff_clear_frags_flag(xdp);
> >> +    }
> >> +
> > 
> > The SKB should be marked via skb->pp_recycle, but I guess you are trying
> > to catch code that doesn't set this correctly?
> > (Slightly worried this will "paper-over" some other buggy code?)
> > 
> >> +    xdp->rxq->mem.type = page_pool_page_is_pp(virt_to_page(xdp->data)) ?
> 
> BTW this may return incorrect results if the page is not order-0.
> IIRC system PPs always return order-0 pages, what about veth code etc?

veth's pp works on order-0 pages well, however I agree it would be better
to use virt_to_head_page() here.

> 
> >> +                MEM_TYPE_PAGE_POOL : MEM_TYPE_PAGE_SHARED;
> >> +}
> > 
> > In the beginning PP_MAGIC / PP_SIGNATURE was primarily used as a
> > debugging feature to catch faulty code that released pp pages to the
> > real page allocator.  It seems to have evolved into something more
> > critical.  Someone also tried to elevate this into a page flag, which
> > would make this more reliable.

exactly here we have the very same issue and we need to correctly return
pages back to their originating page pool.

> Thanks,
> Olek