Netdev List
 help / color / mirror / Atom feed
From: Fabricio Gava <fabriciogava@gmail.com>
To: Bruno Xavier <bfxavier@gmail.com>
Cc: Florian Schauer <florian@schauer.to>,
	netdev@vger.kernel.org, kuba@kernel.org, edumazet@google.com,
	pabeni@redhat.com, davem@davemloft.net, lorenzo@kernel.org,
	hawk@kernel.org, ilias.apalodimas@linaro.org,
	bpf@vger.kernel.org
Subject: Re: [PATCH] net: skbuff: keep the page_pool fragment offset aligned in skb_pp_cow_data()
Date: Sun, 30 Aug 2026 18:23:27 -0300	[thread overview]
Message-ID: <20260830212331.546227-1-fabriciogava@gmail.com> (raw)
In-Reply-To: <20260827122926.31123-1-bfxavier@gmail.com>

On Thu, Aug 27, 2026 at 02:29:17PM +0200, Bruno Xavier wrote:
>     The alternative is to have the page_pool frag allocator guarantee a
>     minimum alignment itself:
> [...]
>     That covers every caller of page_pool_dev_alloc*() instead of just
>     this one, but it changes a generic allocator and would carry a 2021
>     Fixes tag, so I kept the fix in the caller. Happy to send that version
>     instead if you prefer it.

That version has already been sent, by Florian Schauer, the day before
this patch -- "page_pool: keep frag_offset aligned for odd-sized requests",
now at v2:

https://lore.kernel.org/netdev/20260828060822.2628276-1-florian@schauer.to/

It is the same change you sketch, already at its second revision: the v2
changelog credits Eric Dumazet for __alignof__(struct skb_shared_info) in
place of sizeof(long).  Neither thread references the other and both are
still in state "new", so I am replying to both to connect them.

One thing that may bear on the choice between the two: skb_pp_cow_data() is
not the only user of the per-cpu system_page_pool that requests a raw
length.
xdp_copy_frags_from_zc() does the same, net/core/xdp.c:700-705:

	const skb_frag_t *frag = &xinfo->frags[i];
	u32 len = skb_frag_size(frag);
	u32 offset, truesize = len;
	struct page *page;

	page = page_pool_dev_alloc(pp, &offset, &truesize);

Its caller xdp_build_skb_from_zc() takes pp from this_cpu_read(
system_page_pool.pool) at xdp.c:753, and then feeds
page_pool_dev_alloc_va() at xdp.c:754 into napi_build_skb() at xdp.c:758 --
so that path both leaves odd frag_offsets behind and consumes the head
allocations that follow them, on the very pool your patch is protecting.
With the fix in the caller, that door stays open.

I also measured the call-site attribution here.  Over ~75 s of that
workload and some 8.6 million fragment requests reaching
page_pool_alloc_frag_netmem(), the probe saw none originating outside
skb_pp_cow_data() -- but that machine drives neither the zero-copy path nor
a page_pool-backed NIC driver, so no other producer was exercised.  So the
measurement says your patch would be sufficient for that workload.  It does
not show that no other producer exists, and xdp_copy_frags_from_zc() draws
from the same per-cpu pool your patch protects.

One more thing about the scope your Fixes: tag implies.  skb_pp_cow_data()
is also used by veth, at drivers/net/veth.c:762, so the fragment loop is
reachable outside the generic-XDP path that e6d5dbdd20aa added.  That does
not make the tag wrong for where the code came from, but the blast radius
is wider than "xdp running in generic mode" reads.

For what it is worth, I hit exactly your symptom independently, on
different hardware: Fedora 44 with kernels 7.1.8 / 7.1.9 / 7.1.10 on an
i5-13420H, NetBird attaching a generic XDP program to lo, eight panics at
skb_clone+0x159 -- six from raw_v4_input(), two from ipv6_raw_deliver().
Measurements of the three links in the chain are in my reply on Florian's
v2 thread rather than repeated here.

Thanks,
Fabricio Gava

  reply	other threads:[~2026-08-30 21:23 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-27 12:29 [PATCH] net: skbuff: keep the page_pool fragment offset aligned in skb_pp_cow_data() Bruno Xavier
2026-08-30 21:23 ` Fabricio Gava [this message]
2026-08-30 22:11   ` Bruno Xavier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260830212331.546227-1-fabriciogava@gmail.com \
    --to=fabriciogava@gmail.com \
    --cc=bfxavier@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=florian@schauer.to \
    --cc=hawk@kernel.org \
    --cc=ilias.apalodimas@linaro.org \
    --cc=kuba@kernel.org \
    --cc=lorenzo@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox