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

[Florian Westphal <fw@strlen.de>]

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.