[PATCH v5 bpf 0/2] xdp: fix page_pool leaks

[PATCH v5 bpf 0/2] xdp: fix page_pool leaks

[Maciej Fijalkowski]

v1:
https://lore.kernel.org/bpf/20251003140243.2534865-1-maciej.fijalkowski@intel.com/
v1->v2:
- add skb to xdp_buff converter (Jakub)
- postpone restoration of veth's xdp_rxq_info mem model (AI)

v2:
https://lore.kernel.org/bpf/20251017143103.2620164-1-maciej.fijalkowski@intel.com/T/
v2->v3:
- make skb->xdp_buff conversion agnostic of current skb->data position
  (Toke)
- do not pull mac header in veth (Toke)
- use virt_to_head_page() when checking if xdp->data comes from
  page_pool (Olek)
- add ack from Toke

v3:
https://lore.kernel.org/bpf/20251022125209.2649287-1-maciej.fijalkowski@intel.com/
v3->v4:
- include skb->head when calculating packet length (Toke, lkp)

v4:
https://lore.kernel.org/bpf/20251027121318.2679226-1-maciej.fijalkowski@intel.com/
v4->v5:
- rely again on sk_buff::pp_recycle when setting mem type on rxq
  (Jesper, Jakub)
- add ack from Toke on patch 2 and restore it on patch 1


Hi,

here we fix page_pool leaks which happen when fragment is released
within XDP program and memory type is set incorrectly. The reports come
from syzbot and AF_XDP test suite.

Most of the changes are about pulling out the common code for
init/prepare xdp_buff based on existing skb and supplying it with
correct xdp_rxq_info's mem_type that is assigned to xdp_buff. A bit more
stuff, page_pool related, had to be done on veth side.

Originally, this work was started by Octavian Purdila.

Thanks!


Maciej Fijalkowski (2):
  xdp: introduce xdp_convert_skb_to_buff()
  veth: update mem type in xdp_buff

 drivers/net/veth.c | 43 ++++++++++++++++++++++++++-----------------
 include/net/xdp.h  | 25 +++++++++++++++++++++++++
 net/core/dev.c     | 25 ++++---------------------
 3 files changed, 55 insertions(+), 38 deletions(-)

-- 
2.43.0

[PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Maciej Fijalkowski]

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.

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.
Make it agnostic to current skb->data position.

This problem was triggered by syzbot as well as AF_XDP test suite which
is about to be integrated to BPF CI.

Reported-by: syzbot+ff145014d6b0ce64a173@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6756c37b.050a0220.a30f1.019a.GAE@google.com/
Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic mode")
Tested-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Co-developed-by: Octavian Purdila <tavip@google.com>
Signed-off-by: Octavian Purdila <tavip@google.com> # whole analysis, testing, initiating a fix
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> # commit msg and proposed more robust fix
---
 include/net/xdp.h | 25 +++++++++++++++++++++++++
 net/core/dev.c    | 25 ++++---------------------
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/include/net/xdp.h b/include/net/xdp.h
index aa742f413c35..be7cc2eb956d 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -384,6 +384,31 @@ struct sk_buff *xdp_build_skb_from_frame(struct xdp_frame *xdpf,
 					 struct net_device *dev);
 struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf);
 
+static inline
+void xdp_convert_skb_to_buff(struct sk_buff *skb, struct xdp_buff *xdp,
+			     struct xdp_rxq_info *xdp_rxq)
+{
+	u32 frame_sz, pkt_len;
+
+	/* SKB "head" area always have tailroom for skb_shared_info */
+	frame_sz = skb_end_pointer(skb) - skb->head;
+	frame_sz += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	pkt_len =  skb_tail_pointer(skb) - skb_mac_header(skb);
+
+	xdp_init_buff(xdp, frame_sz, xdp_rxq);
+	xdp_prepare_buff(xdp, skb->head, skb->mac_header, pkt_len, true);
+
+	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);
+	}
+
+	xdp->rxq->mem.type = skb->pp_recycle ? MEM_TYPE_PAGE_POOL :
+					       MEM_TYPE_PAGE_SHARED;
+}
+
 static inline
 void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
 			       struct xdp_buff *xdp)
