[PATCH RFC net-next] net: Use fixed slots for skb extensions

[PATCH RFC net-next] net: Use fixed slots for skb extensions

[Jakub Sitnicki]

Replace the dynamic skb extension allocator (->chunks + per-object offset[]
array) with fixed per-id slots with offsets computed at compile time.

The runtime-computed offsets seem to be a leftover from the initial
posting [1], where skb_ext memory was reallocated when a new extension was
activated.

This can make extension delete-then-re-add unsafe, as pointed out by
Sashiko [2]: with bump allocation, re-adding an extension appends a second
copy, eventually overflowing the skb_ext chunks area.

While this does not happen today, because all extensions get dropped on skb
scrub, the BPF metadata skb extension work aims to preserve an extension
across skb scrubbing, which opens the door to this scenario.

[1] https://lore.kernel.org/all/20181210145006.19098-3-fw@strlen.de/
[2] https://lore.kernel.org/all/20260815081452.0DB521F00A3E@smtp.kernel.org/

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
I'm not sure if there is another easy alternative. Moving the skb ext
chunks around to close the gaps would involve making sure that nobody is
holding a pointer to them. Looking for feedback & ideas.
---
 include/linux/skbuff.h | 72 ++++++++++++++++++++++++++++++++-----------
 net/core/skbuff.c      | 83 +++++++++++++++++++-------------------------------
 2 files changed, 85 insertions(+), 70 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 95184183180f..5a5143e47b30 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -5011,45 +5011,81 @@ static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
 }
 
 #ifdef CONFIG_SKB_EXTENSIONS
-enum skb_ext_id {
+
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	SKB_EXT_BRIDGE_NF,
+#define SKB_EXT_X_BRIDGE_NF	X(SKB_EXT_BRIDGE_NF, struct nf_bridge_info)
+#else
+#define SKB_EXT_X_BRIDGE_NF
 #endif
-#ifdef CONFIG_XFRM
-	SKB_EXT_SEC_PATH,
+
+#if IS_ENABLED(CONFIG_XFRM)
+#define SKB_EXT_X_SEC_PATH	X(SKB_EXT_SEC_PATH, struct sec_path)
+#else
+#define SKB_EXT_X_SEC_PATH
 #endif
+
 #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
-	TC_SKB_EXT,
+#define SKB_EXT_X_TC		X(TC_SKB_EXT, struct tc_skb_ext)
+#else
+#define SKB_EXT_X_TC
 #endif
+
 #if IS_ENABLED(CONFIG_MPTCP)
-	SKB_EXT_MPTCP,
+#define SKB_EXT_X_MPTCP		X(SKB_EXT_MPTCP, struct mptcp_ext)
+#else
+#define SKB_EXT_X_MPTCP
 #endif
+
 #if IS_ENABLED(CONFIG_MCTP_FLOWS)
-	SKB_EXT_MCTP,
+#define SKB_EXT_X_MCTP		X(SKB_EXT_MCTP, struct mctp_flow)
+#else
+#define SKB_EXT_X_MCTP
 #endif
+
 #if IS_ENABLED(CONFIG_INET_PSP)
-	SKB_EXT_PSP,
+#define SKB_EXT_X_PSP		X(SKB_EXT_PSP, struct psp_skb_ext)
+#else
+#define SKB_EXT_X_PSP
 #endif
+
 #if IS_ENABLED(CONFIG_CAN)
-	SKB_EXT_CAN,
+#define SKB_EXT_X_CAN		X(SKB_EXT_CAN, struct can_skb_ext)
+#else
+#define SKB_EXT_X_CAN
 #endif
-	SKB_EXT_NUM, /* must be last */
+
+#define SKB_EXT_FOREACH(X)	\
+	SKB_EXT_X_BRIDGE_NF	\
+	SKB_EXT_X_SEC_PATH	\
+	SKB_EXT_X_TC		\
+	SKB_EXT_X_MPTCP		\
+	SKB_EXT_X_MCTP		\
+	SKB_EXT_X_PSP		\
+	SKB_EXT_X_CAN
+
+enum skb_ext_id {
+#define X(id, type)	id,
+	SKB_EXT_FOREACH(X)
+#undef X
+	SKB_EXT_NUM,
 };
 
+extern const u8 skb_ext_offset[SKB_EXT_NUM];
+
 /**
  *	struct skb_ext - sk_buff extensions
  *	@refcnt: 1 on allocation, deallocated on 0
- *	@offset: offset to add to @data to obtain extension address
- *	@chunks: size currently allocated, stored in SKB_EXT_ALIGN_SHIFT units
+ *	@present_extensions: bitmap of extensions stored in @data
  *	@data: start of extension data, variable sized
  *
- *	Note: offsets/lengths are stored in chunks of 8 bytes, this allows
- *	to use 'u8' types while allowing up to 2kb worth of extension data.
+ *	Each extension id occupies a fixed slot within @data, located at
+ *	skb_ext_offset[id] chunks of 8 bytes. Storing offsets/lengths
+ *	in 8-byte chunks allows 'u8' types while allowing up to 2kb worth
+ *	of extension data.
  */
 struct skb_ext {
 	refcount_t refcnt;
-	u8 offset[SKB_EXT_NUM]; /* in chunks of 8 bytes */
-	u8 chunks;		/* same */
+	u8 present_extensions;
 	char data[] __aligned(8);
 };
 
@@ -5087,7 +5123,7 @@ static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *src)
 
 static inline bool __skb_ext_exist(const struct skb_ext *ext, enum skb_ext_id i)
 {
-	return !!ext->offset[i];
+	return ext->present_extensions & (1 << i);
 }
 
 static inline bool skb_ext_exist(const struct sk_buff *skb, enum skb_ext_id id)
