[PATCH 0/12] pull request (net): ipsec 2026-09-07

[PATCH 0/12] pull request (net): ipsec 2026-09-07

[Steffen Klassert]

1) xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()
   Add the up-front nr_frags guard iptfs_skb_add_frags() already has,
   so an out-of-range offset can't walk past the on-stack frags[] array.

2) xfrm: serialize state GC with device state flush
   Serialize xfrm_state destruction against the deferred-device pass
   with a dedicated mutex, since the device GC list doesn't hold a state
   reference and the two paths could free the same state.

3) xfrm: add missing RCU read lock in xfrm_send_migrate_state()
   Hold the RCU read lock around xfrm_nlmsg_multicast() so the
   rcu_dereference() of net->xfrm.nlsk doesn't warn.

4) xfrm: iptfs: fix runt reassembly panic from short inner tot_len
   Require the runt length to cover at least the minimum IP header,
   so a tot_len in [6, 19] (IPv4) can't write past the declared length
   and trip skb_over_panic().

5) ipv6: xfrm: use full sockets in local error paths
   Use skb_to_full_sk() in xfrm6_local_rxpmtu() and xfrm6_local_error()
   and bail out without a full socket, so a TCP_NEW_SYN_RECV request_sock
   isn't miscast as a full inet/IPv6 socket.

6) xfrm: fix compat ALLOCSPI request use-after-free
   Drop the redundant alloc_compat() in xfrm_alloc_userspi() so the
   compat translator no longer reads past the payload and publishes a
   child a multicast clone can still see after xfrm_user_rcv_msg() frees.

7) xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()
   Force the dst before queuing, hold dev across the workqueue deferral,
   and take rcu_read_lock() around the finish() loop, so transport-mode
   reinjection doesn't deref non-refcounted dst/dev under workqueue.

8) xfrm: use hlist_del_init_rcu for state_cache and state_cache_input
   Switch to hlist_del_init_rcu() so a second __xfrm_state_delete() is
   a no-op instead of writing through LIST_POISON2, closing the UAFs.

9) esp: downgrade zerocopy managed frags before mutating skb frags
   Call skb_zcopy_downgrade_managed() before ESP rewrites the skb frag
   array, so per-frag unrefs in esp_ssg_unref() and skb_release_data()
   stay balanced for ubuf-owned managed frags.

10) xfrm: hold net_device reference under RCU in bundle creation
    Read dst->dev via dst_dev_rcu() and keep RCU active through
    xfrm_fill_dst(), so a concurrent RTM_DELLINK can't free dev
    under bundle creation.

11) xfrm: save input state data before secpath resets
    Save the state protocol on the stack while it's still valid and
    use the saved address family for transport_finish(), so post-reset
    dereferences (VTI, XFRM if, MAX_DEPTH error) can't UAF the state.

12) net: xfrm: reject unrepresentable espintcp transport headers
    Use the careful transport-header helper and drop the skb through
    the XFRM error path when the offset can't be represented, instead
    of silently truncating it.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit 4e9442ce551ebd84b52ad649df721e2dc28af95a:

  xfrm: bound nat keepalive state collection (2026-08-18 07:35:01 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git tags/ipsec-2026-09-07

for you to fetch changes up to 96f01b53c2d05e003b040892256de54a586e8529:

  net: xfrm: reject unrepresentable espintcp transport headers (2026-09-01 12:24:11 +0200)

----------------------------------------------------------------
ipsec-2026-09-07

----------------------------------------------------------------
Aleksandr Nogikh (1):
      xfrm: add missing RCU read lock in xfrm_send_migrate_state()

Cen Zhang (Microsoft Security FORGE Labs) (1):
      xfrm: hold net_device reference under RCU in bundle creation

Chengfeng Ye (1):
      xfrm: serialize state GC with device state flush

Eric Dumazet (1):
      xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

Henry Martin (1):
      xfrm: iptfs: fix runt reassembly panic from short inner tot_len

Kyle Zeng (1):
      xfrm: fix compat ALLOCSPI request use-after-free

Maher Azzouzi (1):
      esp: downgrade zerocopy managed frags before mutating skb frags

Roshan Kumar (1):
      xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

Siwei Zhang (1):
      xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

Wyatt Feng (1):
      net: xfrm: reject unrepresentable espintcp transport headers

Zhiling Zou (2):
      ipv6: xfrm: use full sockets in local error paths
      xfrm: save input state data before secpath resets

 net/ipv4/esp4.c         |  6 ++++++
 net/ipv6/esp6.c         |  6 ++++++
 net/ipv6/xfrm6_output.c | 10 ++++++++--
 net/xfrm/espintcp.c     |  6 +++++-
 net/xfrm/xfrm_input.c   | 22 +++++++++++++++++++---
 net/xfrm/xfrm_iptfs.c   | 12 ++++++++++--
 net/xfrm/xfrm_policy.c  | 20 +++++++++++++++-----
 net/xfrm/xfrm_state.c   |  9 +++++++--
 net/xfrm/xfrm_user.c    | 18 +++++-------------
 9 files changed, 81 insertions(+), 28 deletions(-)

[PATCH 01/12] xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

[Steffen Klassert]

From: Roshan Kumar <roshaen09@gmail.com>

iptfs_skb_reset_frag_walk() advances to the fragment containing @offset
with an unbounded loop:

	while (offset >= walk->past + walk->frags[walk->fragi].len)
		walk->past += walk->frags[walk->fragi++].len;

walk->fragi is advanced and walk->frags[walk->fragi] is dereferenced
without ever checking fragi against walk->nr_frags. When the requested
offset is at or beyond the total length spanned by the walk's fragments,
fragi runs past nr_frags and off the end of the fixed-size on-stack
frags[MAX_SKB_FRAGS + 1] array, reading out-of-bounds stack memory.

The two callers behave differently: iptfs_skb_add_frags() already guards
against this with

	if (!walk->nr_frags ||
	    offset >= walk->total + walk->initial_offset)
		return len;

but iptfs_skb_can_add_frags() has no such guard and calls
iptfs_skb_reset_frag_walk() unconditionally, so it performs the
out-of-range walk. Its own "fragi < walk->nr_frags" bound check runs only
afterwards, too late to prevent the read.

This is reachable from the receive path: a crafted IP-TFS (AGGFRAG)
payload delivered to an IPTFS SA drives iptfs_reassem_cont() ->
iptfs_skb_can_add_frags() with an offset past the fragment total, e.g.:

  BUG: KASAN: stack-out-of-bounds in iptfs_skb_reset_frag_walk+0x235/0x250
  Read of size 4 at addr ffff888008ad7210 by task repro/345
   iptfs_skb_reset_frag_walk+0x235/0x250 net/xfrm/xfrm_iptfs.c:392
   iptfs_skb_can_add_frags+0x155/0x310  net/xfrm/xfrm_iptfs.c:420
   iptfs_reassem_cont+0xcf8/0x1140      net/xfrm/xfrm_iptfs.c:902
   iptfs_input_ordered+0x552/0x670      net/xfrm/xfrm_iptfs.c:1280
   iptfs_input+0x3d6/0xde0              net/xfrm/xfrm_iptfs.c:1741
   xfrm_input+0x282f/0x6140             net/xfrm/xfrm_input.c:700
   xfrm4_esp_rcv+0x93/0x120             net/ipv4/xfrm4_protocol.c:104
   ip_rcv+0x278/0x2d0                   net/ipv4/ip_input.c:612

Give iptfs_skb_can_add_frags() the same up-front guard that
iptfs_skb_add_frags() already has, so the walk is never entered with an
out-of-range offset. When it triggers, the caller falls back to the
existing linearize-and-copy path, which is safe.

Fixes: 5f2b6a909574 ("xfrm: iptfs: add skb-fragment sharing code")
Reported-by: Roshan Kumar <roshaen09@gmail.com>
Signed-off-by: Roshan Kumar <roshaen09@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_iptfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 597aedeac26e..2ce15c472cc4 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -416,6 +416,14 @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
 	if (skb_has_frag_list(skb) || skb->pp_recycle != walk->pp_recycle)
 		return false;
 
+	/* Reject an @offset that is at or beyond the end of the walk's data
+	 * before calling iptfs_skb_reset_frag_walk(), whose fragment-advance
+	 * loop is otherwise unbounded and would index past walk->frags[].
+	 * This mirrors the guard already present in iptfs_skb_add_frags().
+	 */
+	if (!walk->nr_frags || offset >= walk->total + walk->initial_offset)
+		return false;
+
 	/* Make offset relative to current frag after setting that */
 	offset = iptfs_skb_reset_frag_walk(walk, offset);
 
-- 
2.43.0

Re: [PATCH 01/12] xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

This adds an up-front range check to iptfs_skb_can_add_frags() so that
iptfs_skb_reset_frag_walk() is never entered with an offset at or beyond
the end of the walk's fragment data, where its unbounded advance loop
would read past walk->frags[]. The guard mirrors the one already present
in iptfs_skb_add_frags().

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 597aedeac26eb..2ce15c472cc4d 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -416,6 +416,14 @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
>  	if (skb_has_frag_list(skb) || skb->pp_recycle != walk->pp_recycle)
>  		return false;
>  
> +	/* Reject an @offset that is at or beyond the end of the walk's data
> +	 * before calling iptfs_skb_reset_frag_walk(), whose fragment-advance
> +	 * loop is otherwise unbounded and would index past walk->frags[].
> +	 * This mirrors the guard already present in iptfs_skb_add_frags().
> +	 */
> +	if (!walk->nr_frags || offset >= walk->total + walk->initial_offset)
> +		return false;
> +

[Severity: Medium]

Should this return true (or be conditioned on len) instead of false?

In the iptfs_reassem_cont() path the walk is prepared and then queried
with the same offset:

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
	...
	fraglen = min(blkoff, remaining);
	copylen = min(fraglen, ipremain);

	if (!skb_has_frag_list(skb) && !skb_has_frag_list(newskb) &&
	    (skb->head_frag || skb->len == skb->data_len) &&
	    skb->pp_recycle == newskb->pp_recycle) {
		fragwalk = &_fragwalk;
		iptfs_skb_prepare_frag_walk(skb, data, fragwalk);
	}

	if (fragwalk &&
	    iptfs_skb_can_add_frags(newskb, fragwalk, data, copylen)) {
	...
}

Since offset == walk->initial_offset == data here, the new clause reduces
to walk->total == 0, and iptfs_skb_prepare_frag_walk() sums exactly
skb->len - data, so it only fires when data == skb->len, i.e. when
remaining == 0 and therefore copylen == 0 as well.

For that case the guard now sends control into the else branch:

	} else {
		if (skb_linearize(newskb)) {
			...
		}

		/* copy fragment data into newskb */
		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
				      copylen)) {
			XFRM_INC_STATS(xs_net(xtfs->x),
				       LINUX_MIB_XFRMINBUFFERERROR);
			goto abandon;
		}
	}

with copylen == 0 and data == skb->len.  skb_copy_seq_read() calls
skb_seq_read() before it looks at len:

net/core/skbuff.c:skb_copy_seq_read() {
	for (;;) {
		sqlen = skb_seq_read(offset, &data, st);
		if (sqlen == 0)
			return -EINVAL;
		if (sqlen >= len) {
	...
}

and skb_seq_read() returns 0 once abs_offset >= st->upper_offset, so the
zero-length copy returns -EINVAL and iptfs_reassem_cont() takes the
abandon label, where iptfs_reassem_abort() frees xtfs->ra_newskb and
destroys the in-progress inner packet reassembly.

Before this patch the same input took the sharing branch: can_add_frags()
fell through its "while (len && fragi < walk->nr_frags)" loop with len == 0
and returned true, iptfs_skb_add_frags() returned immediately via its own
!walk->nr_frags check, and reassembly continued via
"if (copylen < ipremain) xtfs->ra_wantseq++;".

Two inputs from a peer on an IPTFS SA appear to reach this: an AGGFRAG
payload carrying only the basic/CC header with block_offset >= ipremain
while a reassembly is in progress, and the "newskb->len < iphlen" block
copying exactly iphremain == remaining bytes so that "data += copylen"
leaves data == skb->len.

Would a guard on len, for example returning true when len == 0 and only
rejecting when there is data left to add, close the stack out-of-bounds
read without changing the reassembly outcome?

The other caller, iptfs_pskb_add_frags() in __input_process_payload(), is
only reached when capturelen > iphlen and remaining >= iphlen, so it does
not appear to be able to hit the new guard.

For reference, this behaviour is unchanged through the rest of the series:
the only later change to net/xfrm/xfrm_iptfs.c is the ra_runt iplen check
in "xfrm: iptfs: fix runt reassembly panic from short inner tot_len", and
iptfs_skb_can_add_frags() is the same at the end of the series.

Re: [PATCH 01/12] xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

[Steffen Klassert]

Ccing Author of the patch and author of IPTFS.

On Tue, Sep 08, 2026 at 03:48:02PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()
> 
> This adds an up-front range check to iptfs_skb_can_add_frags() so that
> iptfs_skb_reset_frag_walk() is never entered with an offset at or beyond
> the end of the walk's fragment data, where its unbounded advance loop
> would read past walk->frags[]. The guard mirrors the one already present
> in iptfs_skb_add_frags().
> 
> > diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> > index 597aedeac26eb..2ce15c472cc4d 100644
> > --- a/net/xfrm/xfrm_iptfs.c
> > +++ b/net/xfrm/xfrm_iptfs.c
> > @@ -416,6 +416,14 @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
> >  	if (skb_has_frag_list(skb) || skb->pp_recycle != walk->pp_recycle)
> >  		return false;
> >  
> > +	/* Reject an @offset that is at or beyond the end of the walk's data
> > +	 * before calling iptfs_skb_reset_frag_walk(), whose fragment-advance
> > +	 * loop is otherwise unbounded and would index past walk->frags[].
> > +	 * This mirrors the guard already present in iptfs_skb_add_frags().
> > +	 */
> > +	if (!walk->nr_frags || offset >= walk->total + walk->initial_offset)
> > +		return false;
> > +
> 
> [Severity: Medium]
> 
> Should this return true (or be conditioned on len) instead of false?
> 
> In the iptfs_reassem_cont() path the walk is prepared and then queried
> with the same offset:
> 
> net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
> 	...
> 	fraglen = min(blkoff, remaining);
> 	copylen = min(fraglen, ipremain);
> 
> 	if (!skb_has_frag_list(skb) && !skb_has_frag_list(newskb) &&
> 	    (skb->head_frag || skb->len == skb->data_len) &&
> 	    skb->pp_recycle == newskb->pp_recycle) {
> 		fragwalk = &_fragwalk;
> 		iptfs_skb_prepare_frag_walk(skb, data, fragwalk);
> 	}
> 
> 	if (fragwalk &&
> 	    iptfs_skb_can_add_frags(newskb, fragwalk, data, copylen)) {
> 	...
> }
> 
> Since offset == walk->initial_offset == data here, the new clause reduces
> to walk->total == 0, and iptfs_skb_prepare_frag_walk() sums exactly
> skb->len - data, so it only fires when data == skb->len, i.e. when
> remaining == 0 and therefore copylen == 0 as well.
> 
> For that case the guard now sends control into the else branch:
> 
> 	} else {
> 		if (skb_linearize(newskb)) {
> 			...
> 		}
> 
> 		/* copy fragment data into newskb */
> 		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
> 				      copylen)) {
> 			XFRM_INC_STATS(xs_net(xtfs->x),
> 				       LINUX_MIB_XFRMINBUFFERERROR);
> 			goto abandon;
> 		}
> 	}
> 
> with copylen == 0 and data == skb->len.  skb_copy_seq_read() calls
> skb_seq_read() before it looks at len:
> 
> net/core/skbuff.c:skb_copy_seq_read() {
> 	for (;;) {
> 		sqlen = skb_seq_read(offset, &data, st);
> 		if (sqlen == 0)
> 			return -EINVAL;
> 		if (sqlen >= len) {
> 	...
> }
> 
> and skb_seq_read() returns 0 once abs_offset >= st->upper_offset, so the
> zero-length copy returns -EINVAL and iptfs_reassem_cont() takes the
> abandon label, where iptfs_reassem_abort() frees xtfs->ra_newskb and
> destroys the in-progress inner packet reassembly.
> 
> Before this patch the same input took the sharing branch: can_add_frags()
> fell through its "while (len && fragi < walk->nr_frags)" loop with len == 0
> and returned true, iptfs_skb_add_frags() returned immediately via its own
> !walk->nr_frags check, and reassembly continued via
> "if (copylen < ipremain) xtfs->ra_wantseq++;".
> 
> Two inputs from a peer on an IPTFS SA appear to reach this: an AGGFRAG
> payload carrying only the basic/CC header with block_offset >= ipremain
> while a reassembly is in progress, and the "newskb->len < iphlen" block
> copying exactly iphremain == remaining bytes so that "data += copylen"
> leaves data == skb->len.
> 
> Would a guard on len, for example returning true when len == 0 and only
> rejecting when there is data left to add, close the stack out-of-bounds
> read without changing the reassembly outcome?

This might make the IPTFS reassembly inefficient. But as my knowledge
of IPTFS is limited and I don't get reviews from the original author,
I plan to keep this fix.

Re: [PATCH 01/12] xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

[Roshan Kumar]

Hi Steffen,

I had a look at the review and it is right that the guard changes the
len == 0 outcome, with one nuance worth splitting out.

The sharing branch needs a prepared frag walk, so the guard can only
change behavior for skbs that are frag walk eligible (head_frag set, or
all data in frags). For those, before the change
iptfs_skb_can_add_frags() fell through the "while (len && fragi <
walk->nr_frags)" loop and returned true, iptfs_skb_add_frags()
returned immediately on its own " !walk->nr_frags || offset out of
range" check, and reassembly continued with ra_wantseq++. With the
guard the same input returns false, takes the copy branch, and
skb_copy_seq_read(..., 0) returns EINVAL, so the in progress
reassembly is dropped.

For linear skbs the frag walk stays NULL and this corner dropped
reassembly before the change too: the copy branch runs either way and
skb_seq_read at the end of the buffer fails the same way. I reproduced
that part live on v7.3-rc3 today: a partial inner packet followed by
an AGGFRAG basic header only block with block_offset 0xffff kills the
in progress reassembly with and without the fix, so that part already
existed rather than being something the guard introduces.

The review's suggestion closes the gap for the frag walk case: return
true when len == 0, before the offset check. The dangerous walk in
iptfs_skb_reset_frag_walk() is skipped entirely for len == 0, and
iptfs_skb_add_frags() keeps its own bounds check for len > 0, so the
out of bounds read cannot come back this way. The reassembly outcome
stays identical to before the fix for head frag skbs, so there is no
efficiency cost either.

Something like this on top of the patch:

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
        if (skb_has_frag_list(skb) || skb->pp_recycle != walk->pp_recycle)
                return false;

+       /* len == 0: nothing to add, proceed as before the fix. */
+       if (!len)
+               return true;
+
        /* Reject an @offset that is at or beyond the end of the walk's data
         * before calling iptfs_skb_reset_frag_walk(), whose fragment-advance
         * loop is otherwise unbounded and would index past walk->frags[].
         * This mirrors the guard already present in iptfs_skb_add_frags().
         */
        if (!walk->nr_frags || offset >= walk->total + walk->initial_offset)
                return false;

The len == 0 drop for linear skbs existed before this change; I am
happy to look at that separately once this series lands.

Thanks for forwarding the review.

Roshan

[PATCH 02/12] xfrm: serialize state GC with device state flush

[Steffen Klassert]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

The deferred-device pass in xfrm_dev_state_flush() finds states under
xfrm_state_dev_gc_lock, but drops the lock before calling
xfrm_dev_state_free() because the driver callback may sleep.  The device
GC list does not hold an xfrm_state reference, so the state GC worker can
destroy the same state concurrently.

The race can proceed as follows:

  CPU 0                               CPU 1
  find x on the device GC list
  drop xfrm_state_dev_gc_lock
  read x->xso.dev
                                      xfrm_state_gc_destroy(x)
                                      xfrm_dev_state_free(x)
                                      xfrm_state_free(x)
  continue xfrm_dev_state_free(x)

Both paths can invoke the driver callback and drop the device reference.
CPU 0 can also access the xfrm_state after CPU 1 has freed it.

KASAN reported:

  BUG: KASAN: slab-use-after-free in xfrm_dev_state_free+0x24c/0x2a0
  Read of size 8 at addr ffff88810bbaa960 by task poc/102

  Call Trace:
   xfrm_dev_state_free+0x24c/0x2a0
   xfrm_dev_state_flush+0x353/0x400
   xfrm_dev_event+0x26d/0x3a0
   notifier_call_chain+0xc0/0x280
   __dev_notify_flags+0x169/0x250
   netif_change_flags+0xe7/0x160
   dev_change_flags+0x96/0x220
   devinet_ioctl+0x7f4/0x1880

  Allocated by task 87:
   xfrm_state_alloc+0x1e/0x5c0
   xfrm_add_sa+0xe7f/0x5820
   xfrm_user_rcv_msg+0x4f3/0x940

  Freed by task 57:
   kmem_cache_free+0xcb/0x3d0
   xfrm_state_gc_task+0x4a8/0x650
   process_one_work+0x63a/0x1070

Serialize xfrm_state destruction against the deferred-device pass with a
mutex.  Keep xfrm_state_dev_gc_lock limited to list operations and retain
the existing callback and device-reference release ordering.

Fixes: 07b87f9eea0c ("xfrm: Fix unregister netdevice hang on hardware offload.")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 36a4f6793ede..de097bba803b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -226,6 +226,7 @@ static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];
 
 static DEFINE_SPINLOCK(xfrm_state_gc_lock);
 static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