diff --git a/net/core/dev.c b/net/core/dev.c
index 2acfa44927da..a71da4edc493 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5320,35 +5320,18 @@ static struct netdev_rx_queue *netif_get_rxqueue(struct sk_buff *skb)
 u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 			     const struct bpf_prog *xdp_prog)
 {
-	void *orig_data, *orig_data_end, *hard_start;
+	void *orig_data, *orig_data_end;
 	struct netdev_rx_queue *rxqueue;
 	bool orig_bcast, orig_host;
-	u32 mac_len, frame_sz;
+	u32 metalen, act, mac_len;
 	__be16 orig_eth_type;
 	struct ethhdr *eth;
-	u32 metalen, act;
 	int off;
 
-	/* The XDP program wants to see the packet starting at the MAC
-	 * header.
-	 */
+	rxqueue = netif_get_rxqueue(skb);
 	mac_len = skb->data - skb_mac_header(skb);
-	hard_start = skb->data - skb_headroom(skb);
-
-	/* SKB "head" area always have tailroom for skb_shared_info */
-	frame_sz = (void *)skb_end_pointer(skb) - hard_start;
-	frame_sz += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	rxqueue = netif_get_rxqueue(skb);
-	xdp_init_buff(xdp, frame_sz, &rxqueue->xdp_rxq);
-	xdp_prepare_buff(xdp, hard_start, skb_headroom(skb) - mac_len,
-			 skb_headlen(skb) + mac_len, true);
-	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);
-	}
+	xdp_convert_skb_to_buff(skb, xdp, &rxqueue->xdp_rxq);
 
 	orig_data_end = xdp->data_end;
 	orig_data = xdp->data;
-- 
2.43.0

[PATCH v5 bpf 2/2] veth: update mem type in xdp_buff

[Maciej Fijalkowski]

When skb's headroom is not sufficient for XDP purposes,
skb_pp_cow_data() returns new skb with requested headroom space. This
skb was provided by page_pool.

For CONFIG_DEBUG_VM=y and XDP program that uses bpf_xdp_adjust_tail()
against a skb with frags, and mentioned helper consumed enough amount of
bytes that in turn released the page, following splat was observed:

[   32.204881] BUG: Bad page state in process test_progs  pfn:11c98b
[   32.207167] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x11c98b
[   32.210084] flags: 0x1fffe0000000000(node=0|zone=1|lastcpupid=0x7fff)
[   32.212493] raw: 01fffe0000000000 dead000000000040 ff11000123c9b000 0000000000000000
[   32.218056] raw: 0000000000000000 0000000000000001 00000000ffffffff 0000000000000000
[   32.220900] page dumped because: page_pool leak
[   32.222636] Modules linked in: bpf_testmod(O) bpf_preload
[   32.224632] CPU: 6 UID: 0 PID: 3612 Comm: test_progs Tainted: G O        6.17.0-rc5-gfec474d29325 #6969 PREEMPT
[   32.224638] Tainted: [O]=OOT_MODULE
[   32.224639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[   32.224641] Call Trace:
[   32.224644]  <IRQ>
[   32.224646]  dump_stack_lvl+0x4b/0x70
[   32.224653]  bad_page.cold+0xbd/0xe0
[   32.224657]  __free_frozen_pages+0x838/0x10b0
[   32.224660]  ? skb_pp_cow_data+0x782/0xc30
[   32.224665]  bpf_xdp_shrink_data+0x221/0x530
[   32.224668]  ? skb_pp_cow_data+0x6d1/0xc30
[   32.224671]  bpf_xdp_adjust_tail+0x598/0x810
[   32.224673]  ? xsk_destruct_skb+0x321/0x800
[   32.224678]  bpf_prog_004ac6bb21de57a7_xsk_xdp_adjust_tail+0x52/0xd6
[   32.224681]  veth_xdp_rcv_skb+0x45d/0x15a0
[   32.224684]  ? get_stack_info_noinstr+0x16/0xe0
[   32.224688]  ? veth_set_channels+0x920/0x920
[   32.224691]  ? get_stack_info+0x2f/0x80
[   32.224693]  ? unwind_next_frame+0x3af/0x1df0
[   32.224697]  veth_xdp_rcv.constprop.0+0x38a/0xbe0
[   32.224700]  ? common_startup_64+0x13e/0x148
[   32.224703]  ? veth_xdp_rcv_one+0xcd0/0xcd0
[   32.224706]  ? stack_trace_save+0x84/0xa0
[   32.224709]  ? stack_depot_save_flags+0x28/0x820
[   32.224713]  ? __resched_curr.constprop.0+0x332/0x3b0
[   32.224716]  ? timerqueue_add+0x217/0x320
[   32.224719]  veth_poll+0x115/0x5e0
[   32.224722]  ? veth_xdp_rcv.constprop.0+0xbe0/0xbe0
[   32.224726]  ? update_load_avg+0x1cb/0x12d0
[   32.224730]  ? update_cfs_group+0x121/0x2c0
[   32.224733]  __napi_poll+0xa0/0x420
[   32.224736]  net_rx_action+0x901/0xe90
[   32.224740]  ? run_backlog_napi+0x50/0x50
[   32.224743]  ? clockevents_program_event+0x1cc/0x280
[   32.224746]  ? hrtimer_interrupt+0x31e/0x7c0
[   32.224749]  handle_softirqs+0x151/0x430
[   32.224752]  do_softirq+0x3f/0x60
[   32.224755]  </IRQ>

It's because xdp_rxq with mem model set to MEM_TYPE_PAGE_SHARED was used
when initializing xdp_buff.

Fix this by using new helper xdp_convert_skb_to_buff() that, besides
init/prepare xdp_buff, will check if page used for linear part of
xdp_buff comes from page_pool. We assume that linear data and frags will
have same memory provider as currently XDP API does not provide us a way
to distinguish it (the mem model is registered for *whole* Rx queue and
here we speak about single buffer granularity).

Before releasing xdp_buff out of veth via XDP_{TX,REDIRECT}, mem type on
xdp_rxq associated with xdp_buff is restored to its original model. We
need to respect previous setting at least until buff is converted to
frame, as frame carries the mem_type. Add a page_pool variant of
veth_xdp_get() so that we avoid refcount underflow when draining page
frag.

Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/CAADnVQ+bBofJDfieyOYzSmSujSfJwDTQhiz3aJw7hE+4E2_iPA@mail.gmail.com/
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/veth.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index a3046142cb8e..187f30e2cb4b 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -733,7 +733,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
 	}
 }
 