@@ -5106,7 +5142,7 @@ static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id)
 	if (skb_ext_exist(skb, id)) {
 		struct skb_ext *ext = skb->extensions;
 
-		return (void *)ext + (ext->offset[id] << 3);
+		return (void *)ext + (skb_ext_offset[id] << 3);
 	}
 
 	return NULL;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c82a1472a5ea..8e5db579725d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5134,47 +5134,32 @@ EXPORT_SYMBOL_GPL(skb_segment);
 #define SKB_EXT_CHUNKSIZEOF(x)	(ALIGN((sizeof(x)), SKB_EXT_ALIGN_VALUE) / SKB_EXT_ALIGN_VALUE)
 
 static const u8 skb_ext_type_len[] = {
-#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	[SKB_EXT_BRIDGE_NF] = SKB_EXT_CHUNKSIZEOF(struct nf_bridge_info),
-#endif
-#ifdef CONFIG_XFRM
-	[SKB_EXT_SEC_PATH] = SKB_EXT_CHUNKSIZEOF(struct sec_path),
-#endif
-#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
-	[TC_SKB_EXT] = SKB_EXT_CHUNKSIZEOF(struct tc_skb_ext),
-#endif
-#if IS_ENABLED(CONFIG_MPTCP)
-	[SKB_EXT_MPTCP] = SKB_EXT_CHUNKSIZEOF(struct mptcp_ext),
-#endif
-#if IS_ENABLED(CONFIG_MCTP_FLOWS)
-	[SKB_EXT_MCTP] = SKB_EXT_CHUNKSIZEOF(struct mctp_flow),
-#endif
-#if IS_ENABLED(CONFIG_INET_PSP)
-	[SKB_EXT_PSP] = SKB_EXT_CHUNKSIZEOF(struct psp_skb_ext),
-#endif
-#if IS_ENABLED(CONFIG_CAN)
-	[SKB_EXT_CAN] = SKB_EXT_CHUNKSIZEOF(struct can_skb_ext),
-#endif
+#define X(id, type)	[id] = SKB_EXT_CHUNKSIZEOF(type),
+	SKB_EXT_FOREACH(X)
+#undef X
 };
 