+static DEFINE_MUTEX(xfrm_state_gc_mutex);
 
 int __xfrm_state_delete(struct xfrm_state *x);
 
@@ -632,8 +633,10 @@ static void xfrm_state_gc_task(struct work_struct *work)
 
 	synchronize_rcu();
 
+	mutex_lock(&xfrm_state_gc_mutex);
 	hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
 		xfrm_state_gc_destroy(x);
+	mutex_unlock(&xfrm_state_gc_mutex);
 }
 
 static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
@@ -1000,6 +1003,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 out:
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
+	mutex_lock(&xfrm_state_gc_mutex);
 	spin_lock_bh(&xfrm_state_dev_gc_lock);
 restart_gc:
 	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
@@ -1014,6 +1018,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 
 	}
 	spin_unlock_bh(&xfrm_state_dev_gc_lock);
+	mutex_unlock(&xfrm_state_gc_mutex);
 
 	xfrm_flush_gc();
 
-- 
2.43.0

Re: [PATCH 02/12] xfrm: serialize state GC with device state flush

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: serialize state GC with device state flush

This adds a global mutex, xfrm_state_gc_mutex, taken by the state GC worker
around xfrm_state_gc_destroy() and by the deferred-device pass of
xfrm_dev_state_flush(), so the two paths no longer run xfrm_dev_state_free()
on the same state concurrently.  The KASAN slab-use-after-free reported in
xfrm_dev_state_free() from xfrm_dev_state_flush() is addressed by that
serialization.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 36a4f6793edef..de097bba803b7 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -226,6 +226,7 @@ static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];
>  
>  static DEFINE_SPINLOCK(xfrm_state_gc_lock);
>  static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
> +static DEFINE_MUTEX(xfrm_state_gc_mutex);
>  
>  int __xfrm_state_delete(struct xfrm_state *x);
>  
> @@ -632,8 +633,10 @@ static void xfrm_state_gc_task(struct work_struct *work)
>  
>  	synchronize_rcu();
>  
> +	mutex_lock(&xfrm_state_gc_mutex);
>  	hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
>  		xfrm_state_gc_destroy(x);
> +	mutex_unlock(&xfrm_state_gc_mutex);
>  }
>  
>  static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
> @@ -1000,6 +1003,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
>  out:
>  	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
>  
> +	mutex_lock(&xfrm_state_gc_mutex);
>  	spin_lock_bh(&xfrm_state_dev_gc_lock);
>  restart_gc:
>  	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {

[Severity: High]
Is the new mutex enough to cover every xfrm_dev_state_free() caller?  There
is a third caller in the acquire path in xfrm_state_find() that takes
neither the new mutex nor RTNL:

net/xfrm/xfrm_state.c:xfrm_state_find() {
	...
#ifdef CONFIG_XFRM_OFFLOAD
		struct xfrm_dev_offload *xso = &x->xso;

		if (xso->type == XFRM_DEV_OFFLOAD_PACKET) {
			xfrm_dev_state_delete(x);
			xfrm_dev_state_free(x);
		}
#endif
	...
}

That code runs inside the rcu_read_lock() taken earlier in
xfrm_state_find() (it uses xfrm_state_hold_rcu() on the found state), so it
cannot take a sleeping mutex even if one were added there.

xfrm_dev_state_delete() publishes x on the global, reference-less device GC
list while xso->dev is still set:

net/xfrm/xfrm_state.c:xfrm_dev_state_delete() {
	if (dev) {
		dev->xfrmdev_ops->xdo_dev_state_delete(dev, x);
		spin_lock_bh(&xfrm_state_dev_gc_lock);
		hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
		spin_unlock_bh(&xfrm_state_dev_gc_lock);
	}
}

so the deferred pass added here, which matches only on xso->dev == dev, can
pick up the same state and call xfrm_dev_state_free() on it in parallel:

  CPU 0 (xfrm_state_find(), packet offload, km_query() failed)
  xfrm_dev_state_delete(x)		/* x now on xfrm_state_dev_gc_list */
  xfrm_dev_state_free(x)
    dev = READ_ONCE(xso->dev)		/* non-NULL */

  CPU 1 (xfrm_dev_event() -> xfrm_dev_state_flush(), same dev)
  mutex_lock(&xfrm_state_gc_mutex)
  finds x, xso->dev == dev
  spin_unlock_bh(&xfrm_state_dev_gc_lock)
  xfrm_dev_state_free(x)
    dev = READ_ONCE(xso->dev)		/* also non-NULL */

Can both CPUs then run the driver callback and the device put twice?

net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
	if (dev && dev->xfrmdev_ops) {
		spin_lock_bh(&xfrm_state_dev_gc_lock);
		if (!hlist_unhashed(&x->dev_gclist))
			hlist_del(&x->dev_gclist);
		spin_unlock_bh(&xfrm_state_dev_gc_lock);

		if (dev->xfrmdev_ops->xdo_dev_state_free)
			dev->xfrmdev_ops->xdo_dev_state_free(dev, x);
		WRITE_ONCE(xso->dev, NULL);
		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
		netdev_put(dev, &xso->dev_tracker);
	}
}

The hlist_unhashed() test only guards the list removal, not
xdo_dev_state_free() or netdev_put(), and dev is sampled before the
spinlock, so a double driver teardown and a net_device refcount underflow
look possible.

Also, since hlist_del() is used rather than hlist_del_init(), does the
second caller see the poisoned pprev as still hashed and call __hlist_del()
again, writing through LIST_POISON2?

This is the same failure mode the commit message describes:

  "Both paths can invoke the driver callback and drop the device reference."

Would addressing the root cause the commit message names -- "The device GC
list does not hold an xfrm_state reference" -- be a better fit, either by
taking an xfrm_state reference for the device GC list, or by making the
release in xfrm_dev_state_free() idempotent (test-and-clear of xso->dev plus
hlist_del_init() under xfrm_state_dev_gc_lock)?  That would also cover the
xfrm_state_find() caller, which cannot participate in a mutex-based scheme.

For reference, no later commit in this series through 231bb4a13161 changes
xfrm_dev_state_free(), xfrm_dev_state_delete() or the xfrm_state_find() call
site.

> @@ -1014,6 +1018,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
>  
>  	}
>  	spin_unlock_bh(&xfrm_state_dev_gc_lock);
> +	mutex_unlock(&xfrm_state_gc_mutex);
>  
>  	xfrm_flush_gc();
>

Re: [PATCH 02/12] xfrm: serialize state GC with device state flush

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:48:54PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> xfrm: serialize state GC with device state flush
> 
> This adds a global mutex, xfrm_state_gc_mutex, taken by the state GC worker
> around xfrm_state_gc_destroy() and by the deferred-device pass of
> xfrm_dev_state_flush(), so the two paths no longer run xfrm_dev_state_free()
> on the same state concurrently.  The KASAN slab-use-after-free reported in
> xfrm_dev_state_free() from xfrm_dev_state_flush() is addressed by that
> serialization.
> 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 36a4f6793edef..de097bba803b7 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -226,6 +226,7 @@ static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];
> >  
> >  static DEFINE_SPINLOCK(xfrm_state_gc_lock);
> >  static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
> > +static DEFINE_MUTEX(xfrm_state_gc_mutex);
> >  
> >  int __xfrm_state_delete(struct xfrm_state *x);
> >  
> > @@ -632,8 +633,10 @@ static void xfrm_state_gc_task(struct work_struct *work)
> >  
> >  	synchronize_rcu();
> >  
> > +	mutex_lock(&xfrm_state_gc_mutex);
> >  	hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
> >  		xfrm_state_gc_destroy(x);
> > +	mutex_unlock(&xfrm_state_gc_mutex);
> >  }
> >  
> >  static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
> > @@ -1000,6 +1003,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
> >  out:
> >  	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
> >  
> > +	mutex_lock(&xfrm_state_gc_mutex);
> >  	spin_lock_bh(&xfrm_state_dev_gc_lock);
> >  restart_gc:
> >  	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
> 
> [Severity: High]
> Is the new mutex enough to cover every xfrm_dev_state_free() caller?  There
> is a third caller in the acquire path in xfrm_state_find() that takes
> neither the new mutex nor RTNL:
> 
> net/xfrm/xfrm_state.c:xfrm_state_find() {
> 	...
> #ifdef CONFIG_XFRM_OFFLOAD
> 		struct xfrm_dev_offload *xso = &x->xso;
> 
> 		if (xso->type == XFRM_DEV_OFFLOAD_PACKET) {
> 			xfrm_dev_state_delete(x);
> 			xfrm_dev_state_free(x);
> 		}
> #endif
> 	...
> }
> 
> That code runs inside the rcu_read_lock() taken earlier in
> xfrm_state_find() (it uses xfrm_state_hold_rcu() on the found state), so it
> cannot take a sleeping mutex even if one were added there.

That just pointed me to the xfrm_dev_state_free callback of the bonding driver,
it uses a mutex. But this is a different issue.

> 
> xfrm_dev_state_delete() publishes x on the global, reference-less device GC
> list while xso->dev is still set:
> 
> net/xfrm/xfrm_state.c:xfrm_dev_state_delete() {
> 	if (dev) {
> 		dev->xfrmdev_ops->xdo_dev_state_delete(dev, x);
> 		spin_lock_bh(&xfrm_state_dev_gc_lock);
> 		hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
> 		spin_unlock_bh(&xfrm_state_dev_gc_lock);
> 	}
> }
> 
> so the deferred pass added here, which matches only on xso->dev == dev, can
> pick up the same state and call xfrm_dev_state_free() on it in parallel:
> 
>   CPU 0 (xfrm_state_find(), packet offload, km_query() failed)
>   xfrm_dev_state_delete(x)		/* x now on xfrm_state_dev_gc_list */
>   xfrm_dev_state_free(x)
>     dev = READ_ONCE(xso->dev)		/* non-NULL */
> 
>   CPU 1 (xfrm_dev_event() -> xfrm_dev_state_flush(), same dev)
>   mutex_lock(&xfrm_state_gc_mutex)
>   finds x, xso->dev == dev
>   spin_unlock_bh(&xfrm_state_dev_gc_lock)
>   xfrm_dev_state_free(x)
>     dev = READ_ONCE(xso->dev)		/* also non-NULL */
> 
> Can both CPUs then run the driver callback and the device put twice?
> 
> net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
> 	if (dev && dev->xfrmdev_ops) {
> 		spin_lock_bh(&xfrm_state_dev_gc_lock);
> 		if (!hlist_unhashed(&x->dev_gclist))
> 			hlist_del(&x->dev_gclist);
> 		spin_unlock_bh(&xfrm_state_dev_gc_lock);
> 
> 		if (dev->xfrmdev_ops->xdo_dev_state_free)
> 			dev->xfrmdev_ops->xdo_dev_state_free(dev, x);
> 		WRITE_ONCE(xso->dev, NULL);
> 		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> 		netdev_put(dev, &xso->dev_tracker);
> 	}
> }
> 
> The hlist_unhashed() test only guards the list removal, not
> xdo_dev_state_free() or netdev_put(), and dev is sampled before the
> spinlock, so a double driver teardown and a net_device refcount underflow
> look possible.
> 
> Also, since hlist_del() is used rather than hlist_del_init(), does the
> second caller see the poisoned pprev as still hashed and call __hlist_del()
> again, writing through LIST_POISON2?
> 
> This is the same failure mode the commit message describes:
> 
>   "Both paths can invoke the driver callback and drop the device reference."
> 
> Would addressing the root cause the commit message names -- "The device GC
> list does not hold an xfrm_state reference" -- be a better fit, either by
> taking an xfrm_state reference for the device GC list, or by making the
> release in xfrm_dev_state_free() idempotent (test-and-clear of xso->dev plus
> hlist_del_init() under xfrm_state_dev_gc_lock)?  That would also cover the
> xfrm_state_find() caller, which cannot participate in a mutex-based scheme.

This approach can indeed not solve the problem completely. I tend to
revert this one in favor of a fix with the above approach. This is
the destruction path, so a refcount would probably not harm too
much.

Chengfeng, can you have a look at this?

Re: [PATCH 02/12] xfrm: serialize state GC with device state flush

[Chengfeng Ye]