-static void veth_xdp_get(struct xdp_buff *xdp)
+static void veth_xdp_get_shared(struct xdp_buff *xdp)
 {
 	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	int i;
@@ -746,12 +746,33 @@ static void veth_xdp_get(struct xdp_buff *xdp)
 		__skb_frag_ref(&sinfo->frags[i]);
 }
 
+static void veth_xdp_get_pp(struct xdp_buff *xdp)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+	int i;
+
+	page_pool_ref_page(virt_to_page(xdp->data));
+	if (likely(!xdp_buff_has_frags(xdp)))
+		return;
+
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		skb_frag_t *frag = &sinfo->frags[i];
+
+		page_pool_ref_page(netmem_to_page(frag->netmem));
+	}
+}
+
+static void veth_xdp_get(struct xdp_buff *xdp)
+{
+	xdp->rxq->mem.type == MEM_TYPE_PAGE_POOL ?
+		veth_xdp_get_pp(xdp) : veth_xdp_get_shared(xdp);
+}
+
 static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 					struct xdp_buff *xdp,
 					struct sk_buff **pskb)
 {
 	struct sk_buff *skb = *pskb;
-	u32 frame_sz;
 
 	if (skb_shared(skb) || skb_head_is_locked(skb) ||
 	    skb_shinfo(skb)->nr_frags ||
@@ -762,19 +783,7 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		skb = *pskb;
 	}
 
-	/* SKB "head" area always have tailroom for skb_shared_info */
-	frame_sz = skb_end_pointer(skb) - skb->head;
-	frame_sz += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	xdp_init_buff(xdp, frame_sz, &rq->xdp_rxq);
-	xdp_prepare_buff(xdp, skb->head, skb_headroom(skb),
-			 skb_headlen(skb), true);
-
-	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);
-	}
+	xdp_convert_skb_to_buff(skb, xdp, &rq->xdp_rxq);
 	*pskb = skb;
 
 	return 0;
@@ -822,24 +831,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 	case XDP_TX:
 		veth_xdp_get(xdp);
 		consume_skb(skb);
-		xdp->rxq->mem = rq->xdp_mem;
 		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
 			trace_xdp_exception(rq->dev, xdp_prog, act);
 			stats->rx_drops++;
 			goto err_xdp;
 		}
 		stats->xdp_tx++;
+		rq->xdp_rxq.mem = rq->xdp_mem;
 		rcu_read_unlock();
 		goto xdp_xmit;
 	case XDP_REDIRECT:
 		veth_xdp_get(xdp);
 		consume_skb(skb);
-		xdp->rxq->mem = rq->xdp_mem;
 		if (xdp_do_redirect(rq->dev, xdp, xdp_prog)) {
 			stats->rx_drops++;
 			goto err_xdp;
 		}
 		stats->xdp_redirect++;
+		rq->xdp_rxq.mem = rq->xdp_mem;
 		rcu_read_unlock();
 		goto xdp_xmit;
 	default:
-- 
2.43.0

Re: [PATCH v5 bpf 2/2] veth: update mem type in xdp_buff

[bot+bpf-ci]

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


