[PATCH net-next 0/2] net: Prepare skb extensions for reuse across scrubs

[PATCH net-next 0/2] net: Prepare skb extensions for reuse across scrubs

[Jakub Sitnicki]

Following Florian's suggestion from the discussion in [1].

The skb extension allocator retains the extension's offset after the
extension is deleted. This, in theory, allows skb_ext_add() to reuse the
existing storage instead of appending another copy when an extension is
added again.  However, it also means that we can't rely on the offset as an
indicator that the extension is present.

Make the extension cleanup safe, even when the offset is already allocated
and set, by making the individual extension put helpers idempotent, so that
they can be called multiple times.

This is preparatory work to support skb extension chunk area reuse after
skb scrubbing, which would be needed to persist BPF metadata skb extension
across scrubs [2].

[1] https://lore.kernel.org/all/ao9UV9S7rUkyQ8jv@strlen.de/
[2] https://lore.kernel.org/all/64365932-c765-472e-bf6c-b07c9ee25eaf@kernel.org/

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
Jakub Sitnicki (2):
      net: Make skb_ext_put_sp() idempotent
      net: Make skb_ext_put_mctp() idempotent

 net/core/skbuff.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

[PATCH net-next 1/2] net: Make skb_ext_put_sp() idempotent

[Jakub Sitnicki]

The sec_path extension can be released either when it is deleted from an
skb or when the last skb holding the extension block is freed. Make
skb_ext_put_sp() safe to call from both paths by returning when
sec_path->len is zero and clearing it after dropping the XFRM state
references.

This will let __skb_ext_put() release the sec_path without checking whether
the extension is present first, which is needed if we want to reuse skb_ext
chunks area after skb scrubbing.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/skbuff.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 966af3beed94..aa8b42c74f42 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7236,8 +7236,12 @@ static void skb_ext_put_sp(struct sec_path *sp)
 {
 	unsigned int i;
 
+	if (!sp->len)
+		return;
+
 	for (i = 0; i < sp->len; i++)
 		xfrm_state_put(sp->xvec[i]);
+	sp->len = 0;
 }
 #endif
 
@@ -7260,10 +7264,8 @@ void __skb_ext_del(struct sk_buff *skb, enum skb_ext_id id)
 #ifdef CONFIG_XFRM
 	} else if (id == SKB_EXT_SEC_PATH &&
 		   refcount_read(&ext->refcnt) == 1) {
-		struct sec_path *sp = skb_ext_get_ptr(ext, SKB_EXT_SEC_PATH);
 
-		skb_ext_put_sp(sp);
-		sp->len = 0;
+		skb_ext_put_sp(skb_ext_get_ptr(ext, SKB_EXT_SEC_PATH));
 #endif
 	}
 }

-- 
2.43.0

Re: [PATCH net-next 1/2] net: Make skb_ext_put_sp() idempotent

[Jakub Kicinski]

On Mon, 31 Aug 2026 08:27:38 +0200 Jakub Sitnicki wrote:
> +	if (!sp->len)
> +		return;

Somewhat mixed feelings about explicitly checking what is the loop
bound, anyway. But I guess you can argue we shouldn't write len?
Probably not worth a respin either way

>  	for (i = 0; i < sp->len; i++)
>  		xfrm_state_put(sp->xvec[i]);
> +	sp->len = 0;

Re: [PATCH net-next 1/2] net: Make skb_ext_put_sp() idempotent

[Jakub Sitnicki]

On Tue, Sep 01, 2026 at 05:15 PM -07, Jakub Kicinski wrote:
> On Mon, 31 Aug 2026 08:27:38 +0200 Jakub Sitnicki wrote:
>> +	if (!sp->len)
>> +		return;
>
> Somewhat mixed feelings about explicitly checking what is the loop
> bound, anyway. But I guess you can argue we shouldn't write len?
> Probably not worth a respin either way

You read my mind. Then I thought maybe I'm trying to be smarter than the
compiler? But it seems that neither clang nor gcc eliminate the write:

https://godbolt.org/z/EcWTdEYqh

>
>>  	for (i = 0; i < sp->len; i++)
>>  		xfrm_state_put(sp->xvec[i]);
>> +	sp->len = 0;

Re: [PATCH net-next 1/2] net: Make skb_ext_put_sp() idempotent

[Paolo Abeni]