On Mon, Sep 14, 2026 at 7:23 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> Ccing Author of the patch.
>
> On Tue, Sep 08, 2026 at 03:48:54PM -0700, Jakub Kicinski wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > ---
> > xfrm: serialize state GC with device state flush
> >
> > This adds a global mutex, xfrm_state_gc_mutex, taken by the state GC worker
> > around xfrm_state_gc_destroy() and by the deferred-device pass of
> > xfrm_dev_state_flush(), so the two paths no longer run xfrm_dev_state_free()
> > on the same state concurrently.  The KASAN slab-use-after-free reported in
> > xfrm_dev_state_free() from xfrm_dev_state_flush() is addressed by that
> > serialization.
> >
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index 36a4f6793edef..de097bba803b7 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -226,6 +226,7 @@ static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];
> > >
> > >  static DEFINE_SPINLOCK(xfrm_state_gc_lock);
> > >  static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
> > > +static DEFINE_MUTEX(xfrm_state_gc_mutex);
> > >
> > >  int __xfrm_state_delete(struct xfrm_state *x);
> > >
> > > @@ -632,8 +633,10 @@ static void xfrm_state_gc_task(struct work_struct *work)
> > >
> > >     synchronize_rcu();
> > >
> > > +   mutex_lock(&xfrm_state_gc_mutex);
> > >     hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
> > >             xfrm_state_gc_destroy(x);
> > > +   mutex_unlock(&xfrm_state_gc_mutex);
> > >  }
> > >
> > >  static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
> > > @@ -1000,6 +1003,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
> > >  out:
> > >     spin_unlock_bh(&net->xfrm.xfrm_state_lock);
> > >
> > > +   mutex_lock(&xfrm_state_gc_mutex);
> > >     spin_lock_bh(&xfrm_state_dev_gc_lock);
> > >  restart_gc:
> > >     hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
> >
> > [Severity: High]
> > Is the new mutex enough to cover every xfrm_dev_state_free() caller?  There
> > is a third caller in the acquire path in xfrm_state_find() that takes
> > neither the new mutex nor RTNL:
> >
> > net/xfrm/xfrm_state.c:xfrm_state_find() {
> >       ...
> > #ifdef CONFIG_XFRM_OFFLOAD
> >               struct xfrm_dev_offload *xso = &x->xso;
> >
> >               if (xso->type == XFRM_DEV_OFFLOAD_PACKET) {
> >                       xfrm_dev_state_delete(x);
> >                       xfrm_dev_state_free(x);
> >               }
> > #endif
> >       ...
> > }
> >
> > That code runs inside the rcu_read_lock() taken earlier in
> > xfrm_state_find() (it uses xfrm_state_hold_rcu() on the found state), so it
> > cannot take a sleeping mutex even if one were added there.
>
> That just pointed me to the xfrm_dev_state_free callback of the bonding driver,
> it uses a mutex. But this is a different issue.
>
> >
> > xfrm_dev_state_delete() publishes x on the global, reference-less device GC
> > list while xso->dev is still set:
> >
> > net/xfrm/xfrm_state.c:xfrm_dev_state_delete() {
> >       if (dev) {
> >               dev->xfrmdev_ops->xdo_dev_state_delete(dev, x);
> >               spin_lock_bh(&xfrm_state_dev_gc_lock);
> >               hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
> >               spin_unlock_bh(&xfrm_state_dev_gc_lock);
> >       }
> > }
> >
> > so the deferred pass added here, which matches only on xso->dev == dev, can
> > pick up the same state and call xfrm_dev_state_free() on it in parallel:
> >
> >   CPU 0 (xfrm_state_find(), packet offload, km_query() failed)
> >   xfrm_dev_state_delete(x)            /* x now on xfrm_state_dev_gc_list */
> >   xfrm_dev_state_free(x)
> >     dev = READ_ONCE(xso->dev)         /* non-NULL */
> >
> >   CPU 1 (xfrm_dev_event() -> xfrm_dev_state_flush(), same dev)
> >   mutex_lock(&xfrm_state_gc_mutex)
> >   finds x, xso->dev == dev
> >   spin_unlock_bh(&xfrm_state_dev_gc_lock)
> >   xfrm_dev_state_free(x)
> >     dev = READ_ONCE(xso->dev)         /* also non-NULL */
> >
> > Can both CPUs then run the driver callback and the device put twice?
> >
> > net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
> >       if (dev && dev->xfrmdev_ops) {
> >               spin_lock_bh(&xfrm_state_dev_gc_lock);
> >               if (!hlist_unhashed(&x->dev_gclist))
> >                       hlist_del(&x->dev_gclist);
> >               spin_unlock_bh(&xfrm_state_dev_gc_lock);
> >
> >               if (dev->xfrmdev_ops->xdo_dev_state_free)
> >                       dev->xfrmdev_ops->xdo_dev_state_free(dev, x);
> >               WRITE_ONCE(xso->dev, NULL);
> >               xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> >               netdev_put(dev, &xso->dev_tracker);
> >       }
> > }
> >
> > The hlist_unhashed() test only guards the list removal, not
> > xdo_dev_state_free() or netdev_put(), and dev is sampled before the
> > spinlock, so a double driver teardown and a net_device refcount underflow
> > look possible.
> >
> > Also, since hlist_del() is used rather than hlist_del_init(), does the
> > second caller see the poisoned pprev as still hashed and call __hlist_del()
> > again, writing through LIST_POISON2?
> >
> > This is the same failure mode the commit message describes:
> >
> >   "Both paths can invoke the driver callback and drop the device reference."
> >
> > Would addressing the root cause the commit message names -- "The device GC
> > list does not hold an xfrm_state reference" -- be a better fit, either by
> > taking an xfrm_state reference for the device GC list, or by making the
> > release in xfrm_dev_state_free() idempotent (test-and-clear of xso->dev plus
> > hlist_del_init() under xfrm_state_dev_gc_lock)?  That would also cover the
> > xfrm_state_find() caller, which cannot participate in a mutex-based scheme.
>
> This approach can indeed not solve the problem completely. I tend to
> revert this one in favor of a fix with the above approach. This is
> the destruction path, so a refcount would probably not harm too
> much.
>
> Chengfeng, can you have a look at this?

No problem at all, I will prepare a replacement patch with refcount
fix to completely fix the issue.

Best regards,
Chengfeng

[PATCH 03/12] xfrm: add missing RCU read lock in xfrm_send_migrate_state()

[Steffen Klassert]

From: Aleksandr Nogikh <nogikh@google.com>

xfrm_nlmsg_multicast() requires the RCU read lock to be held because it
safely dereferences the net->xfrm.nlsk pointer using rcu_dereference().
When it is called from xfrm_send_migrate_state(), the RCU read lock is not
held, which triggers a suspicious RCU usage warning:

WARNING: suspicious RCU usage
net/xfrm/xfrm_user.c:1630 suspicious rcu_dereference_check() usage!

Call Trace:
 lockdep_rcu_suspicious+0x13f/0x1d0 kernel/locking/lockdep.c:6876
 xfrm_nlmsg_multicast+0x1d8/0x1f0 net/xfrm/xfrm_user.c:1630
 xfrm_send_migrate_state+0x870/0xae0 net/xfrm/xfrm_user.c:3340
 xfrm_do_migrate_state+0x1749/0x1e90 net/xfrm/xfrm_user.c:3507
 xfrm_user_rcv_msg+0x7a8/0xf30 net/xfrm/xfrm_user.c:3907

Fix this by wrapping the xfrm_nlmsg_multicast() call in
xfrm_send_migrate_state() with rcu_read_lock() and rcu_read_unlock().

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Assisted-by: Gemini:gemini-3.5-flash Gemini:gemini-3.1-pro-preview syzbot
Reported-by: syzbot+c0e99a1aa85a286d7a3b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c0e99a1aa85a286d7a3b
Link: https://syzkaller.appspot.com/ai_job?id=8977f559-3a7e-4bb5-b4d6-1196956260b6
Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6266a92cf302..980dbeb5a57d 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3337,7 +3337,11 @@ static int xfrm_send_migrate_state(struct net *net,
 		return err;
 	}
 
-	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
+	rcu_read_lock();
+	err = xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
+	rcu_read_unlock();
+
+	return err;
 }
 
 static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
-- 
2.43.0

[PATCH 04/12] xfrm: iptfs: fix runt reassembly panic from short inner tot_len

[Steffen Klassert]

From: Henry Martin <bsdhenrymartin@gmail.com>

When the start of an inner packet is split across two outer packets
such that fewer than 4 bytes land at the end of the first one,
__input_process_payload() saves those bytes as a runt and skips the
iplen/iphlen validation performed for in-place packets. When the
continuation packet arrives, iptfs_reassem_cont() only requires the
declared inner length to be >= sizeof(ra_runt) (6) before allocating
the reassembly skb with that attacker-controlled length.

However, __iptfs_iphlen() always returns the fixed minimum IP header
size (20 for IPv4, 40 for IPv6), so for an inner IPv4 tot_len in
[6, 19] the header-completion copy writes past the declared packet
length, and the subsequent "ipremain -= copylen" underflows to ~4GB,
leaving the payload copy length bounded only by blkoff (up to 64KB).
At runtime the skb_put() tailroom check turns this into
skb_over_panic(), i.e. an unprivileged kernel panic (DoS), reachable
locally via userns+netns IPTFS SAs and remotely against IPTFS VPN
gateways when the decrypted outer skb is linear (e.g. AF_PACKET taps,
tun/tap delivery).

Align the runt path with the normal path by requiring the declared
inner length to cover at least the IP header size. This also subsumes
the previous >= sizeof(ra_runt) check, since the minimum IP header
is always larger than the runt buffer.

This issue was found by the autokbug dynamic kernel fuzzer at
Tencent Yunding Lab.

Fixes: 075694765446 ("xfrm: iptfs: handle received fragmented inner packets")
Reported-by: Henry Martin <bsdhenrymartin@gmail.com>
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_iptfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 2ce15c472cc4..6920940a35b4 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -828,8 +828,8 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 		 * allocate an in progress skb
 		 */
 		ipremain = __iptfs_iplen(xtfs->ra_runt);