```
commit 96d6e22337a81e8606de33d22173ff80ab961bc8
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

veth: update mem type in xdp_buff

The commit fixes a page_pool leak by properly detecting page_pool memory
and using appropriate reference counting APIs. The xdp_buff mem type is
now dynamically set based on skb->pp_recycle instead of always using
MEM_TYPE_PAGE_SHARED.

Closes: https://lore.kernel.org/bpf/CAADnVQ+bBofJDfieyOYzSmSujSfJwDTQhiz3aJw7hE+4E2_iPA@mail.gmail.com/

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index a3046142c..187f30e2c 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c

[ ... ]

> @@ -733,7 +733,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
>  	}
>  }
>
> -static void veth_xdp_get(struct xdp_buff *xdp)
> +static void veth_xdp_get_shared(struct xdp_buff *xdp)
>  {
>  	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>  	int i;
> @@ -746,12 +746,33 @@ static void veth_xdp_get(struct xdp_buff *xdp)
>  		__skb_frag_ref(&sinfo->frags[i]);
>  }
>
> +static void veth_xdp_get_pp(struct xdp_buff *xdp)
> +{
> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> +	int i;
> +
> +	page_pool_ref_page(virt_to_page(xdp->data));
> +	if (likely(!xdp_buff_has_frags(xdp)))
> +		return;
> +
> +	for (i = 0; i < sinfo->nr_frags; i++) {
> +		skb_frag_t *frag = &sinfo->frags[i];
> +
> +		page_pool_ref_page(netmem_to_page(frag->netmem));
> +	}
> +}
> +
> +static void veth_xdp_get(struct xdp_buff *xdp)
> +{
> +	xdp->rxq->mem.type == MEM_TYPE_PAGE_POOL ?
> +		veth_xdp_get_pp(xdp) : veth_xdp_get_shared(xdp);
> +}
> +

[ ... ]

> @@ -822,24 +831,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>  	case XDP_TX:
>  		veth_xdp_get(xdp);
>  		consume_skb(skb);
> -		xdp->rxq->mem = rq->xdp_mem;
>  		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
>  			trace_xdp_exception(rq->dev, xdp_prog, act);
>  			stats->rx_drops++;
>  			goto err_xdp;
>  		}

Can veth_xdp_rcv_skb() leak page references in the XDP_TX error path?
When veth_xdp_tx() fails, the code jumps to err_xdp without releasing
the page references acquired by veth_xdp_get(). The skb was already
consumed, so the pages still have the extra references from either
veth_xdp_get_pp() or veth_xdp_get_shared().

veth_xdp_tx() can return -EOVERFLOW when xdp_convert_buff_to_frame()
returns NULL, which is a reachable error path.

>  		stats->xdp_tx++;
> +		rq->xdp_rxq.mem = rq->xdp_mem;
>  		rcu_read_unlock();
>  		goto xdp_xmit;
>  	case XDP_REDIRECT:
>  		veth_xdp_get(xdp);
>  		consume_skb(skb);
> -		xdp->rxq->mem = rq->xdp_mem;
>  		if (xdp_do_redirect(rq->dev, xdp, xdp_prog)) {
>  			stats->rx_drops++;
>  			goto err_xdp;
>  		}

Similar question for the XDP_REDIRECT case - when xdp_do_redirect()
fails, does this also leak the page references taken by veth_xdp_get()?

>  		stats->xdp_redirect++;
> +		rq->xdp_rxq.mem = rq->xdp_mem;
>  		rcu_read_unlock();
>  		goto xdp_xmit;
>  	default:


```

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: `veth: update mem type in xdp_buff`
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18923812102

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Jakub Kicinski]

On Wed, 29 Oct 2025 23:13:14 +0100 Maciej Fijalkowski wrote:
> +	xdp->rxq->mem.type = skb->pp_recycle ? MEM_TYPE_PAGE_POOL :
> +					       MEM_TYPE_PAGE_SHARED;

You really need to stop sending patches before I had a chance 
to reply :/ And this is wrong.
-- 
pw-bot: cr

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Maciej Fijalkowski]

On Wed, Oct 29, 2025 at 04:50:20PM -0700, Jakub Kicinski wrote:
> On Wed, 29 Oct 2025 23:13:14 +0100 Maciej Fijalkowski wrote:
> > +	xdp->rxq->mem.type = skb->pp_recycle ? MEM_TYPE_PAGE_POOL :
> > +					       MEM_TYPE_PAGE_SHARED;
> 
> You really need to stop sending patches before I had a chance 
> to reply :/ And this is wrong.

Why do you say so?

netif_receive_generic_xdp()
	netif_skb_check_for_xdp()
	skb_cow_data_for_xdp() failed
		go through skb linearize path
			returned skb data is backed by kmalloc, not page_pool,
			means mem type for this particular xdp_buff has to be
			MEM_TYPE_PAGE_SHARED

Are we on the same page now?

> -- 
> pw-bot: cr

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Jakub Kicinski]

On Thu, 30 Oct 2025 13:14:16 +0100 Maciej Fijalkowski wrote:
> On Wed, Oct 29, 2025 at 04:50:20PM -0700, Jakub Kicinski wrote:
> > On Wed, 29 Oct 2025 23:13:14 +0100 Maciej Fijalkowski wrote:  
> > > +	xdp->rxq->mem.type = skb->pp_recycle ? MEM_TYPE_PAGE_POOL :
> > > +					       MEM_TYPE_PAGE_SHARED;  
> > 
> > You really need to stop sending patches before I had a chance 
> > to reply :/ And this is wrong.  
> 
> Why do you say so?
> 
> netif_receive_generic_xdp()
> 	netif_skb_check_for_xdp()
> 	skb_cow_data_for_xdp() failed
> 		go through skb linearize path
> 			returned skb data is backed by kmalloc, not page_pool,
> 			means mem type for this particular xdp_buff has to be
> 			MEM_TYPE_PAGE_SHARED
> 
> Are we on the same page now?

No, I think I already covered this, maybe you disagreed and I missed it.

The mem_type set here is expected to be used only for freeing pages. 
XDP can only free fagments (when pkt is trimmed), it cannot free the
head from under the skb. So only fragments matter here, we can ignore
the head.

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Maciej Fijalkowski]

On Thu, Oct 30, 2025 at 08:25:19AM -0700, Jakub Kicinski wrote:
> On Thu, 30 Oct 2025 13:14:16 +0100 Maciej Fijalkowski wrote:
> > On Wed, Oct 29, 2025 at 04:50:20PM -0700, Jakub Kicinski wrote:
> > > On Wed, 29 Oct 2025 23:13:14 +0100 Maciej Fijalkowski wrote:  
> > > > +	xdp->rxq->mem.type = skb->pp_recycle ? MEM_TYPE_PAGE_POOL :
> > > > +					       MEM_TYPE_PAGE_SHARED;  
> > > 
> > > You really need to stop sending patches before I had a chance 
> > > to reply :/ And this is wrong.  
> > 
> > Why do you say so?
> > 
> > netif_receive_generic_xdp()
> > 	netif_skb_check_for_xdp()
> > 	skb_cow_data_for_xdp() failed
> > 		go through skb linearize path
> > 			returned skb data is backed by kmalloc, not page_pool,
> > 			means mem type for this particular xdp_buff has to be
> > 			MEM_TYPE_PAGE_SHARED
> > 
> > Are we on the same page now?
> 
> No, I think I already covered this, maybe you disagreed and I missed it.
> 
> The mem_type set here is expected to be used only for freeing pages. 
> XDP can only free fagments (when pkt is trimmed), it cannot free the
> head from under the skb. So only fragments matter here, we can ignore
> the head.

...and given that linearize path would make skb a frag-less one...okay -
I'm buying this! :D I have some other thoughts, but I would like to
finally close this pandora's box, you probably have similar feelings.

So plain assignment like:
xdp->rxq->mem.type = MEM_TYPE_PAGE_POOL;

would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
patch so have to send v6 anyways.

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Jakub Kicinski]

On Thu, 30 Oct 2025 21:22:34 +0100 Maciej Fijalkowski wrote:
> > > Why do you say so?
> > > 
> > > netif_receive_generic_xdp()
> > > 	netif_skb_check_for_xdp()
> > > 	skb_cow_data_for_xdp() failed
> > > 		go through skb linearize path
> > > 			returned skb data is backed by kmalloc, not page_pool,
> > > 			means mem type for this particular xdp_buff has to be
> > > 			MEM_TYPE_PAGE_SHARED
> > > 
> > > Are we on the same page now?  
> > 
> > No, I think I already covered this, maybe you disagreed and I missed it.
> > 
> > The mem_type set here is expected to be used only for freeing pages. 
> > XDP can only free fagments (when pkt is trimmed), it cannot free the
> > head from under the skb. So only fragments matter here, we can ignore
> > the head.  
> 
> ...and given that linearize path would make skb a frag-less one...okay -
> I'm buying this! :D I have some other thoughts, but I would like to
> finally close this pandora's box, you probably have similar feelings.
> 
> So plain assignment like:
> xdp->rxq->mem.type = MEM_TYPE_PAGE_POOL;

Yes, LGTM.

> would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
> patch so have to send v6 anyways.

The veth side unfortunately needs more work than Mr Robot points out.
For some reason veth tries to turn skb into an xdp_frame..

Either we have to make it not do that - we could probably call
xdp_do_generic_redirect() and for Tx .. figure out the right incantation 
to give the frame back to the peer veth.

Or, if we didn't hit CoW, you need to actually add the incantation we
removed here, there:

	xdp->rxq->mem.type = skb->pp_recycle && page_pool_page_is_pp(..) ?
		MEM_TYPE_PAGE_POOL : MEM_TYPE_PAGE_SHARED;

Or CoW the head retroactively if we hit the Tx/Redir path.

My intuition is that first option (making the handling as similar to
XDP generic as possible) is going to be least buggy long term.

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Maciej Fijalkowski]

On Thu, Oct 30, 2025 at 07:05:11PM -0700, Jakub Kicinski wrote:
> On Thu, 30 Oct 2025 21:22:34 +0100 Maciej Fijalkowski wrote:
> > > > Why do you say so?
> > > > 
> > > > netif_receive_generic_xdp()
> > > > 	netif_skb_check_for_xdp()
> > > > 	skb_cow_data_for_xdp() failed
> > > > 		go through skb linearize path
> > > > 			returned skb data is backed by kmalloc, not page_pool,
> > > > 			means mem type for this particular xdp_buff has to be
> > > > 			MEM_TYPE_PAGE_SHARED
> > > > 
> > > > Are we on the same page now?  
> > > 
> > > No, I think I already covered this, maybe you disagreed and I missed it.
> > > 
> > > The mem_type set here is expected to be used only for freeing pages. 
> > > XDP can only free fagments (when pkt is trimmed), it cannot free the
> > > head from under the skb. So only fragments matter here, we can ignore
> > > the head.  
> > 
> > ...and given that linearize path would make skb a frag-less one...okay -
> > I'm buying this! :D I have some other thoughts, but I would like to
> > finally close this pandora's box, you probably have similar feelings.
> > 
> > So plain assignment like:
> > xdp->rxq->mem.type = MEM_TYPE_PAGE_POOL;
> 
> Yes, LGTM.
> 
> > would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
> > patch so have to send v6 anyways.
> 
> The veth side unfortunately needs more work than Mr Robot points out.
> For some reason veth tries to turn skb into an xdp_frame..

That is beyond the scope of the fix that I started doing as you're
undermining overall XDP support in veth, IMHO.

I can follow up on this on some undefined future but right now I will
have to switch to some other work.

If you disagree and insist on addressing skb->xdp_frame in veth within
this patchset then I'm sorry but I will have to postpone my activities
here.

> 
> Either we have to make it not do that - we could probably call
> xdp_do_generic_redirect() and for Tx .. figure out the right incantation 
> to give the frame back to the peer veth.
> 
> Or, if we didn't hit CoW, you need to actually add the incantation we
> removed here, there:
> 
> 	xdp->rxq->mem.type = skb->pp_recycle && page_pool_page_is_pp(..) ?
> 		MEM_TYPE_PAGE_POOL : MEM_TYPE_PAGE_SHARED;
> 
> Or CoW the head retroactively if we hit the Tx/Redir path.
> 
> My intuition is that first option (making the handling as similar to
> XDP generic as possible) is going to be least buggy long term.

Re: [PATCH v5 bpf 2/2] veth: update mem type in xdp_buff

[Maciej Fijalkowski]

On Wed, Oct 29, 2025 at 10:59:36PM +0000, bot+bpf-ci@kernel.org wrote:
> 
> ```
> commit 96d6e22337a81e8606de33d22173ff80ab961bc8
> Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> veth: update mem type in xdp_buff
> 
> The commit fixes a page_pool leak by properly detecting page_pool memory
> and using appropriate reference counting APIs. The xdp_buff mem type is
> now dynamically set based on skb->pp_recycle instead of always using
> MEM_TYPE_PAGE_SHARED.
> 
> Closes: https://lore.kernel.org/bpf/CAADnVQ+bBofJDfieyOYzSmSujSfJwDTQhiz3aJw7hE+4E2_iPA@mail.gmail.com/
> 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index a3046142c..187f30e2c 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> 
> [ ... ]
> 
> > @@ -733,7 +733,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
> >  	}
> >  }
> >
> > -static void veth_xdp_get(struct xdp_buff *xdp)
> > +static void veth_xdp_get_shared(struct xdp_buff *xdp)
> >  {
> >  	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> >  	int i;
> > @@ -746,12 +746,33 @@ static void veth_xdp_get(struct xdp_buff *xdp)
> >  		__skb_frag_ref(&sinfo->frags[i]);
> >  }
> >
> > +static void veth_xdp_get_pp(struct xdp_buff *xdp)
> > +{
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	int i;
> > +
> > +	page_pool_ref_page(virt_to_page(xdp->data));
> > +	if (likely(!xdp_buff_has_frags(xdp)))
> > +		return;
> > +
> > +	for (i = 0; i < sinfo->nr_frags; i++) {
> > +		skb_frag_t *frag = &sinfo->frags[i];
> > +
> > +		page_pool_ref_page(netmem_to_page(frag->netmem));
> > +	}
> > +}
> > +
> > +static void veth_xdp_get(struct xdp_buff *xdp)
> > +{
> > +	xdp->rxq->mem.type == MEM_TYPE_PAGE_POOL ?
> > +		veth_xdp_get_pp(xdp) : veth_xdp_get_shared(xdp);
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -822,24 +831,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
> >  	case XDP_TX:
> >  		veth_xdp_get(xdp);
> >  		consume_skb(skb);
> > -		xdp->rxq->mem = rq->xdp_mem;
> >  		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
> >  			trace_xdp_exception(rq->dev, xdp_prog, act);
> >  			stats->rx_drops++;
> >  			goto err_xdp;
> >  		}
> 
> Can veth_xdp_rcv_skb() leak page references in the XDP_TX error path?
> When veth_xdp_tx() fails, the code jumps to err_xdp without releasing
> the page references acquired by veth_xdp_get(). The skb was already
> consumed, so the pages still have the extra references from either
> veth_xdp_get_pp() or veth_xdp_get_shared().

You're missing xdp_return_buff() on err_xdp path.

> 
> veth_xdp_tx() can return -EOVERFLOW when xdp_convert_buff_to_frame()
> returns NULL, which is a reachable error path.
> 
> >  		stats->xdp_tx++;
> > +		rq->xdp_rxq.mem = rq->xdp_mem;
> >  		rcu_read_unlock();
> >  		goto xdp_xmit;
> >  	case XDP_REDIRECT:
> >  		veth_xdp_get(xdp);
> >  		consume_skb(skb);
> > -		xdp->rxq->mem = rq->xdp_mem;
> >  		if (xdp_do_redirect(rq->dev, xdp, xdp_prog)) {
> >  			stats->rx_drops++;
> >  			goto err_xdp;
> >  		}
> 
> Similar question for the XDP_REDIRECT case - when xdp_do_redirect()
> fails, does this also leak the page references taken by veth_xdp_get()?
> 
> >  		stats->xdp_redirect++;
> > +		rq->xdp_rxq.mem = rq->xdp_mem;
> >  		rcu_read_unlock();
> >  		goto xdp_xmit;
> >  	default:
> 
> 
> ```
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> In-Reply-To-Subject: `veth: update mem type in xdp_buff`
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18923812102