On 9/2/26 7:51 AM, Jakub Sitnicki wrote:
> On Tue, Sep 01, 2026 at 05:15 PM -07, Jakub Kicinski wrote:
>> On Mon, 31 Aug 2026 08:27:38 +0200 Jakub Sitnicki wrote:
>>> +	if (!sp->len)
>>> +		return;
>>
>> Somewhat mixed feelings about explicitly checking what is the loop
>> bound, anyway. But I guess you can argue we shouldn't write len?
>> Probably not worth a respin either way
> 
> You read my mind. Then I thought maybe I'm trying to be smarter than the
> compiler? But it seems that neither clang nor gcc eliminate the write:
> 
> https://godbolt.org/z/EcWTdEYqh
I'm a little late here, but it looks like sashiko cataloged as 
pre-existing a couple of issues that on top of this series can new 
lead to memory corruption:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-skb-ext-prep-work-v1-0-ecc2a8542fd9%40cloudflare.com

Could you please have a look?

/P

Re: [PATCH net-next 1/2] net: Make skb_ext_put_sp() idempotent

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> I'm a little late here, but it looks like sashiko cataloged as 
> pre-existing a couple of issues that on top of this series can new 
> lead to memory corruption:
> 
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-skb-ext-prep-work-v1-0-ecc2a8542fd9%40cloudflare.com
> 
> Could you please have a look?

Very convoluted report.  skb->active_extensions should govern the 'do I
put internal refcounts for ipsec/mctp', not offsets[].

[PATCH net-next 2/2] net: Make skb_ext_put_mctp() idempotent

[Jakub Sitnicki]

The MCTP flow extension can be released either when it is deleted from an
skb or when the last skb holding the extension block is freed. Handle the
release in __skb_ext_del(), which is currently missing, and make
skb_ext_put_mctp() safe to call from both paths.

This will let __skb_ext_put() release the MCTP key without checking whether
the extension is present first, which is needed if we want to reuse skb_ext
chunks area after skb scrubbing.

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/skbuff.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index aa8b42c74f42..ab195b99c853 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7248,8 +7248,11 @@ static void skb_ext_put_sp(struct sec_path *sp)
 #ifdef CONFIG_MCTP_FLOWS
 static void skb_ext_put_mctp(struct mctp_flow *flow)
 {
-	if (flow->key)
-		mctp_key_unref(flow->key);
+	if (!flow->key)
+		return;
+
+	mctp_key_unref(flow->key);
+	flow->key = NULL;
 }
 #endif
 
@@ -7261,13 +7264,20 @@ void __skb_ext_del(struct sk_buff *skb, enum skb_ext_id id)
 	if (skb->active_extensions == 0) {
 		skb->extensions = NULL;
 		__skb_ext_put(ext);
-#ifdef CONFIG_XFRM
-	} else if (id == SKB_EXT_SEC_PATH &&
-		   refcount_read(&ext->refcnt) == 1) {
+		return;
+	}
+
+	if (refcount_read(&ext->refcnt) > 1)
+		return;
 
+#ifdef CONFIG_XFRM
+	if (id == SKB_EXT_SEC_PATH)
 		skb_ext_put_sp(skb_ext_get_ptr(ext, SKB_EXT_SEC_PATH));
 #endif
-	}
+#ifdef CONFIG_MCTP_FLOWS
+	if (id == SKB_EXT_MCTP)
+		skb_ext_put_mctp(skb_ext_get_ptr(ext, SKB_EXT_MCTP));
+#endif
 }
 EXPORT_SYMBOL(__skb_ext_del);
 

-- 
2.43.0

Re: [PATCH net-next 2/2] net: Make skb_ext_put_mctp() idempotent

[Jeremy Kerr]

Hi Jakub,

> The MCTP flow extension can be released either when it is deleted from an
> skb or when the last skb holding the extension block is freed. Handle the
> release in __skb_ext_del(), which is currently missing, and make
> skb_ext_put_mctp() safe to call from both paths.

Looks good. We don't use the _del() path currently, but it also makes
sense to have the two forms consistent.

> This will let __skb_ext_put() release the MCTP key without checking whether
> the extension is present first, which is needed if we want to reuse skb_ext
> chunks area after skb scrubbing.

Acked-by: Jeremy Kerr <jk@codeconstruct.com.au>

Thanks!


Jeremy

Re: [PATCH net-next 0/2] net: Prepare skb extensions for reuse across scrubs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 31 Aug 2026 08:27:37 +0200 you wrote:
> Following Florian's suggestion from the discussion in [1].
> 
> The skb extension allocator retains the extension's offset after the
> extension is deleted. This, in theory, allows skb_ext_add() to reuse the
> existing storage instead of appending another copy when an extension is
> added again.  However, it also means that we can't rely on the offset as an
> indicator that the extension is present.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: Make skb_ext_put_sp() idempotent
    https://git.kernel.org/netdev/net-next/c/d7470cba6dfa
  - [net-next,2/2] net: Make skb_ext_put_mctp() idempotent
    https://git.kernel.org/netdev/net-next/c/dbe45b209edf

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html