-		if (ipremain < sizeof(xtfs->ra_runt)) {
-			/* length has to be at least runtsize large */
+		if (ipremain < __iptfs_iphlen(xtfs->ra_runt)) {
+			/* length has to be at least the IP header size */
 			XFRM_INC_STATS(xs_net(xtfs->x),
 				       LINUX_MIB_XFRMINIPTFSERROR);
 			goto abandon;
-- 
2.43.0

Re: [PATCH 04/12] xfrm: iptfs: fix runt reassembly panic from short inner tot_len

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: iptfs: fix runt reassembly panic from short inner tot_len

This tightens the runt continuation path in iptfs_reassem_cont() so the
declared inner IP length must cover at least the IP header size before the
in-progress reassembly skb is allocated, replacing the old check against
sizeof(xtfs->ra_runt). A few questions below on how completely this matches
the in-place validation, plus two pre-existing observations in the same
function.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 2ce15c472cc4d..6920940a35b49 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -828,8 +828,8 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
>  		 * allocate an in progress skb
>  		 */
>  		ipremain = __iptfs_iplen(xtfs->ra_runt);
> -		if (ipremain < sizeof(xtfs->ra_runt)) {
> -			/* length has to be at least runtsize large */
> +		if (ipremain < __iptfs_iphlen(xtfs->ra_runt)) {
> +			/* length has to be at least the IP header size */

[Severity: Medium]
The changelog says this will "Align the runt path with the normal path by
requiring the declared inner length to cover at least the IP header size."
Is that alignment complete for IPv4 with options?

__iptfs_iphlen() never looks at iph->ihl:

net/xfrm/xfrm_iptfs.c:__iptfs_iphlen() {
	if (iph->version == 0x4)
		return sizeof(*iph);
	return sizeof(struct ipv6hdr);
}

The in-place parser uses the declared header length instead:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
	...
	iplen = be16_to_cpu(iph->tot_len);
	iphlen = iph->ihl << 2;
	if (iplen < iphlen || iphlen < sizeof(*iph)) {
		XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
	...
}

So a runt-started inner IPv4 packet declaring ihl < 5, or ihl * 4 greater
than tot_len (for example tot_len 20 with ihl 15), passes the new check,
gets a 20 byte allocation, is treated as completely reassembled and is
handed to iptfs_complete_inner_skb() and xfrm_input(), while byte-identical
input arriving wholly inside one outer payload is rejected with
LINUX_MIB_XFRMINHDRERROR. Should the runt path also test iph->ihl so the
two parsers agree on both acceptance and MIB accounting?

As far as I can tell the residual effect is bounded, since ip_rcv_core()
drops such a packet:

net/ipv4/ip_input.c:ip_rcv_core() {
	if (iph->ihl < 5 || iph->version != 4)
		goto inhdr_error;
	...
	if (!pskb_may_pull(skb, iph->ihl*4))
		goto inhdr_error;
}

The panic class described in the changelog (tot_len in [6, 19] making
"ipremain -= copylen" underflow) does look closed by this change for the
runt path.

Also, the kernel-doc on __iptfs_iphlen() says it returns "the IP header
size based on the IP version", and the new comment here says "length has to
be at least the IP header size". For IPv4 with options that is the minimum
header size rather than the header size. Would it be worth rewording either
the comment or the helper's kernel-doc?

>  			XFRM_INC_STATS(xs_net(xtfs->x),
>  				       LINUX_MIB_XFRMINIPTFSERROR);
>  			goto abandon;

Two further observations in this same function, both outside the hunk.

[Severity: High]
This isn't a bug introduced by this patch, but can the copy fallback later
in iptfs_reassem_cont() still reach skb_over_panic() on the normal
(non-runt) path?

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
	} else {
		if (skb_linearize(newskb)) {
			XFRM_INC_STATS(xs_net(xtfs->x),
				       LINUX_MIB_XFRMINBUFFERERROR);
			goto abandon;
		}

		/* copy fragment data into newskb */
		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
				      copylen)) {
	...
}

There is no tailroom check before skb_put(newskb, copylen), and copylen is
min(min(blkoff, remaining), ipremain), so it is bounded only by the outer
payload length and the declared inner IP length.

When the in-progress skb came from the frag-sharing path, its linear head
is only iphlen plus XFRM_IPTFS_MIN_L2HEADROOM:

net/xfrm/xfrm_iptfs.c:iptfs_pskb_add_frags() {
	skb = iptfs_alloc_skb(tpl, copy_len, false);
	...
}

reached from __input_process_payload():

			    !(skb = iptfs_pskb_add_frags(first_skb, fragwalk,
							 data, capturelen,
							 skbseq, iphlen))) {

and skb_linearize() only adds about 128 bytes of tailroom:

net/core/skbuff.c:__pskb_pull_tail() {
	int i, k, eat = (skb->tail + delta) - skb->end;
	...
	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
	...
}

A continuation outer packet that is a plain kmalloc'd linear skb fails the
sharing precondition (head_frag is 0 and skb->len != skb->data_len), so it
lands in this fallback. With a roughly 1420 byte reassembly the head ends
up with a couple of hundred bytes of tailroom, and a continuation carrying
several hundred bytes more makes skb->tail exceed skb->end, which
skb_put() turns into skb_over_panic() and BUG(). Would guaranteeing the
tailroom (pskb_expand_head() or a skb_tailroom() test against ipremain)
before the skb_put() be appropriate here? This path still appears to be
present at the tip of the series.

[Severity: Low]
This is a pre-existing asymmetry rather than something this patch changes,
but should the runt path also arm the drop timer when it publishes
xtfs->ra_newskb?

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
		newskb = iptfs_alloc_skb(skb, ipremain, false);
		if (!newskb) {
			XFRM_INC_STATS(xs_net(xtfs->x), LINUX_MIB_XFRMINERROR);
			goto abandon;
		}
		xtfs->ra_newskb = newskb;
	...
}

The reassembly-start site in __input_process_payload() does arm it:

			xtfs->ra_newskb = skb;
			xtfs->ra_wantseq = seq + 1;
			if (!hrtimer_is_queued(&xtfs->drop_timer)) {
				hrtimer_start(&xtfs->drop_timer,
					      xtfs->drop_time_ns,
					      IPTFS_HRTIMER_MODE);
			}

So a reassembly started from the runt path has no expiry, and if the peer
never sends the next sequence the skb (sized by the declared inner length,
up to about 64KB) stays pinned on the SA until another packet arrives or
the state goes away. The memory is reclaimed at teardown:

net/xfrm/xfrm_iptfs.c:iptfs_destroy_state() {
	hrtimer_cancel(&xtfs->drop_timer);

	if (xtfs->ra_newskb)
		kfree_skb(xtfs->ra_newskb);
	...
}

so this looks like retention past the intended drop time rather than a
leak. The publication itself is serialized, since iptfs_input_ordered()
holds xtfs->drop_lock across the iptfs_reassem_cont() call.

Re: [PATCH 04/12] xfrm: iptfs: fix runt reassembly panic from short inner tot_len

[Steffen Klassert]

Ccing Author of the patch and author of IPTFS.

On Tue, Sep 08, 2026 at 03:48:55PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> xfrm: iptfs: fix runt reassembly panic from short inner tot_len
> 
> This tightens the runt continuation path in iptfs_reassem_cont() so the
> declared inner IP length must cover at least the IP header size before the
> in-progress reassembly skb is allocated, replacing the old check against
> sizeof(xtfs->ra_runt). A few questions below on how completely this matches
> the in-place validation, plus two pre-existing observations in the same
> function.
> 
> > diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> > index 2ce15c472cc4d..6920940a35b49 100644
> > --- a/net/xfrm/xfrm_iptfs.c
> > +++ b/net/xfrm/xfrm_iptfs.c
> > @@ -828,8 +828,8 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
> >  		 * allocate an in progress skb
> >  		 */
> >  		ipremain = __iptfs_iplen(xtfs->ra_runt);
> > -		if (ipremain < sizeof(xtfs->ra_runt)) {
> > -			/* length has to be at least runtsize large */
> > +		if (ipremain < __iptfs_iphlen(xtfs->ra_runt)) {
> > +			/* length has to be at least the IP header size */
> 
> [Severity: Medium]
> The changelog says this will "Align the runt path with the normal path by
> requiring the declared inner length to cover at least the IP header size."
> Is that alignment complete for IPv4 with options?
> 
> __iptfs_iphlen() never looks at iph->ihl:
> 
> net/xfrm/xfrm_iptfs.c:__iptfs_iphlen() {
> 	if (iph->version == 0x4)
> 		return sizeof(*iph);
> 	return sizeof(struct ipv6hdr);
> }
> 
> The in-place parser uses the declared header length instead:
> 
> net/xfrm/xfrm_iptfs.c:__input_process_payload() {
> 	...
> 	iplen = be16_to_cpu(iph->tot_len);
> 	iphlen = iph->ihl << 2;
> 	if (iplen < iphlen || iphlen < sizeof(*iph)) {
> 		XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
> 	...
> }
> 
> So a runt-started inner IPv4 packet declaring ihl < 5, or ihl * 4 greater
> than tot_len (for example tot_len 20 with ihl 15), passes the new check,
> gets a 20 byte allocation, is treated as completely reassembled and is
> handed to iptfs_complete_inner_skb() and xfrm_input(), while byte-identical
> input arriving wholly inside one outer payload is rejected with
> LINUX_MIB_XFRMINHDRERROR. Should the runt path also test iph->ihl so the
> two parsers agree on both acceptance and MIB accounting?
> 
> As far as I can tell the residual effect is bounded, since ip_rcv_core()
> drops such a packet:
> 
> net/ipv4/ip_input.c:ip_rcv_core() {
> 	if (iph->ihl < 5 || iph->version != 4)
> 		goto inhdr_error;
> 	...
> 	if (!pskb_may_pull(skb, iph->ihl*4))
> 		goto inhdr_error;
> }
> 
> The panic class described in the changelog (tot_len in [6, 19] making
> "ipremain -= copylen" underflow) does look closed by this change for the
> runt path.

I think this is OK as is. The panic is fixed and it did not add a
regression.

> 
> Also, the kernel-doc on __iptfs_iphlen() says it returns "the IP header
> size based on the IP version", and the new comment here says "length has to
> be at least the IP header size". For IPv4 with options that is the minimum
> header size rather than the header size. Would it be worth rewording either
> the comment or the helper's kernel-doc?
> 
> >  			XFRM_INC_STATS(xs_net(xtfs->x),
> >  				       LINUX_MIB_XFRMINIPTFSERROR);
> >  			goto abandon;
> 
> Two further observations in this same function, both outside the hunk.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can the copy fallback later
> in iptfs_reassem_cont() still reach skb_over_panic() on the normal
> (non-runt) path?

This is not a regression introduced by this patch. Can be fixed as
followup.

[PATCH 05/12] ipv6: xfrm: use full sockets in local error paths

[Steffen Klassert]

From: Zhiling Zou <zhilinz@nebusec.ai>

xfrm6_local_rxpmtu() and xfrm6_local_error() dereference skb->sk as if it
always pointed at a full IPv6 socket.

That is not guaranteed. TCP SYN-ACK skbs can be owned by a
TCP_NEW_SYN_RECV request_sock while the output path itself is driven by the
full listener. If rerouting selects an IPv6 XFRM tunnel route with a lower
MTU, the local PMTU/error handling path can reach these callbacks with that
mini-socket still attached to the skb.

The callbacks then miscast the request socket as a full inet/IPv6 socket and
can read beyond the request_sock allocation when they access inet_sock or
ipv6_pinfo state.

Resolve the owner with skb_to_full_sk() in both callbacks and bail out when
no full socket is attached. This matches the surrounding XFRM IPv6 PMTU/error
logic, which already reasons about full sockets with skb_to_full_sk().

Fixes: dd767856a36e ("xfrm6: Don't call icmpv6_send on local error")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv6/xfrm6_output.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 512bdaf13699..44b221a09a0c 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -19,7 +19,10 @@
 void xfrm6_local_rxpmtu(struct sk_buff *skb, u32 mtu)
 {
 	struct flowi6 fl6;
-	struct sock *sk = skb->sk;
+	struct sock *sk = skb_to_full_sk(skb);
+
+	if (!sk)
+		return;
 
 	fl6.flowi6_oif = sk->sk_bound_dev_if;
 	fl6.daddr = ipv6_hdr(skb)->daddr;
@@ -31,7 +34,10 @@ void xfrm6_local_error(struct sk_buff *skb, u32 mtu)
 {
 	struct flowi6 fl6;
 	const struct ipv6hdr *hdr;
-	struct sock *sk = skb->sk;
+	struct sock *sk = skb_to_full_sk(skb);
+
+	if (!sk)
+		return;
 
 	hdr = skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
 	fl6.fl6_dport = inet_sk(sk)->inet_dport;
-- 
2.43.0

Re: [PATCH 05/12] ipv6: xfrm: use full sockets in local error paths

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipv6: xfrm: use full sockets in local error paths

xfrm6_local_rxpmtu() and xfrm6_local_error() used skb->sk directly and cast
it to a full inet/IPv6 socket, which is wrong when a SYN-ACK skb is owned by
a TCP_NEW_SYN_RECV request_sock.  Both callbacks now resolve the owner with
skb_to_full_sk() and return early when there is no full socket attached.

> diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
> index 512bdaf136997..44b221a09a0c8 100644
> --- a/net/ipv6/xfrm6_output.c
> +++ b/net/ipv6/xfrm6_output.c

[ ... ]

> @@ -31,7 +34,10 @@ void xfrm6_local_error(struct sk_buff *skb, u32 mtu)
>  {
>  	struct flowi6 fl6;
>  	const struct ipv6hdr *hdr;
> -	struct sock *sk = skb->sk;
> +	struct sock *sk = skb_to_full_sk(skb);
> +
> +	if (!sk)
> +		return;

[Severity: High]
Should the IPv4 sibling of this same afinfo->local_error hook get the same
treatment?  xfrm4_local_error() in net/ipv4/xfrm4_output.c still hands the
raw skb->sk to inet_sk():

net/ipv4/xfrm4_output.c:xfrm4_local_error() {
	hdr = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
	ip_local_error(skb->sk, EMSGSIZE, hdr->daddr,
		       inet_sk(skb->sk)->inet_dport, mtu);
}

inet_dport is skc_dport and stays in bounds, but ip_local_error() starts
with:

net/ipv4/ip_sockglue.c:ip_local_error() {
	if (!inet_test_bit(RECVERR, sk))
		return;

which expands to test_bit(INET_FLAGS_RECVERR, &inet_sk(sk)->inet_flags).
inet_flags sits after the embedded struct sock in struct inet_sock, well
past the end of a tcp_request_sock allocation, so isn't this the same read
beyond the request_sock allocation that the commit message describes for
IPv6?  And if that out-of-bounds bit happens to read as set, execution
continues into sock_queue_err_skb(sk, skb), which touches
sk->sk_error_queue - also outside the request_sock object.

Is that path reachable today?  For a SYN-ACK, tcp_make_synack() attaches the
skb to the request sock:

net/ipv4/tcp_output.c:tcp_make_synack() {
	case TCP_SYNACK_NORMAL:
	case TCP_SYNACK_RETRANS:
		skb_set_owner_edemux(skb, req_to_sk(req));

and for an IPTFS SA the output path goes
iptfs_output_collect()->iptfs_is_too_big(), which gates on the sk argument
(the listener) rather than on skb->sk:

net/xfrm/xfrm_iptfs.c:iptfs_is_too_big() {
	if (sk)
		xfrm_local_error(skb, pmtu);

and the AF_INET branch of the dispatcher does no full-socket check either:

net/xfrm/xfrm_output.c:xfrm_local_error() {
	if (skb->protocol == htons(ETH_P_IP))
		proto = AF_INET;

The classic ESP path avoids this because xfrm4_tunnel_check_size() checks
skb->sk && sk_fullsock(skb->sk) before calling xfrm_local_error(), so the
contract looks like it is enforced only at some call sites.  Would it make
sense to fix xfrm4_local_error() in the same series, or push the
skb_to_full_sk() check up into xfrm_local_error() so both families are
covered?

>  
>  	hdr = skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
>  	fl6.fl6_dport = inet_sk(sk)->inet_dport;

[Severity: Low]
Does this now report the wrong offender port?  sk_to_full_sk() replaces the
request sock with the listener:

include/net/inet_sock.h:sk_to_full_sk() {
	if (sk && READ_ONCE(sk->sk_state) == TCP_NEW_SYN_RECV)
		sk = inet_reqsk(sk)->rsk_listener;

A listening socket has inet_dport == 0, while fl6.daddr on the next line
still comes from the packet, i.e. the real peer.

ipv6_local_error() stores that value:

net/ipv6/datagram.c:ipv6_local_error() {
	serr->port = fl6->fl6_dport;

and ipv6_recv_error() hands it to userspace:

net/ipv6/datagram.c:ipv6_recv_error() {
		sin->sin6_port = serr->port;

So with IPV6_RECVERR set on the listener, the SO_EE_OFFENDER sockaddr_in6
ends up as <real peer address, port 0>.  Before this change the same
expression read the request sock's skc_dport, which is the request's actual
remote port.  Would using the request's remote port (ir_rmt_port) when the
skb owner is a request sock keep both halves of the error record
consistent?

Re: [PATCH 05/12] ipv6: xfrm: use full sockets in local error paths

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:48:56PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ipv6: xfrm: use full sockets in local error paths
> 
> xfrm6_local_rxpmtu() and xfrm6_local_error() used skb->sk directly and cast
> it to a full inet/IPv6 socket, which is wrong when a SYN-ACK skb is owned by
> a TCP_NEW_SYN_RECV request_sock.  Both callbacks now resolve the owner with
> skb_to_full_sk() and return early when there is no full socket attached.
> 
> > diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
> > index 512bdaf136997..44b221a09a0c8 100644
> > --- a/net/ipv6/xfrm6_output.c
> > +++ b/net/ipv6/xfrm6_output.c
> 
> [ ... ]
> 
> > @@ -31,7 +34,10 @@ void xfrm6_local_error(struct sk_buff *skb, u32 mtu)
> >  {
> >  	struct flowi6 fl6;
> >  	const struct ipv6hdr *hdr;
> > -	struct sock *sk = skb->sk;
> > +	struct sock *sk = skb_to_full_sk(skb);
> > +
> > +	if (!sk)
> > +		return;
> 
> [Severity: High]
> Should the IPv4 sibling of this same afinfo->local_error hook get the same
> treatment?  xfrm4_local_error() in net/ipv4/xfrm4_output.c still hands the
> raw skb->sk to inet_sk():
> 
> net/ipv4/xfrm4_output.c:xfrm4_local_error() {
> 	hdr = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
> 	ip_local_error(skb->sk, EMSGSIZE, hdr->daddr,
> 		       inet_sk(skb->sk)->inet_dport, mtu);
> }

There are still direct references of skb->sk that might need to
be moved to skb_to_full_sk in the codebase. The patch fixed what
described in the commit message and did not introduce a regression.

[PATCH 06/12] xfrm: fix compat ALLOCSPI request use-after-free

[Steffen Klassert]

From: Kyle Zeng <kylebot@openai.com>

xfrm_state_netlink() builds the ALLOCSPI response with
dump_one_state(), which already calls alloc_compat() with the response
skb and header.

xfrm_alloc_userspi() then calls alloc_compat() again, but passes the
original request skb and its header. For a compat request, the
translator therefore interprets the 228-byte compat xfrm_userspi_info
as the 232-byte native layout and reads four bytes past the declared
payload. It also publishes the translated child through the request's
frag_list.

A multicast clone of the request shares skb_shared_info and can observe
that child. xfrm_user_rcv_msg() frees it after the request handler
returns, racing a compat receiver which may still be copying from it and
resulting in a use-after-free.

Remove the redundant conversion. The response keeps its correct compat
translation from dump_one_state(), and no child is attached to the
inbound request.

Fixes: 5f3eea6b7e8f ("xfrm/compat: Attach xfrm dumps to 64=>32 bit translator")
Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Co-developed-by: David Lee <david.lee@trailofbits.com>
Signed-off-by: David Lee <david.lee@trailofbits.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 980dbeb5a57d..a2587c7e796b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1877,7 +1877,6 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net *net = sock_net(skb->sk);
 	struct xfrm_state *x;
 	struct xfrm_userspi_info *p;
-	struct xfrm_translator *xtr;
 	struct sk_buff *resp_skb;
 	xfrm_address_t *daddr;
 	int family;
@@ -1943,17 +1942,6 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
-	xtr = xfrm_get_translator();
-	if (xtr) {
-		err = xtr->alloc_compat(skb, nlmsg_hdr(skb));
-
-		xfrm_put_translator(xtr);
-		if (err) {
-			kfree_skb(resp_skb);
-			goto out;
-		}
-	}
-
 	err = nlmsg_unicast(xfrm_net_nlsk(net, skb), resp_skb, NETLINK_CB(skb).portid);
 
 out:
-- 
2.43.0

[PATCH 07/12] xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

[Steffen Klassert]

From: Eric Dumazet <edumazet@google.com>

syzbot reported a suspicious RCU usage warning in ip6_pkt_drop():

  WARNING: suspicious RCU usage in ip6_pkt_drop
  include/net/addrconf.h:389 suspicious rcu_dereference_check() usage!

  Call Trace:
   __in6_dev_get_safely include/net/addrconf.h:389 [inline]
   ip6_pkt_drop+0x596/0x610 net/ipv6/route.c:4620
   ip6_pkt_discard+0x1c/0x30 net/ipv6/route.c:4651
   xfrm_trans_reinject+0x324/0x630 net/xfrm/xfrm_input.c:806
   process_one_work kernel/workqueue.c:3322 [inline]
   process_scheduled_works+0xa8e/0x14e0 kernel/workqueue.c:3405
   worker_thread+0xa47/0xfb0 kernel/workqueue.c:3486

When commit 4f4920669d21 ("xfrm: Reinject transport-mode packets through
workqueue") converted xfrm_trans_reinject from a tasklet to a workqueue,
the reinjection loop ceased running in softirq context. Workqueue workers
run in process context where local_bh_disable() does not enter an RCU
read-side critical section under CONFIG_PREEMPT_RCU.

Because finish callbacks (such as ip6_rcv_finish) expect to run under an
RCU read lock (performing route lookups, l3mdev lookups, and accessing
RCU-protected data structures), invoking them in workqueue context without
rcu_read_lock() triggers RCU lockdep warnings.

Furthermore, packets queued to the workqueue via xfrm_trans_queue_net()
may carry non-refcounted (noref) dst entries (e.g. from ip_route_input_noref).
Additionally, on netdevice unregistration, dst_dev_put() replaces dst->dev
with blackhole_netdev, so dst entries do not keep skb->dev alive while
queued in the workqueue.

Fix these issues by:
1. Calling skb_dst_force(skb) in xfrm_trans_queue_net() while still in the
   caller's RCU section to ensure dst is reference-counted before queuing.
2. Holding a reference on skb->dev via dev_hold()/dev_put() across workqueue
   deferral so skb->dev remains valid during finish() callback processing.
3. Acquiring rcu_read_lock() around the finish callback invocation loop in
   xfrm_trans_reinject().

Fixes: 4f4920669d21 ("xfrm: Reinject transport-mode packets through workqueue")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Liu Jian <liujian56@huawei.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_input.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index eecab337bd0a..8f6109eada7e 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -800,12 +800,17 @@ static void xfrm_trans_reinject(struct work_struct *work)
 	spin_unlock_bh(&trans->queue_lock);
 
 	local_bh_disable();
+	rcu_read_lock();
 	while ((skb = __skb_dequeue(&queue))) {
 		struct net *net = XFRM_TRANS_SKB_CB(skb)->net;
+		struct net_device *dev = skb->dev;
 
 		XFRM_TRANS_SKB_CB(skb)->finish(net, NULL, skb);
+		if (dev)
+			dev_put(dev);
 		put_net(net);
 	}
+	rcu_read_unlock();
 	local_bh_enable();
 }
 
@@ -821,12 +826,18 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
 	if (skb_queue_len(&trans->queue) >= READ_ONCE(net_hotdata.max_backlog))
 		return -ENOBUFS;
 
+	if (skb_dst(skb) && !skb_dst_force(skb))
+		return -EHOSTUNREACH;
+
 	BUILD_BUG_ON(sizeof(struct xfrm_trans_cb) > sizeof(skb->cb));
 
 	hold_net = maybe_get_net(net);
 	if (!hold_net)
 		return -ENODEV;
 
+	if (skb->dev)
+		dev_hold(skb->dev);
+
 	XFRM_TRANS_SKB_CB(skb)->finish = finish;
 	XFRM_TRANS_SKB_CB(skb)->net = hold_net;
 	spin_lock_bh(&trans->queue_lock);
-- 
2.43.0

Re: [PATCH 07/12] xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

This adds rcu_read_lock() around the finish() callback loop in
xfrm_trans_reinject(), forces the dst to be refcounted in
xfrm_trans_queue_net(), and takes a reference on skb->dev across the
workqueue deferral.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index eecab337bd0a7..8f6109eada7ea 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -800,12 +800,17 @@ static void xfrm_trans_reinject(struct work_struct *work)
>  	spin_unlock_bh(&trans->queue_lock);
>  
>  	local_bh_disable();
> +	rcu_read_lock();
>  	while ((skb = __skb_dequeue(&queue))) {
>  		struct net *net = XFRM_TRANS_SKB_CB(skb)->net;
> +		struct net_device *dev = skb->dev;
>  
>  		XFRM_TRANS_SKB_CB(skb)->finish(net, NULL, skb);
> +		if (dev)
> +			dev_put(dev);
>  		put_net(net);
>  	}
> +	rcu_read_unlock();
>  	local_bh_enable();
>  }
>  
> @@ -821,12 +826,18 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
>  	if (skb_queue_len(&trans->queue) >= READ_ONCE(net_hotdata.max_backlog))
>  		return -ENOBUFS;
>  
> +	if (skb_dst(skb) && !skb_dst_force(skb))
> +		return -EHOSTUNREACH;
> +
>  	BUILD_BUG_ON(sizeof(struct xfrm_trans_cb) > sizeof(skb->cb));
>  
>  	hold_net = maybe_get_net(net);
>  	if (!hold_net)
>  		return -ENODEV;
>  
> +	if (skb->dev)
> +		dev_hold(skb->dev);

[Severity: High]
Is skb->dev guaranteed to be alive here for every caller of
xfrm_trans_queue_net()?

The three transport-input callers look fine: xfrm_input() holds
rcu_read_lock() around afinfo->transport_finish(), and espintcp_rcv()
sets skb->dev via dev_get_by_index_rcu() inside the same RCU section.

The ESPinTCP output caller looks different. esp_output_tail_tcp() in
net/ipv4/esp4.c (and its net/ipv6/esp6.c twin) only does:

	local_bh_disable();
	err = xfrm_trans_queue_net(xs_net(x), skb, esp_output_tcp_encap_cb);
	local_bh_enable();

There is no rcu_read_lock() here, and per this patch's own commit message
local_bh_disable() is not an RCU read-side critical section under
CONFIG_PREEMPT_RCU.

That path is also reachable asynchronously. esp_output_tail() registers
the completion with aead_request_set_callback(req, 0, esp_output_done,
skb), and when crypto_aead_encrypt() returns -EINPROGRESS,
esp_output_done() later calls esp_output_tail_tcp() from a cryptd worker
or driver completion context, long after the receive-side RCU section
ended.

For a forwarded packet, what is skb->dev at that point? ip_forward() ->
NF_HOOK(..., ip_forward_finish) -> dst_output() enters the xfrm bundle
output with skb->dev still pointing at the ingress netdevice:

net/ipv4/ip_forward.c:ip_forward_finish() {
	...
	skb_clear_tstamp(skb);
	return dst_output(net, sk, skb);
}

Nothing on that path resets skb->dev (net/xfrm/xfrm_output.c only assigns
skb->dev on the HW-offload xmit path and in the IPv6 pmtu error path), and
nothing holds a reference on the ingress device - the skb only records
skb_iif, and the bundle dst references the egress device.

So if the ingress device is unregistered and freed while the request sits
in the crypto queue, does dev_hold(skb->dev) here increment a refcount in
freed memory, with the matching dev_put() in xfrm_trans_reinject() writing
to it a second time? With CONFIG_PCPU_DEV_REFCNT=n this would also hit
refcount_warn_saturate() on increment-from-zero.

There is a second aspect: esp_output_tcp_encap_cb() only uses
skb_dst(skb)->xfrm and never touches skb->dev, so the hold seems to be
unnecessary for that caller. Would it make sense to take the reference
only in the callers where skb->dev is provably live and the finish()
callback needs it - for example recording the held device in struct
xfrm_trans_cb - rather than holding unconditionally in the shared helper?

Re: [PATCH 07/12] xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:48:57PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> xfrm: add missing rcu_read_lock(), skb_dst_force() and dev_hold() for xfrm_trans_reinject()
> 
> This adds rcu_read_lock() around the finish() callback loop in
> xfrm_trans_reinject(), forces the dst to be refcounted in
> xfrm_trans_queue_net(), and takes a reference on skb->dev across the
> workqueue deferral.
> 
> > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > index eecab337bd0a7..8f6109eada7ea 100644
> > --- a/net/xfrm/xfrm_input.c
> > +++ b/net/xfrm/xfrm_input.c
> > @@ -800,12 +800,17 @@ static void xfrm_trans_reinject(struct work_struct *work)
> >  	spin_unlock_bh(&trans->queue_lock);
> >  
> >  	local_bh_disable();
> > +	rcu_read_lock();
> >  	while ((skb = __skb_dequeue(&queue))) {
> >  		struct net *net = XFRM_TRANS_SKB_CB(skb)->net;
> > +		struct net_device *dev = skb->dev;
> >  
> >  		XFRM_TRANS_SKB_CB(skb)->finish(net, NULL, skb);
> > +		if (dev)
> > +			dev_put(dev);
> >  		put_net(net);
> >  	}
> > +	rcu_read_unlock();
> >  	local_bh_enable();
> >  }
> >  
> > @@ -821,12 +826,18 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
> >  	if (skb_queue_len(&trans->queue) >= READ_ONCE(net_hotdata.max_backlog))
> >  		return -ENOBUFS;
> >  
> > +	if (skb_dst(skb) && !skb_dst_force(skb))
> > +		return -EHOSTUNREACH;
> > +
> >  	BUILD_BUG_ON(sizeof(struct xfrm_trans_cb) > sizeof(skb->cb));
> >  
> >  	hold_net = maybe_get_net(net);
> >  	if (!hold_net)
> >  		return -ENODEV;
> >  
> > +	if (skb->dev)
> > +		dev_hold(skb->dev);
> 
> [Severity: High]
> Is skb->dev guaranteed to be alive here for every caller of
> xfrm_trans_queue_net()?
> 
> The three transport-input callers look fine: xfrm_input() holds
> rcu_read_lock() around afinfo->transport_finish(), and espintcp_rcv()
> sets skb->dev via dev_get_by_index_rcu() inside the same RCU section.

The patch fixes a syzbot report in the input path and is corrext as is.

> 
> The ESPinTCP output caller looks different. esp_output_tail_tcp() in
> net/ipv4/esp4.c (and its net/ipv6/esp6.c twin) only does:
> 
> 	local_bh_disable();
> 	err = xfrm_trans_queue_net(xs_net(x), skb, esp_output_tcp_encap_cb);
> 	local_bh_enable();
> 
> There is no rcu_read_lock() here, and per this patch's own commit message
> local_bh_disable() is not an RCU read-side critical section under
> CONFIG_PREEMPT_RCU.
> 
> That path is also reachable asynchronously. esp_output_tail() registers
> the completion with aead_request_set_callback(req, 0, esp_output_done,
> skb), and when crypto_aead_encrypt() returns -EINPROGRESS,
> esp_output_done() later calls esp_output_tail_tcp() from a cryptd worker
> or driver completion context, long after the receive-side RCU section
> ended.
> 
> For a forwarded packet, what is skb->dev at that point? ip_forward() ->
> NF_HOOK(..., ip_forward_finish) -> dst_output() enters the xfrm bundle
> output with skb->dev still pointing at the ingress netdevice:
> 
> net/ipv4/ip_forward.c:ip_forward_finish() {
> 	...
> 	skb_clear_tstamp(skb);
> 	return dst_output(net, sk, skb);
> }
> 
> Nothing on that path resets skb->dev (net/xfrm/xfrm_output.c only assigns
> skb->dev on the HW-offload xmit path and in the IPv6 pmtu error path), and
> nothing holds a reference on the ingress device - the skb only records
> skb_iif, and the bundle dst references the egress device.
> 
> So if the ingress device is unregistered and freed while the request sits
> in the crypto queue, does dev_hold(skb->dev) here increment a refcount in
> freed memory, with the matching dev_put() in xfrm_trans_reinject() writing
> to it a second time? With CONFIG_PCPU_DEV_REFCNT=n this would also hit
> refcount_warn_saturate() on increment-from-zero.
> 
> There is a second aspect: esp_output_tcp_encap_cb() only uses
> skb_dst(skb)->xfrm and never touches skb->dev, so the hold seems to be
> unnecessary for that caller. Would it make sense to take the reference
> only in the callers where skb->dev is provably live and the finish()
> callback needs it - for example recording the held device in struct
> xfrm_trans_cb - rather than holding unconditionally in the shared helper?

What is described here is in the output path, this needs separate
investigation.

[PATCH 08/12] xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

[Steffen Klassert]

From: Siwei Zhang <fourdizhang@tencent.com>

Commit 14acf9652e56 ("xfrm: defensively unhash xfrm_state lists in
__xfrm_state_delete") converted bydst/bysrc/byseq/byspi from
hlist_del_rcu() to hlist_del_init_rcu() so that a second
__xfrm_state_delete() on the same object becomes a no-op rather than a
write through LIST_POISON pprev. It missed state_cache and
state_cache_input, which kept hlist_del_rcu():

- hlist_del_rcu() leaves pprev = LIST_POISON2 (non-NULL), so
  hlist_unhashed() returns false.
- hlist_del_init_rcu() leaves pprev = NULL, so hlist_unhashed()
  returns true.

A second __xfrm_state_delete() therefore enters __hlist_del() on the
already-deleted state_cache/state_cache_input nodes and does
WRITE_ONCE(*pprev, next) through LIST_POISON2 — a write use-after-free
once the slab is reused. The corruption can in turn cause a subsequent
hlist_for_each_entry_rcu traversal to follow a dangling next pointer,
producing the read use-after-free reported in xfrm_input_state_lookup().

Switch state_cache and state_cache_input to hlist_del_init_rcu() to
match the other four lists, closing the write use-after-free and, with
it, the read use-after-free it spawns.

Assisted-by: CodeBuddy:GLM-5.2
Fixes: 0045e3d80613 ("xfrm: Cache used outbound xfrm states at the policy.")
Fixes: 81a331a0e72d ("xfrm: Add an inbound percpu state cache.")
Cc: stable@vger.kernel.org
Signed-off-by: Siwei Zhang <fourdizhang@tencent.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index de097bba803b..e45aa1ed5b96 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -826,9 +826,9 @@ int __xfrm_state_delete(struct xfrm_state *x)
 		if (!hlist_unhashed(&x->byseq))
 			hlist_del_init_rcu(&x->byseq);
 		if (!hlist_unhashed(&x->state_cache))
-			hlist_del_rcu(&x->state_cache);
+			hlist_del_init_rcu(&x->state_cache);
 		if (!hlist_unhashed(&x->state_cache_input))
-			hlist_del_rcu(&x->state_cache_input);
+			hlist_del_init_rcu(&x->state_cache_input);
 
 		if (!hlist_unhashed(&x->byspi))
 			hlist_del_init_rcu(&x->byspi);
-- 
2.43.0

Re: [PATCH 08/12] xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: use hlist_del_init_rcu for state_cache and state_cache_input

This switches the two remaining hlist_del_rcu() calls in
__xfrm_state_delete() (state_cache and state_cache_input) to
hlist_del_init_rcu(), so that all six unhash sites in that function leave
pprev == NULL.  The changelog describes this as closing a write
use-after-free that in turn produces a read use-after-free in
xfrm_input_state_lookup().

There are no Link: tags in the commit header.

Two questions below, one about the changelog and one about whether the
state_cache half of the change can have any effect.

> A second __xfrm_state_delete() therefore enters __hlist_del() on the
> already-deleted state_cache/state_cache_input nodes and does
> WRITE_ONCE(*pprev, next) through LIST_POISON2 — a write use-after-free
> once the slab is reused. The corruption can in turn cause a subsequent
> hlist_for_each_entry_rcu traversal to follow a dangling next pointer,
> producing the read use-after-free reported in xfrm_input_state_lookup().

[Severity: High]
Can a second __xfrm_state_delete() reach the unhash block at all?  The
whole body is gated on km.state, and the gate sets DEAD as its first
action:

net/xfrm/xfrm_state.c:__xfrm_state_delete() {
	if (x->km.state != XFRM_STATE_DEAD) {
		x->km.state = XFRM_STATE_DEAD;

		spin_lock(&net->xfrm.xfrm_state_lock);
		list_del(&x->km.all);
		hlist_del_init_rcu(&x->bydst);
		...
}

All four callers hold x->lock across that test-and-set:

	xfrm_state_delete()      spin_lock_bh(&x->lock) ... spin_unlock_bh()
	xfrm_timer_handler()     spin_lock(&x->lock), and also returns early
	                         on x->km.state == XFRM_STATE_DEAD
	xfrm_add_sa_expire()     spin_lock_bh(&x->lock) (net/xfrm/xfrm_user.c)
	xfrm_do_migrate_state()  spin_lock_bh(&x->lock) (net/xfrm/xfrm_user.c)

and no path resets km.state away from DEAD.  Wouldn't a second call
therefore return -ESRCH before touching any hlist?

Separately, is the described mechanism the one that would occur?  If the
gate were somehow bypassed, __hlist_del()'s very first store goes through
the poisoned pprev:

include/linux/list.h:__hlist_del() {
	struct hlist_node *next = n->next;
	struct hlist_node **pprev = n->pprev;

	WRITE_ONCE(*pprev, next);
	if (next)
		WRITE_ONCE(next->pprev, pprev);
}

Since hlist_del_rcu() leaves pprev == LIST_POISON2, which is a fixed
illegal address, isn't the result an immediate oops rather than "a write
use-after-free once the slab is reused"?  And as the store to
next->pprev is never reached, how would this corrupt a live chain so that
a later hlist_for_each_entry_rcu() follows a dangling next pointer?

If the read use-after-free in xfrm_input_state_lookup() is real, it looks
like it has some other cause (an xfrm_state still reachable from an RCU
list after being freed), which this change does not address.  Given the
two Fixes: tags and Cc: stable, could the changelog be reworded to
describe the change as consistency hardening, and could a Reported-by:,
Closes: or the KASAN splat/reproducer be included so the actual lifetime
bug can be tracked?

One more note on the stated threat model: if a double
__xfrm_state_delete() were possible, list_del(&x->km.all),
net->xfrm.state_num-- and xfrm_state_put(x) in the same body are still
non-idempotent, so would making only the hlist removals idempotent be
sufficient?  Turning the poisoned-pprev write into a silent no-op also
removes the oops that would point at the real problem.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index de097bba803b7..e45aa1ed5b965 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -826,9 +826,9 @@ int __xfrm_state_delete(struct xfrm_state *x)
>  		if (!hlist_unhashed(&x->byseq))
>  			hlist_del_init_rcu(&x->byseq);
>  		if (!hlist_unhashed(&x->state_cache))
> -			hlist_del_rcu(&x->state_cache);
> +			hlist_del_init_rcu(&x->state_cache);

[Severity: Medium]
Can !hlist_unhashed(&x->state_cache) ever be true here?  The only site in
the tree that hashes x->state_cache is at the tail of xfrm_state_find():

net/xfrm/xfrm_state.c:xfrm_state_find() {
	if (x && x->km.state == XFRM_STATE_VALID && !cached &&
	    (!(pol->flags & XFRM_POLICY_CPU_ACQUIRE) || x->pcpu_num == pcpu_id)) {
		spin_lock_bh(&net->xfrm.xfrm_state_lock);
		if (hlist_unhashed(&x->state_cache))
			hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list);
		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
	}

but cached is set at a label that is also the fall-through of the second
state_cache_list loop:

cached:
	cached = true;
	if (best)
		goto found;
	else if (error)
		best = NULL;

The only jump to that label is the cache-hit case "if (best) goto cached;"
after the first loop, and all other paths simply fall into it, so isn't
cached unconditionally true by the time the tail check runs?  That would
make the hlist_add_head_rcu(&x->state_cache, &pol->state_cache_list) call
unreachable, leave pol->state_cache_list permanently empty (the loop in
xfrm_policy_kill() then also always walks an empty list), and mean the
guard above can never be true.

The cached = true placement comes from 0045e3d80613 ("xfrm: Cache used
outbound xfrm states at the policy.") rather than from this patch, but it
appears to make the state_cache half of this change inert and contradicts
the changelog's claim that the state_cache node can be double-deleted.
Would setting cached = true only on the cache-hit branch, instead of at a
label that is also fallen into, be the right fix?

>  		if (!hlist_unhashed(&x->state_cache_input))
> -			hlist_del_rcu(&x->state_cache_input);
> +			hlist_del_init_rcu(&x->state_cache_input);
>  
>  		if (!hlist_unhashed(&x->byspi))
>  			hlist_del_init_rcu(&x->byspi);

[PATCH 09/12] esp: downgrade zerocopy managed frags before mutating skb frags

[Steffen Klassert]

From: Maher Azzouzi <maherazz04@gmail.com>

On the out-of-place output path (esp->inplace == false) ESP rewrites the
skb frag array: esp_output_head() appends a trailer frag and
esp_output_tail() replaces the frags with a destination page, both
referenced with get_page().

When the skb carries zerocopy managed frags (SKBFL_MANAGED_FRAG_REFS) the
payload frags are owned by the ubuf and must not be referenced or
unreferenced individually, but ESP mutates the frag array without ever
downgrading the skb.  This breaks the managed-frag invariant two ways:

  - esp_ssg_unref() walks the source scatterlist and drops a page
    reference for every frag, including the ubuf-owned payload frags,
    pushing their refcount below the GUP pin bias while the pages are
    still pinned, i.e. a use-after-free of the zerocopy pages;

  - esp_output_tail() installs its destination page as frag 0 with
    get_page() but leaves SKBFL_MANAGED_FRAG_REFS set, so
    skb_release_data() takes the skip_unref branch and never drops that
    reference, leaking the x->xfrag page at packet rate.

Fix this the way every other frag-mutating site does (__ip_append_data(),
__ip6_append_data(), tcp_sendmsg_locked()) and call
skb_zcopy_downgrade_managed() before ESP touches the frag array: it takes
a real reference on each existing frag and clears SKBFL_MANAGED_FRAG_REFS,
so the per-frag unref in esp_ssg_unref() and the frag release in
skb_release_data() are both balanced and no mixed-ownership frag array is
left behind.

Fixes: 753f1ca4e1e5 ("net: introduce managed frags infrastructure")
Signed-off-by: Maher Azzouzi <maherazz04@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/esp4.c | 6 ++++++
 net/ipv6/esp6.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index dfc81ee969ae..faa48f5b9739 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -441,6 +441,12 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
 
 			esp->inplace = false;
 
+			/* Take real page refs and clear SKBFL_MANAGED_FRAG_REFS before
+			 * we mutate the frag array, so the per-frag unref stays balanced
+			 * for zerocopy managed frags (see __ip_append_data()).
+			 */
+			skb_zcopy_downgrade_managed(skb);
+
 			allocsize = ALIGN(tailen, L1_CACHE_BYTES);
 
 			spin_lock_bh(&x->lock);
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 296b57926abb..a3a3857eed98 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -470,6 +470,12 @@ int esp6_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info
 
 			esp->inplace = false;
 
+			/* Take real page refs and clear SKBFL_MANAGED_FRAG_REFS before
+			 * we mutate the frag array, so the per-frag unref stays balanced
+			 * for zerocopy managed frags (see __ip_append_data()).
+			 */
+			skb_zcopy_downgrade_managed(skb);
+
 			allocsize = ALIGN(tailen, L1_CACHE_BYTES);
 
 			spin_lock_bh(&x->lock);
-- 
2.43.0

Re: [PATCH 09/12] esp: downgrade zerocopy managed frags before mutating skb frags

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
esp: downgrade zerocopy managed frags before mutating skb frags

This adds a skb_zcopy_downgrade_managed() call to the out-of-place ESP
output path in esp_output_head() and esp6_output_head(), so that the frag
array is no longer mutated while SKBFL_MANAGED_FRAG_REFS is set.  Real
page references are taken on the existing frags before ESP appends its
trailer frag.

> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index a6c18aea7498d..e76db5817e78e 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -441,6 +441,12 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
>  
>  			esp->inplace = false;
>  
> +			/* Take real page refs and clear SKBFL_MANAGED_FRAG_REFS before
> +			 * we mutate the frag array, so the per-frag unref stays balanced
> +			 * for zerocopy managed frags (see __ip_append_data()).
> +			 */
> +			skb_zcopy_downgrade_managed(skb);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the sibling xfrm
transform need the same treatment?  ipcomp_post_acomp() in
net/xfrm/xfrm_ipcomp.c rewrites the frag array too, and there is no
skb_zcopy_downgrade_managed() anywhere under net/xfrm/.

ipcomp_setup_req() has a fast path that deliberately avoids any cow, so
the implicit downgrade inside pskb_expand_head() never runs:

net/xfrm/xfrm_ipcomp.c:ipcomp_setup_req() {
	...
	if (!skb_cloned(skb) && skb_headlen(skb) >= minhead) {
		if (!skb_is_nonlinear(skb)) {
			nfrags = 1;
			break;
		} else if (!skb_has_frag_list(skb)) {
			nfrags = skb_shinfo(skb)->nr_frags;
			nfrags++;
			break;
		}
	}
	...
}

ipcomp_post_acomp() then drops the old frags and installs its own pages:

net/xfrm/xfrm_ipcomp.c:ipcomp_post_acomp() {
	...
	pskb_trim_unique(skb, 0);
	__skb_put(skb, hlen);
	...
		skb_frag_fill_page_desc(frag, page, 0, len);

		skb_shinfo(skb)->nr_frags++;
	} while ((dlen -= len));
	...
}

Since the skb is not cloned here, ___pskb_trim() takes the drop_pages
path, and skb_frag_unref() is a no-op while SKBFL_MANAGED_FRAG_REFS is
set, so the flag survives the trim.  The pages installed afterwards come
from alloc_page() in ipcomp_setup_req(), i.e. they carry real references.

At release time skb_release_data() then skips the unref loop:

net/core/skbuff.c:skb_release_data() {
	...
	if (skb_zcopy(skb)) {
		bool skip_unref = shinfo->flags & SKBFL_MANAGED_FRAG_REFS;

		skb_zcopy_clear(skb, true);
		if (skip_unref)
			goto free_head;
	}
	...
}

Does this leak the compression output pages, one or more order-0 pages per
transmitted IPComp packet?  That looks like the same failure mode the
commit message describes for ESP ("leaking the x->xfrag page at packet
rate"), just reached through IPComp instead.

[Severity: Medium]
This is a pre-existing issue, but the patch adds a second irreversible
side effect at the same point, so it may be worth handling here.

esp->inplace = false is committed (and now the downgrade as well) before
the allocation that can fail:

net/ipv4/esp4.c:esp_output_head() {
	...
			esp->inplace = false;
	...
			skb_zcopy_downgrade_managed(skb);

			allocsize = ALIGN(tailen, L1_CACHE_BYTES);

			spin_lock_bh(&x->lock);

			if (unlikely(!skb_page_frag_refill(allocsize, pfrag, GFP_ATOMIC))) {
				spin_unlock_bh(&x->lock);
				goto cow;
			}
	...
}

On the refill failure, should esp->inplace be restored to true before the
goto cow?

The cow path calls skb_cow_data(), which pulls all paged data into the
linear area via __pskb_pull_tail(), and since the branch required
!skb_has_frag_list(skb) it returns with *trailer == skb.  pskb_put() then
only extends the linear region, so the skb leaves esp_output_head() with
nr_frags == 0 and data_len == 0.

esp_output_tail() still branches only on esp->inplace:

net/ipv4/esp4.c:esp_output_tail() {
	...
	if (!esp->inplace) {
		int allocsize;
		struct page_frag *pfrag = &x->xfrag;

		allocsize = ALIGN(skb->data_len, L1_CACHE_BYTES);
	...
		skb_shinfo(skb)->nr_frags = 1;
		page = pfrag->page;
		get_page(page);
		__skb_fill_page_desc(skb, 0, page, pfrag->offset, skb->data_len);
	...
}

With data_len == 0 the refill of size 0 succeeds trivially, nr_frags is
forced to 1 and a zero-length frag is installed with an extra page
reference, while pfrag->offset is not advanced.  Can this hand a zero
length fragment to the driver, and issue the out-of-place AEAD request
with src and dst scatterlists that both describe the same linear buffer?

>  			allocsize = ALIGN(tailen, L1_CACHE_BYTES);
>  
>  			spin_lock_bh(&x->lock);
> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index 72ec0d7d11209..b1c9b36f76dc4 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
> @@ -471,6 +471,12 @@ int esp6_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info
>  
>  			esp->inplace = false;
>  
> +			/* Take real page refs and clear SKBFL_MANAGED_FRAG_REFS before
> +			 * we mutate the frag array, so the per-frag unref stays balanced
> +			 * for zerocopy managed frags (see __ip_append_data()).
> +			 */
> +			skb_zcopy_downgrade_managed(skb);
> +

[Severity: Medium]
This is a pre-existing issue, but the same missing esp->inplace rollback
applies to the IPv6 path: esp6_output_head() commits esp->inplace = false
before the skb_page_frag_refill() that can goto cow, and esp6_output_tail()
branches only on esp->inplace.

For reference, net/ipv4/esp4.c and net/ipv6/esp6.c are unchanged in the
rest of this series, so the stale esp->inplace behaviour is still present
at its end.

>  			allocsize = ALIGN(tailen, L1_CACHE_BYTES);
>  
>  			spin_lock_bh(&x->lock);

Re: [PATCH 09/12] esp: downgrade zerocopy managed frags before mutating skb frags

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:49:00PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> esp: downgrade zerocopy managed frags before mutating skb frags
> 
> This adds a skb_zcopy_downgrade_managed() call to the out-of-place ESP
> output path in esp_output_head() and esp6_output_head(), so that the frag
> array is no longer mutated while SKBFL_MANAGED_FRAG_REFS is set.  Real
> page references are taken on the existing frags before ESP appends its
> trailer frag.
> 
> > diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> > index a6c18aea7498d..e76db5817e78e 100644
> > --- a/net/ipv4/esp4.c
> > +++ b/net/ipv4/esp4.c
> > @@ -441,6 +441,12 @@ int esp_output_head(struct xfrm_state *x, struct sk_buff *skb, struct esp_info *
> >  
> >  			esp->inplace = false;
> >  
> > +			/* Take real page refs and clear SKBFL_MANAGED_FRAG_REFS before
> > +			 * we mutate the frag array, so the per-frag unref stays balanced
> > +			 * for zerocopy managed frags (see __ip_append_data()).
> > +			 */
> > +			skb_zcopy_downgrade_managed(skb);
> > +
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the sibling xfrm
> transform need the same treatment?  ipcomp_post_acomp() in
> net/xfrm/xfrm_ipcomp.c rewrites the frag array too, and there is no
> skb_zcopy_downgrade_managed() anywhere under net/xfrm/.

This can be fixed with a followup patch.

[PATCH 10/12] xfrm: hold net_device reference under RCU in bundle creation

[Steffen Klassert]

From: "Cen Zhang (Microsoft Security FORGE Labs)" <blbllhy@gmail.com>

xfrm_bundle_create() and xfrm_create_dummy_bundle() read dst->dev into
a local pointer without taking a device reference, then pass it to
xfrm_fill_dst(). A concurrent RTM_DELLINK replaces dst->dev via
dst_dev_put() and frees the old net_device, causing a use-after-free
when xfrm6_fill_dst() later dereferences the stale dev pointer.

  BUG: KASAN: slab-use-after-free in xfrm6_fill_dst+0x82c/0x860
  (net/ipv6/xfrm6_policy.c:86 netdev_hold())
  Read of size 8 at addr ffff8880142fe588 by task exploit/153
  Call Trace:
   xfrm6_fill_dst+0x82c/0x860
   xfrm_resolve_and_create_bundle+0x21d4/0x2bd0
   xfrm_lookup_with_ifid+0x485/0x1640
   ip6_dst_lookup_flow+0x19b/0x1e0
   udpv6_sendmsg+0x1443/0x2dd0

Fix this by reading dst->dev via dst_dev_rcu() and keeping the RCU
read-side critical section active until xfrm_fill_dst() has taken the
required device references.

Fixes: 25ee3286dcbc ("[IPSEC]: Merge common code into xfrm_bundle_create")
Fixes: a0073fe18e71 ("xfrm: Add a state resolution packet queue")
Suggested-by: Steffen Klassert <steffen.klassert@secunet.com>
Reported-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Link: https://lore.kernel.org/all/20260820200245.44312-1-blbllhy@gmail.com/
Cc: AutonomousCodeSecurity@microsoft.com
Assisted-by: GitHub-Copilot:claude-opus-4.6
Signed-off-by: Cen Zhang (Microsoft Security FORGE Labs) <blbllhy@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 932a313b9460..513c9f228334 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2770,9 +2770,12 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	xdst0->path = dst;
 
 	err = -ENODEV;
-	dev = dst->dev;
-	if (!dev)
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (!dev) {
+		rcu_read_unlock();
 		goto free_dst;
+	}
 
 	xfrm_init_path(xdst0, dst, nfheader_len);
 	xfrm_init_pmtu(bundle, nx);
@@ -2780,8 +2783,10 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	for (xdst_prev = xdst0; xdst_prev != (struct xfrm_dst *)dst;
 	     xdst_prev = (struct xfrm_dst *) xfrm_dst_child(&xdst_prev->u.dst)) {
 		err = xfrm_fill_dst(xdst_prev, dev, fl);
-		if (err)
+		if (err) {
+			rcu_read_unlock();
 			goto free_dst;
+		}
 
 		xdst_prev->u.dst.header_len = header_len;
 		xdst_prev->u.dst.trailer_len = trailer_len;
@@ -2789,6 +2794,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		trailer_len -= xdst_prev->u.dst.xfrm->props.trailer_len;
 	}
 
+	rcu_read_unlock();
 	return &xdst0->u.dst;
 
 put_states:
@@ -3058,11 +3064,15 @@ static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net,
 	xfrm_init_path((struct xfrm_dst *)dst1, dst, 0);
 
 	err = -ENODEV;
-	dev = dst->dev;
-	if (!dev)
+	rcu_read_lock();
+	dev = dst_dev_rcu(dst);
+	if (!dev) {
+		rcu_read_unlock();
 		goto free_dst;
+	}
 
 	err = xfrm_fill_dst(xdst, dev, fl);
+	rcu_read_unlock();
 	if (err)
 		goto free_dst;
 
-- 
2.43.0

Re: [PATCH 10/12] xfrm: hold net_device reference under RCU in bundle creation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: hold net_device reference under RCU in bundle creation

This converts the dst->dev reads in xfrm_bundle_create() and
xfrm_create_dummy_bundle() to dst_dev_rcu() and keeps the RCU read-side
critical section open until xfrm_fill_dst() has taken the device
references, closing a use-after-free when a concurrent RTM_DELLINK runs
dst_dev_put().

Link: https://lore.kernel.org/all/20260820200245.44312-1-blbllhy@gmail.com/

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 932a313b9460a..513c9f2283347 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -2770,9 +2770,12 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
>  	xdst0->path = dst;
>  
>  	err = -ENODEV;
> -	dev = dst->dev;
> -	if (!dev)
> +	rcu_read_lock();
> +	dev = dst_dev_rcu(dst);
> +	if (!dev) {
> +		rcu_read_unlock();
>  		goto free_dst;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but should the same treatment be
applied to the other dst->dev reads in this file?  The caller in the reported
stack trace, xfrm_lookup_with_ifid(), still reads dst_orig->dev twice and
dereferences it on the nopol path with no rcu_read_lock() held:

net/xfrm/xfrm_policy.c:xfrm_lookup_with_ifid() {
	...
nopol:
	if ((!dst_orig->dev || !(dst_orig->dev->flags & IFF_LOOPBACK)) &&
	    READ_ONCE(net->xfrm.policy_default[dir]) == XFRM_USERPOLICY_BLOCK) {
		err = -EPERM;
		goto error;
	}
	...
}

dst_orig here is a plain route dst that the caller only holds a dst
reference on, and a dst reference does not pin dst->dev.  The writer side
publishes the replacement and drops the device reference together:

net/core/dst.c:dst_dev_put() {
	...
	rcu_assign_pointer(dst->dev_rcu, blackhole_netdev);
	netdev_ref_replace(dev, blackhole_netdev, &dst->dev_tracker,
			   GFP_ATOMIC);
	...
}

The old device is then freed after the synchronize_net() in
unregister_netdevice_many(), which only waits for readers inside an RCU
read-side critical section.  If this thread is preempted between the NULL
test on dst_orig->dev and the dev->flags load, can it read a freed
net_device the same way xfrm6_fill_dst() did?  The entry path is the one in
the commit message, udpv6_sendmsg() -> ip6_dst_lookup_flow() ->
xfrm_lookup_route() -> xfrm_lookup_with_ifid(), reaching nopol when
DST_NOXFRM is set or policy_count is zero.

Would dst_dev_rcu() under an rcu_read_lock() be appropriate here as well?

There is a second unannotated read of the same __rcu published pointer in
__xfrm_decode_session():

net/xfrm/xfrm_policy.c:__xfrm_decode_session() {
	...
		if (skb_dst(skb) && skb_dst(skb)->dev)
			oif = skb_dst(skb)->dev->ifindex;
	...
}

This bypasses the skb_dst_dev_rcu()/dst_dev_rcu() accessor contract and its
RCU coverage depends entirely on the caller.  Is that read guaranteed to run
inside an RCU read-side critical section on every path into
__xfrm_decode_session()?

For what it is worth, the dst->dev read in xfrm_bundle_ok() looks fine: the
dst there is an xfrm_dst whose ->dev holds its own tracked reference taken
by xfrm4_fill_dst()/xfrm6_fill_dst() via netdev_hold(), so it cannot be
freed while that xfrm dst is alive.

Re: [PATCH 10/12] xfrm: hold net_device reference under RCU in bundle creation

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:49:01PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> xfrm: hold net_device reference under RCU in bundle creation
> 
> This converts the dst->dev reads in xfrm_bundle_create() and
> xfrm_create_dummy_bundle() to dst_dev_rcu() and keeps the RCU read-side
> critical section open until xfrm_fill_dst() has taken the device
> references, closing a use-after-free when a concurrent RTM_DELLINK runs
> dst_dev_put().
> 
> Link: https://lore.kernel.org/all/20260820200245.44312-1-blbllhy@gmail.com/
> 
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 932a313b9460a..513c9f2283347 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -2770,9 +2770,12 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> >  	xdst0->path = dst;
> >  
> >  	err = -ENODEV;
> > -	dev = dst->dev;
> > -	if (!dev)
> > +	rcu_read_lock();
> > +	dev = dst_dev_rcu(dst);
> > +	if (!dev) {
> > +		rcu_read_unlock();
> >  		goto free_dst;
> > +	}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but should the same treatment be
> applied to the other dst->dev reads in this file?  The caller in the reported
> stack trace, xfrm_lookup_with_ifid(), still reads dst_orig->dev twice and
> dereferences it on the nopol path with no rcu_read_lock() held:

This is no regression introduced by this patch. Can be fixed with a
followup.

[PATCH 11/12] xfrm: save input state data before secpath resets

[Steffen Klassert]

From: Zhiling Zou <zhilinz@nebusec.ai>

xfrm_input() stores the current xfrm_state in the skb secpath while it
continues receive-side processing. Some input paths can reset that secpath
before xfrm_input() has finished dereferencing the state.

Receive callback users such as VTI and XFRM interfaces can reset the
secpath. The VTI receive path does so before checking whether the packet
crosses network namespaces, while the XFRM interface path does so only for
cross-network-namespace packets. The XFRM_MAX_DEPTH error path can also
reset the secpath before the final drop callback reports the current
state's protocol.

If secpath_reset() drops the last state reference while the state is
concurrently deleted, xfrm_input() can still dereference the freed state
when selecting transport_finish() or reporting the drop callback protocol.

Save the state protocol on the stack while the state is still valid,
and use the already saved address family for transport_finish(). A larval
XFRM_STATE_ACQ state has no type, so retain nexthdr as its protocol. This
preserves the existing drop-path fallback while avoiding the post-reset
state dereferences without adding an extra state reference to every
received packet.

Fixes: df3893c176e9 ("vti: Update the ipv4 side to use it's own receive hook.")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_input.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 8f6109eada7e..5ed87d51392a 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -474,6 +474,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	struct xfrm_state *x = NULL;
 	xfrm_address_t *daddr;
 	u32 mark = skb->mark;
+	u8 xfrm_proto = nexthdr;
 	unsigned int family = AF_UNSPEC;
 	int decaps = 0;
 	int async = 0;
@@ -485,6 +486,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	if (encap_type < 0 || (xo && (xo->flags & XFRM_GRO || encap_type == 0 ||
 				      encap_type == UDP_ENCAP_ESPINUDP))) {
 		x = xfrm_input_state(skb);
+		xfrm_proto = x->type ? x->type->proto : nexthdr;
 
 		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
 			if (x->km.state == XFRM_STATE_ACQ)
@@ -592,11 +594,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		x = xfrm_input_state_lookup(net, mark, daddr, spi, nexthdr, family);
 		if (x == NULL) {
+			xfrm_proto = nexthdr;
 			secpath_reset(skb);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOSTATES);
 			xfrm_audit_state_notfound(skb, family, spi, seq);
 			goto drop;
 		}
+		xfrm_proto = x->type ? x->type->proto : nexthdr;
 
 		if (unlikely(x->dir && x->dir != XFRM_SA_DIR_IN)) {
 			secpath_reset(skb);
@@ -604,6 +608,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			xfrm_audit_state_notfound(skb, family, spi, seq);
 			xfrm_state_put(x);
 			x = NULL;
+			xfrm_proto = nexthdr;
 			goto drop;
 		}
 
@@ -728,7 +733,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	} while (!err);
 
 	rcu_read_lock();
-	err = xfrm_rcv_cb(skb, family, x->type->proto, 0);
+	err = xfrm_rcv_cb(skb, family, xfrm_proto, 0);
 	if (err) {
 		rcu_read_unlock();
 		goto drop;
@@ -753,7 +758,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			xfrm_gro = xo->flags & XFRM_GRO;
 
 		err = -EAFNOSUPPORT;
-		afinfo = xfrm_state_afinfo_get_rcu(x->props.family);
+		afinfo = xfrm_state_afinfo_get_rcu(family);
 		if (likely(afinfo))
 			err = afinfo->transport_finish(skb, xfrm_gro || async);
 		if (xfrm_gro) {
@@ -776,7 +781,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 drop:
 	if (async)
 		dev_put(dev);
-	xfrm_rcv_cb(skb, family, x && x->type ? x->type->proto : nexthdr, -1);
+	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	kfree_skb(skb);
 	return 0;
 }
-- 
2.43.0

[PATCH 12/12] net: xfrm: reject unrepresentable espintcp transport headers

[Steffen Klassert]

From: Wyatt Feng <wf.kernel.dev@gmail.com>

ESP-in-TCP can hand xfrm packets whose transport header offset no longer
fits after the stream parser trims the TCP envelope. The plain transport
header reset truncates that offset and triggers the skb warning path.

Use the careful transport-header helper and drop the skb through the
existing XFRM error path when the offset cannot be represented.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:GPT-5.4
Signed-off-by: Wyatt Feng <wf.kernel.dev@gmail.com>
Signed-off-by: Ren Wei <weir@nebusec.ai>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/espintcp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index 674aedc5af5a..3e72b9f067b9 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c
@@ -30,7 +30,11 @@ static void handle_esp(struct sk_buff *skb, struct sock *sk)
 {
 	struct tcp_skb_cb *tcp_cb = (struct tcp_skb_cb *)skb->cb;
 
-	skb_reset_transport_header(skb);
+	if (!skb_reset_transport_header_careful(skb)) {
+		XFRM_INC_STATS(sock_net(sk), LINUX_MIB_XFRMINERROR);
+		kfree_skb(skb);
+		return;
+	}
 
 	/* restore IP CB, we need at least IP6CB->nhoff */
 	memmove(skb->cb, &tcp_cb->header, sizeof(tcp_cb->header));
-- 
2.43.0

Re: [PATCH 12/12] net: xfrm: reject unrepresentable espintcp transport headers

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: xfrm: reject unrepresentable espintcp transport headers

handle_esp() in net/xfrm/espintcp.c switches from the plain transport
header reset to skb_reset_transport_header_careful(), and drops the skb
with LINUX_MIB_XFRMINERROR when the offset does not fit in
skb->transport_header.

> diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> index 674aedc5af5a2..3e72b9f067b9b 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -30,7 +30,11 @@ static void handle_esp(struct sk_buff *skb, struct sock *sk)
>  {
>  	struct tcp_skb_cb *tcp_cb = (struct tcp_skb_cb *)skb->cb;
>  
> -	skb_reset_transport_header(skb);
> +	if (!skb_reset_transport_header_careful(skb)) {
> +		XFRM_INC_STATS(sock_net(sk), LINUX_MIB_XFRMINERROR);
> +		kfree_skb(skb);
> +		return;
> +	}

[Severity: High]
Is this gate placed early enough to cover the pulls that the ESP input
path performs on the same skb afterwards?

skb_reset_transport_header_careful() in include/linux/skbuff.h only
rejects offsets that are already unrepresentable:

	long offset = skb->data - skb->head;

	if (unlikely(offset != (typeof(skb->transport_header))offset))
		return false;

	if (unlikely(offset == (typeof(skb->transport_header))~0U))
		return false;

so any offset up to 65534 is accepted here with no margin left.

handle_esp() then calls xfrm4_rcv_encap()/xfrm6_rcv_encap(), and the
chain xfrm_input() -> esp_input() -> esp_input_done2() pulls the ESP
header and IV off the very same skb and resets the transport header with
the unhardened helpers, in net/ipv4/esp4.c:

	skb_pull_rcsum(skb, hlen);
	if (x->props.mode == XFRM_MODE_TUNNEL ||
	    x->props.mode == XFRM_MODE_IPTFS)
		skb_reset_transport_header(skb);
	else
		skb_set_transport_header(skb, -ihl);

with hlen = sizeof(struct ip_esp_hdr) + crypto_aead_ivsize(aead).
esp6_input_done2() in net/ipv6/esp6.c has the same shape for the
AF_INET6 branch of handle_esp().

For an skb whose head offset lands in the window [65536 - hlen, 65534],
the new check passes but the offset after skb_pull_rcsum() no longer
fits, so doesn't the truncation the changelog wants to eliminate simply
move downstream, now on a partially decrypted packet instead of a clean
drop?

Can such near-limit offsets be reached in practice?  espintcp_rcv() does:

	/* remove header, leave non-ESP marker/SPI */
	if (!pskb_pull(skb, rxm->offset + 2)) {

which makes skb->data - skb->head equal to the initial payload offset
plus rxm->offset + 2, and rxm->offset is bounded only by the size of one
coalesced TCP receive-queue skb.  If offsets at or above 65535 are
reachable (which is the premise of this patch), offsets a few bytes below
65536 look equally reachable, and the peer selects them at byte
granularity through the 16-bit espintcp length prefix.

For ESP transport mode there also seems to be a second-order effect.  With
k = (offset + hlen) - 65536 and k >= ihl, esp_input_done2() leaves
transport_header at (k - ihl), i.e. wrapped, and
xfrm4_transport_input()/xfrm6_transport_input() in net/xfrm/xfrm_input.c
consume it directly:

	int ihl = skb->data - skb_transport_header(skb);

	if (skb->transport_header != skb->network_header) {
		memmove(skb_transport_header(skb),
			skb_network_header(skb), ihl);
		...
	skb_reset_transport_header(skb);

Can ihl here become roughly 64 KiB and turn that memmove() into a copy
that runs past the packet data into the trailing skb_shared_info?  With
AES-CBC (ivsize 16, hlen 24) k can be 20..23, which is >= a 20-byte ihl.

Would it make sense either to give the espintcp check a margin covering
the largest downstream ESP header plus IV pull (or avoid the large linear
pull in espintcp_rcv() altogether), or to convert the esp4/esp6 and
xfrm_input transport-mode reset sites to the careful helpers as well?

Conversely, if offsets in that window cannot occur, what makes the check
added here reachable at all?

Re: [PATCH 12/12] net: xfrm: reject unrepresentable espintcp transport headers

[Steffen Klassert]

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:49:02PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: xfrm: reject unrepresentable espintcp transport headers
> 
> handle_esp() in net/xfrm/espintcp.c switches from the plain transport
> header reset to skb_reset_transport_header_careful(), and drops the skb
> with LINUX_MIB_XFRMINERROR when the offset does not fit in
> skb->transport_header.
> 
> > diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> > index 674aedc5af5a2..3e72b9f067b9b 100644
> > --- a/net/xfrm/espintcp.c
> > +++ b/net/xfrm/espintcp.c
> > @@ -30,7 +30,11 @@ static void handle_esp(struct sk_buff *skb, struct sock *sk)
> >  {
> >  	struct tcp_skb_cb *tcp_cb = (struct tcp_skb_cb *)skb->cb;
> >  
> > -	skb_reset_transport_header(skb);
> > +	if (!skb_reset_transport_header_careful(skb)) {
> > +		XFRM_INC_STATS(sock_net(sk), LINUX_MIB_XFRMINERROR);
> > +		kfree_skb(skb);
> > +		return;
> > +	}
> 
> [Severity: High]
> Is this gate placed early enough to cover the pulls that the ESP input
> path performs on the same skb afterwards?
> 
> skb_reset_transport_header_careful() in include/linux/skbuff.h only
> rejects offsets that are already unrepresentable:
> 
> 	long offset = skb->data - skb->head;
> 
> 	if (unlikely(offset != (typeof(skb->transport_header))offset))
> 		return false;
> 
> 	if (unlikely(offset == (typeof(skb->transport_header))~0U))
> 		return false;
> 
> so any offset up to 65534 is accepted here with no margin left.
> 
> handle_esp() then calls xfrm4_rcv_encap()/xfrm6_rcv_encap(), and the
> chain xfrm_input() -> esp_input() -> esp_input_done2() pulls the ESP
> header and IV off the very same skb and resets the transport header with
> the unhardened helpers, in net/ipv4/esp4.c:
> 
> 	skb_pull_rcsum(skb, hlen);
> 	if (x->props.mode == XFRM_MODE_TUNNEL ||
> 	    x->props.mode == XFRM_MODE_IPTFS)
> 		skb_reset_transport_header(skb);
> 	else
> 		skb_set_transport_header(skb, -ihl);
> 
> with hlen = sizeof(struct ip_esp_hdr) + crypto_aead_ivsize(aead).
> esp6_input_done2() in net/ipv6/esp6.c has the same shape for the
> AF_INET6 branch of handle_esp().
> 
> For an skb whose head offset lands in the window [65536 - hlen, 65534],
> the new check passes but the offset after skb_pull_rcsum() no longer
> fits, so doesn't the truncation the changelog wants to eliminate simply
> move downstream, now on a partially decrypted packet instead of a clean
> drop?
> 
> Can such near-limit offsets be reached in practice?  espintcp_rcv() does:
> 
> 	/* remove header, leave non-ESP marker/SPI */
> 	if (!pskb_pull(skb, rxm->offset + 2)) {
> 
> which makes skb->data - skb->head equal to the initial payload offset
> plus rxm->offset + 2, and rxm->offset is bounded only by the size of one
> coalesced TCP receive-queue skb.  If offsets at or above 65535 are
> reachable (which is the premise of this patch), offsets a few bytes below
> 65536 look equally reachable, and the peer selects them at byte
> granularity through the 16-bit espintcp length prefix.
> 
> For ESP transport mode there also seems to be a second-order effect.  With
> k = (offset + hlen) - 65536 and k >= ihl, esp_input_done2() leaves
> transport_header at (k - ihl), i.e. wrapped, and
> xfrm4_transport_input()/xfrm6_transport_input() in net/xfrm/xfrm_input.c
> consume it directly:
> 
> 	int ihl = skb->data - skb_transport_header(skb);
> 
> 	if (skb->transport_header != skb->network_header) {
> 		memmove(skb_transport_header(skb),
> 			skb_network_header(skb), ihl);
> 		...
> 	skb_reset_transport_header(skb);
> 
> Can ihl here become roughly 64 KiB and turn that memmove() into a copy
> that runs past the packet data into the trailing skb_shared_info?  With
> AES-CBC (ivsize 16, hlen 24) k can be 20..23, which is >= a 20-byte ihl.
> 
> Would it make sense either to give the espintcp check a margin covering
> the largest downstream ESP header plus IV pull (or avoid the large linear
> pull in espintcp_rcv() altogether), or to convert the esp4/esp6 and
> xfrm_input transport-mode reset sites to the careful helpers as well?
> 
> Conversely, if offsets in that window cannot occur, what makes the check
> added here reachable at all?

The patch did not add a regression, but the question if that
can be triggered at all is valid.

Wyatt can you explain how you tiggered this bug?

Some clarifications on the upstreaming process (was: [PATCH 0/12] pull request (net): ipsec 2026-09-07)

[Steffen Klassert]

Hi,

I need some clarifications on how to handle future ipsec patches.
Since we have the AI-generated patches and patch reviews things
changed quite a bit and I have problems to upstream ipsec fixes.

I have the following problematic situations:

1) Sashiko found nothing in the original patch submisstion,
but found an issue when I resent the patch with the pull
request. I think this can be solved by asking the author
to send an incremental fix on top of the ipsec tree if
it does not happen too often.

2) How to treat preexisting issues that are not introduced by the
patch under review? I'd say that's ok as long as the bug is
completely fixed with the patch. Is that acceptable?

3) Which severity is ok to accept? Maybe this:

- High, only if the review is wrong?
- Medium, only with good reson?
- Low, ok to accept?

4) Some patches for the ipsec and ipsec-next tree don't get Sashiko
reviews either because they don't apply to net or net-next, or
because of some other reasons I'm not aware of. This is the biggest
issue, I see the Sashiko review only after I sent a pull request.
This makes the upstreaming process complicated and delays fixes
quite a bit. I requested some infrastructure from the LF to get
this fixed, but no answer so far. Any other ideas how to fix
this issue?

I think 1-3 can be solved by an agreement on how to treat these
situations, but 4 needs to be fixed to keep the upstreaming
process working.

Re: Some clarifications on the upstreaming process

[Paolo Abeni]

On 9/9/26 8:38 AM, Steffen Klassert wrote:
> I need some clarifications on how to handle future ipsec patches.
> Since we have the AI-generated patches and patch reviews things
> changed quite a bit and I have problems to upstream ipsec fixes.
> 
> I have the following problematic situations:
> 
> 1) Sashiko found nothing in the original patch submisstion,
> but found an issue when I resent the patch with the pull
> request. I think this can be solved by asking the author
> to send an incremental fix on top of the ipsec tree if
> it does not happen too often.

This is usually the correct approach, with some exceptions i.e. the
newly found issue is impactful (the new code is exploitable).

> 2) How to treat preexisting issues that are not introduced by the
> patch under review? I'd say that's ok as long as the bug is
> completely fixed with the patch. Is that acceptable?

Yes, pre-existing issue are generally better handled as follow-up. With
some exception. i.e. sometimes sashiko says "this is a pre-existing
issue, but it looks like the path doesn't actually fix the pre-existing
issue it's supposed to fix..."

Or when the pre-existing issue is very strongly tied to the issue at hand.
> 3) Which severity is ok to accept? Maybe this:
> 
> - High, only if the review is wrong?
> - Medium, only with good reson?
> - Low, ok to accept?

AFAIK the above is the current general guidance.

Note that some brief comments on the ML in reply to sashiko feedback
would help in all the above mentioned cases.
> 4) Some patches for the ipsec and ipsec-next tree don't get Sashiko
> reviews either because they don't apply to net or net-next, or
> because of some other reasons I'm not aware of. This is the biggest
> issue, I see the Sashiko review only after I sent a pull request.
> This makes the upstreaming process complicated and delays fixes
> quite a bit. I requested some infrastructure from the LF to get
> this fixed, but no answer so far. Any other ideas how to fix
> this issue?

I think are 2 separate points:

4.1 missing sashiko reviews on edge cases
4.2 difficulty to reproduce the sashiko/clashiko review process in advance.

WRT 4.1 things should generally improve over time, with the exception of
patch that do not apply. I think we can't do much for them, but they
also should not matter much, right?

WRT 4.2 the current guidance is to run AI reviews before submission.
Sashiko could be installed and run locally. The nipa instance (clashiko)
is slighly more effective than sashiko.dev because it runs several
recent models and its result are indeed hard to replicate locally/in
advance.

Clashiko currently runs on (very significant) meta-sponsored budget, I
think it would be hard to extend it's usage to netdev's subsystems.

/P

Re: Some clarifications on the upstreaming process

[Matthieu Baerts]

Hi Steffen, Paolo,

(+cc Pablo, Florian)

Sorry to jump in the discussion, but I have similar issues with MPTCP
patches.

On 09/09/2026 11:23, Paolo Abeni wrote:
> On 9/9/26 8:38 AM, Steffen Klassert wrote:

(...)

>> 4) Some patches for the ipsec and ipsec-next tree don't get Sashiko
>> reviews either because they don't apply to net or net-next, or
>> because of some other reasons I'm not aware of.

From what I saw, sashiko.dev tries to apply patches on top of the
correct tree by at least looking at the modified files and the
MAINTAINERS file, and possibly the prefix [1] from what I understood.

For example this recent patch [2] got applied on top of 96f01b53c2d0,
which corresponds to today's 'ipset' tree [3] (tag: ipsec-2026-09-07)

[1] https://github.com/sashiko-dev/sashiko/issues/48
[2]
https://sashiko.dev/#/patchset/migrate-state-fixes-v2-0-c3e2767f0d96%40secunet.com
[3] https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git

For Clashiko, it only tests what's for net/net-next, same as for the
selftests, etc. if I'm not mistaken.

>> This is the biggest
>> issue, I see the Sashiko review only after I sent a pull request.
>> This makes the upstreaming process complicated and delays fixes
>> quite a bit. I requested some infrastructure from the LF to get
>> this fixed, but no answer so far. Any other ideas how to fix
>> this issue?
> 
> I think are 2 separate points:
> 
> 4.1 missing sashiko reviews on edge cases
> 4.2 difficulty to reproduce the sashiko/clashiko review process in advance.
> 
> WRT 4.1 things should generally improve over time, with the exception of
> patch that do not apply. I think we can't do much for them, but they
> also should not matter much, right?
> 
> WRT 4.2 the current guidance is to run AI reviews before submission.
> Sashiko could be installed and run locally. The nipa instance (clashiko)
> is slighly more effective than sashiko.dev because it runs several
> recent models and its result are indeed hard to replicate locally/in
> advance.
> 
> Clashiko currently runs on (very significant) meta-sponsored budget, I
> think it would be hard to extend it's usage to netdev's subsystems.

We might need to find a solution for the subsystems for this 4th point.
I have the same issue with MPTCP, and it seems it is the same with
Netfilter (and likely others) from what I saw. I would prefer to have
Clashiko reviews before applying patches on my side: to reduce the risk
to deal with new issues later on, and to let the author dealing with
that (instead of me days/weeks after).

From what I understood, Clashiko is still being tweaked, and that's the
current priority. Maybe later, subsystems can have their patches
reviewed by Clashiko as well?

If that's a budget issue that cannot be solved easily, I wonder if
Clashiko shouldn't ignore subsystems patches: I see its value, but I
also see the cost for the different subsystems :-/

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

Hi Matthieu,

On Wed, Sep 09, 2026 at 12:22:14PM +0200, Matthieu Baerts wrote:
> Hi Steffen, Paolo,
> 
> (+cc Pablo, Florian)
> 
> Sorry to jump in the discussion, but I have similar issues with MPTCP
> patches.
> 
> On 09/09/2026 11:23, Paolo Abeni wrote:
> > On 9/9/26 8:38 AM, Steffen Klassert wrote:
> 
> (...)
> 
> >> 4) Some patches for the ipsec and ipsec-next tree don't get Sashiko
> >> reviews either because they don't apply to net or net-next, or
> >> because of some other reasons I'm not aware of.
> 
> >From what I saw, sashiko.dev tries to apply patches on top of the
> correct tree by at least looking at the modified files and the
> MAINTAINERS file, and possibly the prefix [1] from what I understood.

Yes, that's correct. I did not notice this yet. Thanks!

> 
> For example this recent patch [2] got applied on top of 96f01b53c2d0,
> which corresponds to today's 'ipset' tree [3] (tag: ipsec-2026-09-07)
> 
> [1] https://github.com/sashiko-dev/sashiko/issues/48
> [2]
> https://sashiko.dev/#/patchset/migrate-state-fixes-v2-0-c3e2767f0d96%40secunet.com
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git
> 
> For Clashiko, it only tests what's for net/net-next, same as for the
> selftests, etc. if I'm not mistaken.
> 
> >> This is the biggest
> >> issue, I see the Sashiko review only after I sent a pull request.
> >> This makes the upstreaming process complicated and delays fixes
> >> quite a bit. I requested some infrastructure from the LF to get
> >> this fixed, but no answer so far. Any other ideas how to fix
> >> this issue?
> > 
> > I think are 2 separate points:
> > 
> > 4.1 missing sashiko reviews on edge cases
> > 4.2 difficulty to reproduce the sashiko/clashiko review process in advance.
> > 
> > WRT 4.1 things should generally improve over time, with the exception of
> > patch that do not apply. I think we can't do much for them, but they
> > also should not matter much, right?
> > 
> > WRT 4.2 the current guidance is to run AI reviews before submission.
> > Sashiko could be installed and run locally. The nipa instance (clashiko)
> > is slighly more effective than sashiko.dev because it runs several
> > recent models and its result are indeed hard to replicate locally/in
> > advance.
> > 
> > Clashiko currently runs on (very significant) meta-sponsored budget, I
> > think it would be hard to extend it's usage to netdev's subsystems.
> 
> We might need to find a solution for the subsystems for this 4th point.
> I have the same issue with MPTCP, and it seems it is the same with
> Netfilter (and likely others) from what I saw. I would prefer to have
> Clashiko reviews before applying patches on my side: to reduce the risk
> to deal with new issues later on, and to let the author dealing with
> that (instead of me days/weeks after).

Right, patches must get the review before they get applied to
a git tree, not after. It was always like that and we should
try to get back to it. The current workflow feels broken from
a submaintainers point of view. The submaintaners are now the
bottleneck.

> >From what I understood, Clashiko is still being tweaked, and that's the
> current priority. Maybe later, subsystems can have their patches
> reviewed by Clashiko as well?

That's what I would hope for.

> If that's a budget issue that cannot be solved easily, I wonder if
> Clashiko shouldn't ignore subsystems patches: I see its value, but I
> also see the cost for the different subsystems :-/

Maybe we can work with a compromise in the meantime. Clashiko
reviews all subsystem patches that apply to net or net-next
when the patches are submitted to the list as it is now.
But then the subsystem pull requests are done without
resending all patches to the list. So patches do not get
reviewed again with the pull request. That would avoid the
hassle with reviews of already applied patches.

Would that approach be acceptable for the netdev maintainers?

Re: Some clarifications on the upstreaming process

[Matthieu Baerts]

Hi Steffen,

On 10/09/2026 10:17, Steffen Klassert wrote:
> On Wed, Sep 09, 2026 at 12:22:14PM +0200, Matthieu Baerts wrote:
>> On 09/09/2026 11:23, Paolo Abeni wrote:
>>> On 9/9/26 8:38 AM, Steffen Klassert wrote:

(...)

>>>> This is the biggest
>>>> issue, I see the Sashiko review only after I sent a pull request.
>>>> This makes the upstreaming process complicated and delays fixes
>>>> quite a bit. I requested some infrastructure from the LF to get
>>>> this fixed, but no answer so far. Any other ideas how to fix
>>>> this issue?
>>>
>>> I think are 2 separate points:
>>>
>>> 4.1 missing sashiko reviews on edge cases
>>> 4.2 difficulty to reproduce the sashiko/clashiko review process in advance.
>>>
>>> WRT 4.1 things should generally improve over time, with the exception of
>>> patch that do not apply. I think we can't do much for them, but they
>>> also should not matter much, right?
>>>
>>> WRT 4.2 the current guidance is to run AI reviews before submission.
>>> Sashiko could be installed and run locally. The nipa instance (clashiko)
>>> is slighly more effective than sashiko.dev because it runs several
>>> recent models and its result are indeed hard to replicate locally/in
>>> advance.
>>>
>>> Clashiko currently runs on (very significant) meta-sponsored budget, I
>>> think it would be hard to extend it's usage to netdev's subsystems.
>>
>> We might need to find a solution for the subsystems for this 4th point.
>> I have the same issue with MPTCP, and it seems it is the same with
>> Netfilter (and likely others) from what I saw. I would prefer to have
>> Clashiko reviews before applying patches on my side: to reduce the risk
>> to deal with new issues later on, and to let the author dealing with
>> that (instead of me days/weeks after).
> 
> Right, patches must get the review before they get applied to
> a git tree, not after. It was always like that and we should
> try to get back to it. The current workflow feels broken from
> a submaintainers point of view. The submaintaners are now the
> bottleneck.

Indeed, same here.

>> >From what I understood, Clashiko is still being tweaked, and that's the
>> current priority. Maybe later, subsystems can have their patches
>> reviewed by Clashiko as well?
> 
> That's what I would hope for.
> 
>> If that's a budget issue that cannot be solved easily, I wonder if
>> Clashiko shouldn't ignore subsystems patches: I see its value, but I
>> also see the cost for the different subsystems :-/
> 
> Maybe we can work with a compromise in the meantime. Clashiko
> reviews all subsystem patches that apply to net or net-next
> when the patches are submitted to the list as it is now.
> But then the subsystem pull requests are done without
> resending all patches to the list. So patches do not get
> reviewed again with the pull request. That would avoid the
> hassle with reviews of already applied patches.

I guess that could work for you because ipsec specific patches are sent
to the same list, but not for subsystems with dedicated mailing lists.
In our case, new features and complex fixes are usually discussed there
first, with potentially multiple revisions, before being sent to netdev.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

On Thu, Sep 10, 2026 at 10:35:58AM +0200, Matthieu Baerts wrote:

...

> >> >From what I understood, Clashiko is still being tweaked, and that's the
> >> current priority. Maybe later, subsystems can have their patches
> >> reviewed by Clashiko as well?
> > 
> > That's what I would hope for.
> > 
> >> If that's a budget issue that cannot be solved easily, I wonder if
> >> Clashiko shouldn't ignore subsystems patches: I see its value, but I
> >> also see the cost for the different subsystems :-/
> > 
> > Maybe we can work with a compromise in the meantime. Clashiko
> > reviews all subsystem patches that apply to net or net-next
> > when the patches are submitted to the list as it is now.
> > But then the subsystem pull requests are done without
> > resending all patches to the list. So patches do not get
> > reviewed again with the pull request. That would avoid the
> > hassle with reviews of already applied patches.
> 
> I guess that could work for you because ipsec specific patches are sent
> to the same list, but not for subsystems with dedicated mailing lists.
> In our case, new features and complex fixes are usually discussed there
> first, with potentially multiple revisions, before being sent to netdev.

We have some (inofficial) mailing list for IPsec as well. We discuss
early stage RFC patches and concepts there. But the rule for IPsec
is that patches must be send to netdev at least once before they
get applied. Maybe you could use a similar worflow? I guess that
would make clashiko integration for subsystems easier, as it does
not need to monitor multiple mailing lists.

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

On Wed, Sep 09, 2026 at 11:23:56AM +0200, Paolo Abeni wrote:
> On 9/9/26 8:38 AM, Steffen Klassert wrote:
> > I need some clarifications on how to handle future ipsec patches.
> > Since we have the AI-generated patches and patch reviews things
> > changed quite a bit and I have problems to upstream ipsec fixes.
> > 
> > I have the following problematic situations:
> > 
> > 1) Sashiko found nothing in the original patch submisstion,
> > but found an issue when I resent the patch with the pull
> > request. I think this can be solved by asking the author
> > to send an incremental fix on top of the ipsec tree if
> > it does not happen too often.
> 
> This is usually the correct approach, with some exceptions i.e. the
> newly found issue is impactful (the new code is exploitable).

Are you willing to pull this as is, or should I resend the pull
request with the incremental fix? The latter leads to problem
4.1 (patch does not apply to net).

> 
> > 2) How to treat preexisting issues that are not introduced by the
> > patch under review? I'd say that's ok as long as the bug is
> > completely fixed with the patch. Is that acceptable?
> 
> Yes, pre-existing issue are generally better handled as follow-up. With
> some exception. i.e. sometimes sashiko says "this is a pre-existing
> issue, but it looks like the path doesn't actually fix the pre-existing
> issue it's supposed to fix..."o
> 
> Or when the pre-existing issue is very strongly tied to the issue at hand.
> > 3) Which severity is ok to accept? Maybe this:
> > 
> > - High, only if the review is wrong?
> > - Medium, only with good reson?
> > - Low, ok to accept?
> 
> AFAIK the above is the current general guidance.
> 
> Note that some brief comments on the ML in reply to sashiko feedback
> would help in all the above mentioned cases.

Ok, will do this.

> > 4) Some patches for the ipsec and ipsec-next tree don't get Sashiko
> > reviews either because they don't apply to net or net-next, or
> > because of some other reasons I'm not aware of. This is the biggest
> > issue, I see the Sashiko review only after I sent a pull request.
> > This makes the upstreaming process complicated and delays fixes
> > quite a bit. I requested some infrastructure from the LF to get
> > this fixed, but no answer so far. Any other ideas how to fix
> > this issue?
> 
> I think are 2 separate points:
> 
> 4.1 missing sashiko reviews on edge cases
> 4.2 difficulty to reproduce the sashiko/clashiko review process in advance.
> 
> WRT 4.1 things should generally improve over time, with the exception of
> patch that do not apply. I think we can't do much for them, but they
> also should not matter much, right?

Well, the 'patch does not apply' is one of my biggest problems.

> WRT 4.2 the current guidance is to run AI reviews before submission.
> Sashiko could be installed and run locally.

If somebody pays for the LLM tokens...

I don't have any influence on the patch author. But as most of
the patches are AI-generated, I'd guess they have some AI review
too. Unfortunately this does not mean they are correct.

> The nipa instance (clashiko)
> is slighly more effective than sashiko.dev because it runs several
> recent models and its result are indeed hard to replicate locally/in
> advance.
> 
> Clashiko currently runs on (very significant) meta-sponsored budget, I
> think it would be hard to extend it's usage to netdev's subsystems.

But that would fix the issue. Finally all the patches are reviewd by
Clashiko anyway when I send the pull request. If the nipa picks the
correct tree (ipsec or ipsec-next) the patches get reviewed already
when submitted and I could send the pull request without attaching
the patches. So this would be still one review per patch.

The current workflow makes me running in circles. I have no idea
how many iterations it needs to get a PR upstream, while the
queue of new patches continues to grow.

Re: Some clarifications on the upstreaming process

[Paolo Abeni]

On 9/9/26 12:23 PM, Steffen Klassert wrote:
> On Wed, Sep 09, 2026 at 11:23:56AM +0200, Paolo Abeni wrote:
>> On 9/9/26 8:38 AM, Steffen Klassert wrote:
>>> I need some clarifications on how to handle future ipsec patches.
>>> Since we have the AI-generated patches and patch reviews things
>>> changed quite a bit and I have problems to upstream ipsec fixes.
>>>
>>> I have the following problematic situations:
>>>
>>> 1) Sashiko found nothing in the original patch submisstion,
>>> but found an issue when I resent the patch with the pull
>>> request. I think this can be solved by asking the author
>>> to send an incremental fix on top of the ipsec tree if
>>> it does not happen too often.
>>
>> This is usually the correct approach, with some exceptions i.e. the
>> newly found issue is impactful (the new code is exploitable).
> 
> Are you willing to pull this as is, or should I resend the pull
> request with the incremental fix? The latter leads to problem
> 4.1 (patch does not apply to net).
Due to time schedule I can't look into the series detail before tomorrow
my time. Since Jakub forwarded the AI report and left the series in PW I
assume he is looking for your replies to the AI comments - which is
pretty much would I'll do.

/P

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

On Wed, Sep 09, 2026 at 12:34:42PM +0200, Paolo Abeni wrote:
> On 9/9/26 12:23 PM, Steffen Klassert wrote:
> > On Wed, Sep 09, 2026 at 11:23:56AM +0200, Paolo Abeni wrote:
> >> On 9/9/26 8:38 AM, Steffen Klassert wrote:
> >>> I need some clarifications on how to handle future ipsec patches.
> >>> Since we have the AI-generated patches and patch reviews things
> >>> changed quite a bit and I have problems to upstream ipsec fixes.
> >>>
> >>> I have the following problematic situations:
> >>>
> >>> 1) Sashiko found nothing in the original patch submisstion,
> >>> but found an issue when I resent the patch with the pull
> >>> request. I think this can be solved by asking the author
> >>> to send an incremental fix on top of the ipsec tree if
> >>> it does not happen too often.
> >>
> >> This is usually the correct approach, with some exceptions i.e. the
> >> newly found issue is impactful (the new code is exploitable).
> > 
> > Are you willing to pull this as is, or should I resend the pull
> > request with the incremental fix? The latter leads to problem
> > 4.1 (patch does not apply to net).
> Due to time schedule I can't look into the series detail before tomorrow
> my time. Since Jakub forwarded the AI report and left the series in PW I
> assume he is looking for your replies to the AI comments - which is
> pretty much would I'll do.

It was a general question, not directly related to this pull request.

There were several issues reported, it will take a bit of time
to review the AI comments.

Re: Some clarifications on the upstreaming process

[Jakub Kicinski]

On Wed, 9 Sep 2026 12:44:08 +0200 Steffen Klassert wrote:
> On Wed, Sep 09, 2026 at 12:34:42PM +0200, Paolo Abeni wrote:
> > On 9/9/26 12:23 PM, Steffen Klassert wrote:  
> > > Are you willing to pull this as is, or should I resend the pull
> > > request with the incremental fix? The latter leads to problem
> > > 4.1 (patch does not apply to net).  
> > Due to time schedule I can't look into the series detail before tomorrow
> > my time. Since Jakub forwarded the AI report and left the series in PW I
> > assume he is looking for your replies to the AI comments - which is
> > pretty much would I'll do.  
> 
> It was a general question, not directly related to this pull request.
> 
> There were several issues reported, it will take a bit of time
> to review the AI comments.

Thanks, I'll toss this version form PW, please let us know if it's good
(enough) and we should pull as is after all.

In general, I'd of course like to allow more use of the meta sponsored
models but my time is limited, and to some extent so are the funds (to
some extent because I suspect the volume is negligible compared to
netdev).

At the same time I worry that *shikos are making it easier for our
maintainers to ignore the fundamental changes which AI brings to SW
engineering. IOW I'd like to encourage more local use and
experimentation.

Sorry for not answering your actual questions. TBH I don't think we
have any real answers for most of them.

Re: Some clarifications on the upstreaming process

[Matthieu Baerts]

Hi Jakub,

On 09/09/2026 20:57, Jakub Kicinski wrote:

(...)

> At the same time I worry that *shikos are making it easier for our
> maintainers to ignore the fundamental changes which AI brings to SW
> engineering. IOW I'd like to encourage more local use and
> experimentation.

As an individual, I'm not sure whether it's a good idea for me to
support a local Clashiko myself. I would probably need to find
more/dedicated funding for that :-/

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

On Wed, Sep 09, 2026 at 11:57:53AM -0700, Jakub Kicinski wrote:
> On Wed, 9 Sep 2026 12:44:08 +0200 Steffen Klassert wrote:
> > On Wed, Sep 09, 2026 at 12:34:42PM +0200, Paolo Abeni wrote:
> > > On 9/9/26 12:23 PM, Steffen Klassert wrote:  
> > > > Are you willing to pull this as is, or should I resend the pull
> > > > request with the incremental fix? The latter leads to problem
> > > > 4.1 (patch does not apply to net).  
> > > Due to time schedule I can't look into the series detail before tomorrow
> > > my time. Since Jakub forwarded the AI report and left the series in PW I
> > > assume he is looking for your replies to the AI comments - which is
> > > pretty much would I'll do.  
> > 
> > It was a general question, not directly related to this pull request.
> > 
> > There were several issues reported, it will take a bit of time
> > to review the AI comments.
> 
> Thanks, I'll toss this version form PW, please let us know if it's good
> (enough) and we should pull as is after all.

I'll sort it out for that pull request, but I can't do that for every PR.

> In general, I'd of course like to allow more use of the meta sponsored
> models but my time is limited, and to some extent so are the funds (to
> some extent because I suspect the volume is negligible compared to
> netdev).

I'd guess it is negligible. It is one review per subsystem patch,
as it is now. It is just before the patch is applied. So I think
the actual cost is to set the things up.

> At the same time I worry that *shikos are making it easier for our
> maintainers to ignore the fundamental changes which AI brings to SW
> engineering. IOW I'd like to encourage more local use and
> experimentation.

We do local use and experimentation and yes, we could probably
do more. But that's a process and I have only limited influence
on that.

But anyway, this does not solve the root problem that patches
get their final review after they are applied to git (as
mentioned in the other mail in this thread).

> Sorry for not answering your actual questions. TBH I don't think we
> have any real answers for most of them.

Unfortunately we have to find an answer for these questions.
My time is limited as well, that's why I came up with that.
So I hope we find a solution before I have to raise the
white flag ;)

Re: Some clarifications on the upstreaming process

[Steffen Klassert]

On Wed, Sep 09, 2026 at 11:57:53AM -0700, Jakub Kicinski wrote:
> On Wed, 9 Sep 2026 12:44:08 +0200 Steffen Klassert wrote:
> > On Wed, Sep 09, 2026 at 12:34:42PM +0200, Paolo Abeni wrote:
> > > On 9/9/26 12:23 PM, Steffen Klassert wrote:  
> > > > Are you willing to pull this as is, or should I resend the pull
> > > > request with the incremental fix? The latter leads to problem
> > > > 4.1 (patch does not apply to net).  
> > > Due to time schedule I can't look into the series detail before tomorrow
> > > my time. Since Jakub forwarded the AI report and left the series in PW I
> > > assume he is looking for your replies to the AI comments - which is
> > > pretty much would I'll do.  
> > 
> > It was a general question, not directly related to this pull request.
> > 
> > There were several issues reported, it will take a bit of time
> > to review the AI comments.
> 
> Thanks, I'll toss this version form PW, please let us know if it's good
> (enough) and we should pull as is after all.

Most of the patches are OK, but I'll give the patch authors some time to
comment and resend the pull request then. I send just the pull request
without resending the patches to the list if that's OK. I don't want
to have yet another AI review round...