Re: [PATCH v5 bpf 2/2] veth: update mem type in xdp_buff

[Chris Mason]

On 10/31/25 8:46 AM, Maciej Fijalkowski wrote:
> On Wed, Oct 29, 2025 at 10:59:36PM +0000, bot+bpf-ci@kernel.org wrote:
>>
>> ```
>> commit 96d6e22337a81e8606de33d22173ff80ab961bc8
>> Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>>
>> veth: update mem type in xdp_buff
>>
>> The commit fixes a page_pool leak by properly detecting page_pool memory
>> and using appropriate reference counting APIs. The xdp_buff mem type is
>> now dynamically set based on skb->pp_recycle instead of always using
>> MEM_TYPE_PAGE_SHARED.
>>
>> Closes: https://lore.kernel.org/bpf/CAADnVQ+bBofJDfieyOYzSmSujSfJwDTQhiz3aJw7hE+4E2_iPA@mail.gmail.com/ 
>>
>>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>>> index a3046142c..187f30e2c 100644
>>> --- a/drivers/net/veth.c
>>> +++ b/drivers/net/veth.c
>>
>> [ ... ]
>>
>>> @@ -733,7 +733,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
>>>  	}
>>>  }
>>>
>>> -static void veth_xdp_get(struct xdp_buff *xdp)
>>> +static void veth_xdp_get_shared(struct xdp_buff *xdp)
>>>  {
>>>  	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>>>  	int i;
>>> @@ -746,12 +746,33 @@ static void veth_xdp_get(struct xdp_buff *xdp)
>>>  		__skb_frag_ref(&sinfo->frags[i]);
>>>  }
>>>
>>> +static void veth_xdp_get_pp(struct xdp_buff *xdp)
>>> +{
>>> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>>> +	int i;
>>> +
>>> +	page_pool_ref_page(virt_to_page(xdp->data));
>>> +	if (likely(!xdp_buff_has_frags(xdp)))
>>> +		return;
>>> +
>>> +	for (i = 0; i < sinfo->nr_frags; i++) {
>>> +		skb_frag_t *frag = &sinfo->frags[i];
>>> +
>>> +		page_pool_ref_page(netmem_to_page(frag->netmem));
>>> +	}
>>> +}
>>> +
>>> +static void veth_xdp_get(struct xdp_buff *xdp)
>>> +{
>>> +	xdp->rxq->mem.type == MEM_TYPE_PAGE_POOL ?
>>> +		veth_xdp_get_pp(xdp) : veth_xdp_get_shared(xdp);
>>> +}
>>> +
>>
>> [ ... ]
>>
>>> @@ -822,24 +831,24 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>>>  	case XDP_TX:
>>>  		veth_xdp_get(xdp);
>>>  		consume_skb(skb);
>>> -		xdp->rxq->mem = rq->xdp_mem;
>>>  		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
>>>  			trace_xdp_exception(rq->dev, xdp_prog, act);
>>>  			stats->rx_drops++;
>>>  			goto err_xdp;
>>>  		}
>>
>> Can veth_xdp_rcv_skb() leak page references in the XDP_TX error path?
>> When veth_xdp_tx() fails, the code jumps to err_xdp without releasing
>> the page references acquired by veth_xdp_get(). The skb was already
>> consumed, so the pages still have the extra references from either
>> veth_xdp_get_pp() or veth_xdp_get_shared().
> 
> You're missing xdp_return_buff() on err_xdp path.

Thanks, looking at the logs, that's exactly what happened.  AI thinks
the goto code looks like this:

drop:
    stats->rx_drops++;
xdp_drop:
    rcu_read_unlock();
    kfree_skb(skb);
err_xdp:
    return NULL;

Since I can't find this code snippet elsewhere in the kernel, I think it
just made it up.  I'll add some language that forces it to confirm the
snippets are correct.

-chris

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Jakub Kicinski]

On Fri, 31 Oct 2025 12:37:37 +0100 Maciej Fijalkowski wrote:
> > > would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
> > > patch so have to send v6 anyways.  
> > 
> > The veth side unfortunately needs more work than Mr Robot points out.
> > For some reason veth tries to turn skb into an xdp_frame..  
> 
> That is beyond the scope of the fix that I started doing as you're
> undermining overall XDP support in veth, IMHO.
> 
> I can follow up on this on some undefined future but right now I will
> have to switch to some other work.
> 
> If you disagree and insist on addressing skb->xdp_frame in veth within
> this patchset then I'm sorry but I will have to postpone my activities
> here.

Yeah, I understand. A lot of the skb<>XDP integration is a steaming
pile IMO, as mentioned elsewhere. I'd like to keep the core clean
tho, so if there's some corner cases in veth after your changes
I'll live. But I'm worried that the bugs in veth will make you
want to preserve the conditional in xdp_convert_skb_to_buff() :(

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Maciej Fijalkowski]

On Fri, Oct 31, 2025 at 11:49:52AM -0700, Jakub Kicinski wrote:
> On Fri, 31 Oct 2025 12:37:37 +0100 Maciej Fijalkowski wrote:
> > > > would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
> > > > patch so have to send v6 anyways.  
> > > 
> > > The veth side unfortunately needs more work than Mr Robot points out.
> > > For some reason veth tries to turn skb into an xdp_frame..  
> > 
> > That is beyond the scope of the fix that I started doing as you're
> > undermining overall XDP support in veth, IMHO.
> > 
> > I can follow up on this on some undefined future but right now I will
> > have to switch to some other work.
> > 
> > If you disagree and insist on addressing skb->xdp_frame in veth within
> > this patchset then I'm sorry but I will have to postpone my activities
> > here.
> 
> Yeah, I understand. A lot of the skb<>XDP integration is a steaming
> pile IMO, as mentioned elsewhere. I'd like to keep the core clean
> tho, so if there's some corner cases in veth after your changes
> I'll live. But I'm worried that the bugs in veth will make you
> want to preserve the conditional in xdp_convert_skb_to_buff() :(

Probably the conditional would cover these corner cases, but I am not
insisting on it. skb_pp_cow_data() is called under certain circumstances
in veth but I have never seen a case on my side where it was skipped,
mostly because of headroom size being too small.

Re: [PATCH v5 bpf 1/2] xdp: introduce xdp_convert_skb_to_buff()

[Jesper Dangaard Brouer]

On 03/11/2025 13.16, Maciej Fijalkowski wrote:
> On Fri, Oct 31, 2025 at 11:49:52AM -0700, Jakub Kicinski wrote:
>> On Fri, 31 Oct 2025 12:37:37 +0100 Maciej Fijalkowski wrote:
>>>>> would be fine for you? Plus AI reviewer has kicked me in the nuts on veth
>>>>> patch so have to send v6 anyways.
>>>>
>>>> The veth side unfortunately needs more work than Mr Robot points out.
>>>> For some reason veth tries to turn skb into an xdp_frame..
>>>
>>> That is beyond the scope of the fix that I started doing as you're
>>> undermining overall XDP support in veth, IMHO.
>>>
>>> I can follow up on this on some undefined future but right now I will
>>> have to switch to some other work.
>>>
>>> If you disagree and insist on addressing skb->xdp_frame in veth within
>>> this patchset then I'm sorry but I will have to postpone my activities
>>> here.
>>
>> Yeah, I understand. A lot of the skb<>XDP integration is a steaming
>> pile IMO, as mentioned elsewhere. I'd like to keep the core clean
>> tho, so if there's some corner cases in veth after your changes
>> I'll live. But I'm worried that the bugs in veth will make you
>> want to preserve the conditional in xdp_convert_skb_to_buff() :(
> 
> Probably the conditional would cover these corner cases, but I am not
> insisting on it. skb_pp_cow_data() is called under certain circumstances
> in veth but I have never seen a case on my side where it was skipped,
> mostly because of headroom size being too small.

This reminds me that we should change/increase veth
net_device.needed_headroom when there is an XDP prog attached.
As you said, today all SKB will basically get mem reallocated.

--Jesper