-static __always_inline __no_profile unsigned int skb_ext_total_length(void)
-{
-	unsigned int l = SKB_EXT_CHUNKSIZEOF(struct skb_ext);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(skb_ext_type_len); i++)
-		l += skb_ext_type_len[i];
+struct skb_ext_layout {
+	u8	header[sizeof(struct skb_ext)] __aligned(SKB_EXT_ALIGN_VALUE);
+#define X(id, type)	type f_##id __aligned(SKB_EXT_ALIGN_VALUE);
+	SKB_EXT_FOREACH(X)
+#undef X
+};
 
-	return l;
-}
+const u8 skb_ext_offset[SKB_EXT_NUM] = {
+#define X(id, type)	[id] = offsetof(struct skb_ext_layout, f_##id) / SKB_EXT_ALIGN_VALUE,
+	SKB_EXT_FOREACH(X)
+#undef X
+};
+EXPORT_SYMBOL(skb_ext_offset);
 
 static noinline void __init __no_profile skb_extensions_init(void)
 {
 	BUILD_BUG_ON(SKB_EXT_NUM > 8);
-	BUILD_BUG_ON(skb_ext_total_length() > 255);
+	BUILD_BUG_ON(sizeof(struct skb_ext_layout) > 255 * SKB_EXT_ALIGN_VALUE);
 
 	skbuff_ext_cache = kmem_cache_create("skbuff_ext_cache",
-					     SKB_EXT_ALIGN_VALUE * skb_ext_total_length(),
+					     sizeof(struct skb_ext_layout),
 					     0,
 					     SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 					     NULL);
@@ -7083,7 +7068,7 @@ EXPORT_SYMBOL(skb_condense);
 #ifdef CONFIG_SKB_EXTENSIONS
 static void *skb_ext_get_ptr(struct skb_ext *ext, enum skb_ext_id id)
 {
-	return (void *)ext + (ext->offset[id] * SKB_EXT_ALIGN_VALUE);
+	return (void *)ext + (skb_ext_offset[id] * SKB_EXT_ALIGN_VALUE);
 }
 
 /**
@@ -7100,7 +7085,7 @@ struct skb_ext *__skb_ext_alloc(gfp_t flags)
 	struct skb_ext *new = kmem_cache_alloc(skbuff_ext_cache, flags);
 
 	if (new) {
-		memset(new->offset, 0, sizeof(new->offset));
+		new->present_extensions = 0;
 		refcount_set(&new->refcnt, 1);
 	}
 
@@ -7111,6 +7096,7 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old,
 					 unsigned int old_active)
 {
 	struct skb_ext *new;
+	int i;
 
 	if (refcount_read(&old->refcnt) == 1)
 		return old;
@@ -7119,7 +7105,12 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old,
 	if (!new)
 		return NULL;
 
-	memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE);
+	memcpy(new, old, SKB_EXT_CHUNKSIZEOF(*old) * SKB_EXT_ALIGN_VALUE);
+	for (i = 0; i < SKB_EXT_NUM; i++) {
+		if (old->present_extensions & (1 << i))
+			memcpy(skb_ext_get_ptr(new, i), skb_ext_get_ptr(old, i),
+			       skb_ext_type_len[i] * SKB_EXT_ALIGN_VALUE);
+	}
 	refcount_set(&new->refcnt, 1);
 
 #ifdef CONFIG_XFRM
@@ -7156,12 +7147,8 @@ static struct skb_ext *skb_ext_maybe_cow(struct skb_ext *old,
 void *__skb_ext_set(struct sk_buff *skb, enum skb_ext_id id,
 		    struct skb_ext *ext)
 {
-	unsigned int newlen, newoff = SKB_EXT_CHUNKSIZEOF(*ext);
-
 	skb_ext_put(skb);
-	newlen = newoff + skb_ext_type_len[id];
-	ext->chunks = newlen;
-	ext->offset[id] = newoff;
+	ext->present_extensions = 1 << id;
 	skb->extensions = ext;
 	skb->active_extensions = 1 << id;
 	return skb_ext_get_ptr(ext, id);
@@ -7184,31 +7171,23 @@ EXPORT_SYMBOL_NS_GPL(__skb_ext_set, "NETDEV_INTERNAL");
  */
 void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
 {
-	struct skb_ext *new, *old = NULL;
-	unsigned int newlen, newoff;
+	struct skb_ext *new;
 
 	if (skb->active_extensions) {
-		old = skb->extensions;
-
-		new = skb_ext_maybe_cow(old, skb->active_extensions);
+		new = skb_ext_maybe_cow(skb->extensions,
+					skb->active_extensions);
 		if (!new)
 			return NULL;
 
 		if (__skb_ext_exist(new, id))
 			goto set_active;
-
-		newoff = new->chunks;
 	} else {
-		newoff = SKB_EXT_CHUNKSIZEOF(*new);
-
 		new = __skb_ext_alloc(GFP_ATOMIC);
 		if (!new)
 			return NULL;
 	}
 
-	newlen = newoff + skb_ext_type_len[id];
-	new->chunks = newlen;
-	new->offset[id] = newoff;
+	new->present_extensions |= 1 << id;
 set_active:
 	skb->slow_gro = 1;
 	skb->extensions = new;

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Florian Westphal]

Jakub Sitnicki <jakub@cloudflare.com> wrote:
> Replace the dynamic skb extension allocator (->chunks + per-object offset[]
> array) with fixed per-id slots with offsets computed at compile time.

Why is that better than

struct skb_ext {
	refcount_t refcnt;
	struct secpath s;
	struct nf_bridge_info b;
	...

?

Yes, initially this was krealloc()'d area.  But the other assumption
was that most skbs will carry no extension at all, or, in some configs
one maybe two (IPsec gateway for instance).

Thats why the first added extension is also at the beginning of the
memory blob (that needs to be accessed anyway), regardless of the ID.

I don't insist on keeping offsets[], if you feel like microbenchmarking
different use-cases to see if it makes a difference to have a fixed
memory layout feel free to explore that.

> The runtime-computed offsets seem to be a leftover from the initial
> posting [1], where skb_ext memory was reallocated when a new extension was
> activated.
> 
> This can make extension delete-then-re-add unsafe, as pointed out by
> Sashiko [2]: with bump allocation, re-adding an extension appends a second
> copy, eventually overflowing the skb_ext chunks area.

This can be solved by not zeroing the offset[] area and fixing
skb_ext_put_mctp() to NULL flow->key.

SKB_EXT_SEC_PATH is fine because it sets sp->len 0, so a
skb_ext_reset() after skb_ext_del(skb, SKB_EXT_SEC_PATH);
doesn't result in any UaF/double-refcount-puts.

skb_ext_add() already re-enables skb->active_extensions if the requested
ID already has its offset[] set.

IOW, offset[ID] = .. reserves the space, it doesn't say the extension is
still active.

> While this does not happen today, because all extensions get dropped on skb
> scrub, the BPF metadata skb extension work aims to preserve an extension
> across skb scrubbing, which opens the door to this scenario.
> 
> [1] https://lore.kernel.org/all/20181210145006.19098-3-fw@strlen.de/
> [2] https://lore.kernel.org/all/20260815081452.0DB521F00A3E@smtp.kernel.org/

Looking at [2] and the original patch:

static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
{
	struct skb_ext *old = skb->extensions;
	struct skb_ext *ext;
	int i;

	if (refcount_read(&old->refcnt) == 1) {
		skb_ext_put_each(old, keep);
		ext = old;
	} else {
		ext = skb_ext_maybe_cow(old, keep);
		if (!ext)
			return -ENOMEM;
		skb->extensions = ext;
	}

	for (i = 0; i < SKB_EXT_NUM; i++) {
		if (!(keep & (1 << i)))
			ext->offset[i] = 0;

Yes, this ext->offset[] = 0 is a problem, but its
not needed, I think. This is enough:

	}
	skb->active_extensions = keep;

(or maybe use &= so as to flag something as active
 that was never enabled).

Regarding skb_ext_maybe_cow() in above function: Why not ..


- Fix skb_ext_put_mctp to be safe against double-put.
- add skb_ext_cow, direct copy of skb_ext_maybe_cow() sans refcount check.
  skb_ext_maybe_cow() retains the refcount check and wraps skb_ext_cow().

After that:

static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
{
        struct skb_ext *old = skb->extensions;
        struct skb_ext *ext;
        int i;

        if (refcount_read(&old->refcnt) == 1) {
		skb_ext_put_each(old, keep);
		skb->active_extensions &= keep;
		return;
	}

This is where it gets interesting.  As LLM generated comment
says, we can get here with old->refcnt == 1: other CPU
changed refcount 2 -> 1 right now (after == 1 was false).

But thats not a problem, since we own a reference, the extension
area will not go away and the likelyhood of this race happening
is rather low anyway. So AFAICS this is fine:

     ext = skb_ext_cow(old, keep);
     if (!ext)
        return -ENOMEM;

Then 'skb->ext = ext' and set ->active_extensions
to the correct value (i.e. clear non-'kept' extensions).

Then call __skb_ext_put(ext).

In case we still have a clone: COW was required, the
__skb_ext_put() detached 'our' skb from the other ext blob.

Other clone will eventually call __skb_ext_put(ext) again
to release resources.

In the other case, the __skb_ext_put(ext) discarded the old memory blob
and all non-keep resources -- the kept ones had inner references (xfrm
states for instance) incremented.

Did I miss anything?  I apologize for not reviewing the initial
patchset, I promise to get to it quicker next time.

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> (or maybe use &= so as to flag something as active
>  that was never enabled).
> 
> Regarding skb_ext_maybe_cow() in above function: Why not ..
> 
> - Fix skb_ext_put_mctp to be safe against double-put.

Should be done in any case.  __skb_ext_del() also lacks
SKB_EXT_MCTP handling.

Once thats fixed, skb_ext_scrub() could look like this, no?

static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
{
	int i;

	for (i = 0; i < SKB_EXT_NUM; i++) {
		if (((i << 1) & keep) == 0)
			skb_ext_del(skb, i);
	}
}

AFAICS that handles all cases, also, doing skb_ext_del() for
all active extensions is supposed to be equal to skb_ext_reset().

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Paolo Abeni]

On 8/26/26 11:02 PM, Florian Westphal wrote:
> Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> Replace the dynamic skb extension allocator (->chunks + per-object offset[]
>> array) with fixed per-id slots with offsets computed at compile time.
> 
> Why is that better than
> 
> struct skb_ext {
> 	refcount_t refcnt;
> 	struct secpath s;
> 	struct nf_bridge_info b;
> 	...
> 
> ?

I *think* the layout above would possibly be better (with compiler's
guard around each struct definition).

> Yes, initially this was krealloc()'d area.  But the other assumption
> was that most skbs will carry no extension at all, or, in some configs
> one maybe two (IPsec gateway for instance).
> 
> Thats why the first added extension is also at the beginning of the
> memory blob (that needs to be accessed anyway), regardless of the ID.
> 
> I don't insist on keeping offsets[], if you feel like microbenchmarking
> different use-cases to see if it makes a difference to have a fixed
> memory layout feel free to explore that.

Both options for different skb ext layouts save a few bytes from the
final `struct skb_ext` size. This is IMHO quite relevant as the total
size is approaching the memory partition size (IIRC it's almost 256
bytes), and the bpf ext could make skb_ext require the next one (512).

That in turn should impact performances quite noticeably (IIRC we
observed measurable regression for bulk transfers due to similar changes
in the past), as the number of slabs required to support the same number
of in-flight packets will double, putting more pressure on the memory
allocator and possibly hitting the slab slow-path.

Still WRT optimizing skb_ext size, I think that it should be feasible to
optimize the layout proposed by Florian by taking in account that some
exts are 'mutually exclusive' i.e. on top of my head mptcp and bridge
should never be attached to the same skb, and I *guess* can_skb_ext is
mutually exclusive with most of the others.

The layout could be adapted to such constraints, and there could be
run-time checks (under DEBUG_NET) to verify such constrains at skb_add
time leveraging `present_extensions` and a static matrix describing the
mutual exclusive status for all extensions.

/P

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Oliver Hartkopp]

On 27.08.26 09:42, Paolo Abeni wrote:
> On 8/26/26 11:02 PM, Florian Westphal wrote:
>> Jakub Sitnicki <jakub@cloudflare.com> wrote:

> Still WRT optimizing skb_ext size, I think that it should be feasible to
> optimize the layout proposed by Florian by taking in account that some
> exts are 'mutually exclusive' i.e. on top of my head mptcp and bridge
> should never be attached to the same skb, and I *guess* can_skb_ext is
> mutually exclusive with most of the others.

I would say with *all* of the others. CAN skbs only interact with 
ARPHDR_CAN netdevs containing a special ml_priv reference. And 
routing/forwarding is done by a CAN specific net/can/gw.c code.

The only code outside net/can that looks into CAN skbs is:
em_canid.c  Ematch rule to match CAN frames according to their CAN IDs

But I think this is not even relevant for CONFIG_NET_TC_SKB_EXT.

> The layout could be adapted to such constraints, and there could be
> run-time checks (under DEBUG_NET) to verify such constrains at skb_add
> time leveraging `present_extensions` and a static matrix describing the
> mutual exclusive status for all extensions.

+1

Best regards,
Oliver

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Jakub Sitnicki]

On Thu, Aug 27, 2026 at 09:42 AM +02, Paolo Abeni wrote:
> On 8/26/26 11:02 PM, Florian Westphal wrote:
>> Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>> Replace the dynamic skb extension allocator (->chunks + per-object offset[]
>>> array) with fixed per-id slots with offsets computed at compile time.
>> 
>> Why is that better than
>> 
>> struct skb_ext {
>> 	refcount_t refcnt;
>> 	struct secpath s;
>> 	struct nf_bridge_info b;
>> 	...
>> 
>> ?
>
> I *think* the layout above would possibly be better (with compiler's
> guard around each struct definition).
>
>> Yes, initially this was krealloc()'d area.  But the other assumption
>> was that most skbs will carry no extension at all, or, in some configs
>> one maybe two (IPsec gateway for instance).
>> 
>> Thats why the first added extension is also at the beginning of the
>> memory blob (that needs to be accessed anyway), regardless of the ID.
>> 
>> I don't insist on keeping offsets[], if you feel like microbenchmarking
>> different use-cases to see if it makes a difference to have a fixed
>> memory layout feel free to explore that.
>
> Both options for different skb ext layouts save a few bytes from the
> final `struct skb_ext` size. This is IMHO quite relevant as the total
> size is approaching the memory partition size (IIRC it's almost 256
> bytes), and the bpf ext could make skb_ext require the next one (512).
>
> That in turn should impact performances quite noticeably (IIRC we
> observed measurable regression for bulk transfers due to similar changes
> in the past), as the number of slabs required to support the same number
> of in-flight packets will double, putting more pressure on the memory
> allocator and possibly hitting the slab slow-path.
>
> Still WRT optimizing skb_ext size, I think that it should be feasible to
> optimize the layout proposed by Florian by taking in account that some
> exts are 'mutually exclusive' i.e. on top of my head mptcp and bridge
> should never be attached to the same skb, and I *guess* can_skb_ext is
> mutually exclusive with most of the others.

Right, we could have unions for the mutually exclusive options.

> The layout could be adapted to such constraints, and there could be
> run-time checks (under DEBUG_NET) to verify such constrains at skb_add
> time leveraging `present_extensions` and a static matrix describing the
> mutual exclusive status for all extensions.

Having checks like that that sounds like the right first step that can
be done independently of skb_ext layout changes. I can give that a shot.

Thanks for feedback.

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> > Yes, initially this was krealloc()'d area.  But the other assumption
> > was that most skbs will carry no extension at all, or, in some configs
> > one maybe two (IPsec gateway for instance).
> > 
> > Thats why the first added extension is also at the beginning of the
> > memory blob (that needs to be accessed anyway), regardless of the ID.
> > 
> > I don't insist on keeping offsets[], if you feel like microbenchmarking
> > different use-cases to see if it makes a difference to have a fixed
> > memory layout feel free to explore that.
> 
> Both options for different skb ext layouts save a few bytes from the
> final `struct skb_ext` size. This is IMHO quite relevant as the total
> size is approaching the memory partition size (IIRC it's almost 256
> bytes), and the bpf ext could make skb_ext require the next one (512).

We're close to the 8-extensions limit (u8 id), the total blob size can
be 2k, as offset is in '>> 8' units.

> Still WRT optimizing skb_ext size, I think that it should be feasible to
> optimize the layout proposed by Florian by taking in account that some
> exts are 'mutually exclusive' i.e. on top of my head mptcp and bridge
> should never be attached to the same skb, and I *guess* can_skb_ext is
> mutually exclusive with most of the others.

Yes, there are extensions that are expected to be mutually exclusive,

> The layout could be adapted to such constraints, and there could be
> run-time checks (under DEBUG_NET) to verify such constrains at skb_add
> time leveraging `present_extensions` and a static matrix describing the
> mutual exclusive status for all extensions.

Yes, simple union this, union that, is too fragile, skb_ext_add will
have to check and reject as you say.

OTOH, I think this is orthogonal to what Jakub S. needs for the BPF
extension.

Re: [PATCH RFC net-next] net: Use fixed slots for skb extensions

[Jakub Sitnicki]

On Wed, Aug 26, 2026 at 11:02 PM +02, Florian Westphal wrote:
> Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> Replace the dynamic skb extension allocator (->chunks + per-object offset[]
>> array) with fixed per-id slots with offsets computed at compile time.
>
> Why is that better than
>
> struct skb_ext {
> 	refcount_t refcnt;
> 	struct secpath s;
> 	struct nf_bridge_info b;
> 	...
>
> ?

Not better, just different. Header depenendencies get in the way -
xfrm.h defines struct sec_path but it also includes skbuff.h. Nothing
that can't be dealt with. Perhaps it'd be simpliest to move skb_ext to
its own header? I will have to give it a try.

> Yes, initially this was krealloc()'d area.  But the other assumption
> was that most skbs will carry no extension at all, or, in some configs
> one maybe two (IPsec gateway for instance).
>
> Thats why the first added extension is also at the beginning of the
> memory blob (that needs to be accessed anyway), regardless of the ID.
>
> I don't insist on keeping offsets[], if you feel like microbenchmarking
> different use-cases to see if it makes a difference to have a fixed
> memory layout feel free to explore that.
>
>> The runtime-computed offsets seem to be a leftover from the initial
>> posting [1], where skb_ext memory was reallocated when a new extension was
>> activated.
>> 
>> This can make extension delete-then-re-add unsafe, as pointed out by
>> Sashiko [2]: with bump allocation, re-adding an extension appends a second
>> copy, eventually overflowing the skb_ext chunks area.
>
> This can be solved by not zeroing the offset[] area and fixing
> skb_ext_put_mctp() to NULL flow->key.
>
> SKB_EXT_SEC_PATH is fine because it sets sp->len 0, so a
> skb_ext_reset() after skb_ext_del(skb, SKB_EXT_SEC_PATH);
> doesn't result in any UaF/double-refcount-puts.
>
> skb_ext_add() already re-enables skb->active_extensions if the requested
> ID already has its offset[] set.
>
> IOW, offset[ID] = .. reserves the space, it doesn't say the extension is
> still active.

That's a good point.
Didn't occur to me that we could think about offset like that.

>
>> While this does not happen today, because all extensions get dropped on skb
>> scrub, the BPF metadata skb extension work aims to preserve an extension
>> across skb scrubbing, which opens the door to this scenario.
>> 
>> [1] https://lore.kernel.org/all/20181210145006.19098-3-fw@strlen.de/
>> [2] https://lore.kernel.org/all/20260815081452.0DB521F00A3E@smtp.kernel.org/
>
> Looking at [2] and the original patch:
>
> static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
> {
> 	struct skb_ext *old = skb->extensions;
> 	struct skb_ext *ext;
> 	int i;
>
> 	if (refcount_read(&old->refcnt) == 1) {
> 		skb_ext_put_each(old, keep);
> 		ext = old;
> 	} else {
> 		ext = skb_ext_maybe_cow(old, keep);
> 		if (!ext)
> 			return -ENOMEM;
> 		skb->extensions = ext;
> 	}
>
> 	for (i = 0; i < SKB_EXT_NUM; i++) {
> 		if (!(keep & (1 << i)))
> 			ext->offset[i] = 0;
>
> Yes, this ext->offset[] = 0 is a problem, but its
> not needed, I think. This is enough:
>
> 	}
> 	skb->active_extensions = keep;
>
> (or maybe use &= so as to flag something as active
>  that was never enabled).
>
> Regarding skb_ext_maybe_cow() in above function: Why not ..
>
>
> - Fix skb_ext_put_mctp to be safe against double-put.
> - add skb_ext_cow, direct copy of skb_ext_maybe_cow() sans refcount check.
>   skb_ext_maybe_cow() retains the refcount check and wraps skb_ext_cow().
>
> After that:
>
> static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
> {
>         struct skb_ext *old = skb->extensions;
>         struct skb_ext *ext;
>         int i;
>
>         if (refcount_read(&old->refcnt) == 1) {
> 		skb_ext_put_each(old, keep);
> 		skb->active_extensions &= keep;
> 		return;
> 	}
>
> This is where it gets interesting.  As LLM generated comment
> says, we can get here with old->refcnt == 1: other CPU
> changed refcount 2 -> 1 right now (after == 1 was false).
>
> But thats not a problem, since we own a reference, the extension
> area will not go away and the likelyhood of this race happening
> is rather low anyway. So AFAICS this is fine:
>
>      ext = skb_ext_cow(old, keep);
>      if (!ext)
>         return -ENOMEM;
>
> Then 'skb->ext = ext' and set ->active_extensions
> to the correct value (i.e. clear non-'kept' extensions).
>
> Then call __skb_ext_put(ext).
>
> In case we still have a clone: COW was required, the
> __skb_ext_put() detached 'our' skb from the other ext blob.
>
> Other clone will eventually call __skb_ext_put(ext) again
> to release resources.
>
> In the other case, the __skb_ext_put(ext) discarded the old memory blob
> and all non-keep resources -- the kept ones had inner references (xfrm
> states for instance) incremented.
>
> Did I miss anything?  I apologize for not reviewing the initial
> patchset, I promise to get to it quicker next time.

Sounds sane to me. Or at least I can't poke any holes in it.

Thanks for sharing your thoughts. I really appreciate the input.