[PATCH net v8 1/3] xfrm: use skb->dev before dropping the reference held across async

[PATCH net v8 1/3] xfrm: use skb->dev before dropping the reference held across async

[Qihang]

The skb->dev reference that is held across an async resumption is
released before skb->dev is used, on two exits of xfrm_input():

  - the drop path runs the receive callbacks after the reference is
    dropped; xfrmi_rcv_cb() reads skb->dev->ifindex, so it can race a
    concurrent teardown of the device
  - the decaps success exit calls gro_cells_receive() after the
    reference is dropped; gro_cells_receive() reads skb->dev

Use skb->dev first and drop the reference afterwards in both cases,
matching what the transport exit already does.

The early drop for a state that is no longer valid releases the
reference on its own; let the drop label handle that reference too.

Fixes: 1c428b038400 ("xfrm: hold dev ref until after transport_finish NF_HOOK")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/xfrm/xfrm_input.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 5ed87d513..cfd5cea03 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -496,7 +496,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 					       LINUX_MIB_XFRMINSTATEINVALID);
 
 			if (encap_type == -1)
-				dev_put(dev);
+				async = 1;
 			goto drop;
 		}
 
@@ -747,9 +747,10 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			sp->olen = 0;
 		if (skb_valid_dst(skb))
 			skb_dst_drop(skb);
+		/* gro_cells_receive() reads skb->dev. */
+		gro_cells_receive(&gro_cells, skb);
 		if (async)
 			dev_put(dev);
-		gro_cells_receive(&gro_cells, skb);
 		rcu_read_unlock();
 		return 0;
 	} else {
@@ -779,9 +780,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 drop_unlock:
 	spin_unlock(&x->lock);
 drop:
+	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	if (async)
 		dev_put(dev);
-	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	kfree_skb(skb);
 	return 0;
 }
-- 
2.54.0 (Apple Git-157)

[PATCH net v8 2/3] xfrm: iptfs: hand the drop timer back to the reorder window

[Qihang]

__iptfs_reassem_done() cancels the drop timer whenever an in-progress
packet finishes reassembly, even if the reorder window still holds
out-of-order packets.  Those packets were saved with a drop time of their
own and the same timer is what ages them out.

If the missing packet never arrives, the window is never drained: the
queued packets are not delivered, and each one keeps the xfrm_state
reference it took from its secpath, so the state cannot be destroyed.
Nothing re-arms the timer to clean that up.  iptfs_set_window_drop_times()
only starts it when the walk back through the window reaches the empty
slot 0, and an occupied slot 0 stops the walk.

Cancel the timer as before and, if the window has entries, arm it again
for the drop time of slot 0.  Leaving the queued timer in place instead
would keep the expiry of the reassembly that just finished, which is
earlier than the window's own deadline; a reassembly started in between
would then not arm its timer and could be dropped before its budget ran
out.  Arming it for slot 0 keeps what iptfs_drop_timer() expects, one
deadline that belongs to the window.

Fixes: 6be02e3e4f376 ("xfrm: iptfs: handle reordering of received packets")
Reported-by: Lilly Aronleigh <lilly@aronleigh.au>
Link: https://lore.kernel.org/netdev/20260824072851.301644-3-lilly@aronleigh.au/
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/xfrm/xfrm_iptfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 0313a43aa..5ea3e77dd 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -708,6 +708,15 @@ static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
 
 	/* We don't care if it works locking takes care of things */
 	hrtimer_try_to_cancel(&xtfs->drop_timer);
+	/* The timer also ages out the reorder window, if it has any entries. */
+	if (xtfs->w_savedlen) {
+		time64_t now = ktime_get_raw_fast_ns();
+
+		hrtimer_start(&xtfs->drop_timer,
+			      xtfs->w_saved[0].drop_time - now,
+			      IPTFS_HRTIMER_MODE);
+	}
+
 	if (free)
 		kfree_skb(xtfs->ra_newskb);
 	xtfs->ra_newskb = NULL;
-- 
2.54.0 (Apple Git-157)

[PATCH net v8 3/3] xfrm: hold the tunnel device reference across the input path

[Qihang]

vti_input() and vti6_input_proto() cache the tunnel pointer in the skb
control buffer without taking a reference.  With async crypto the
receive callback runs from the crypto completion, after a possible
device teardown, and dereferences the freed tunnel device:

    BUG: KASAN: slab-use-after-free in vti_rcv_cb

Take a reference on the tunnel device when the pointer is cached.  The
tunnel is the device's private area, so the reference keeps the tunnel
structure alive as well.  xfrm6_rcv_spi() takes the same reference for
the tunnel it is given; it is a separate entry point, so vti6 does not
hold the reference twice.

A successful vti/vti6 callback leaves the cached tunnel in place for
xfrm_input() to release after delivery.  A callback that fails only
updates the error counters; the common drop path releases the reference
after the skb has been freed.  The helper takes the device out of the
skb before gro_cells_receive() and transport_finish(), which may already
consume the skb, so the device reference is always dropped after the skb
is gone.  The early drop for a state that is no longer valid releases
the reference too, and needs the address family for that, so set the
family before the validity check.

The reference is released right after gro_cells_receive().  The NAPI
that processes the queued skb later still reads skb->dev without a
reference.  That window is pre-existing on the synchronous path as well
and is not changed here.

IPTFS consumes the outer skb itself and frees it directly, so it must
release the reference as well.  The IPTFS state carries the address
family the tunnel was resolved for, and the caller passes it in.  Add
xfrm_input_drop() and xfrm_input_consume() for the outer-skb free paths
in __input_process_payload(), iptfs_input_ordered(), iptfs_input(),
iptfs_drop_timer() and iptfs_destroy_state().  Only the outer skb
carries the cached tunnel; the inner skbs built by iptfs_alloc_skb()
have a zeroed control buffer, so they do not release it.  The drop paths
keep their skb drop reason and the consumed skb keeps consume_skb()
semantics.

Fixes: df3893c176e9 ("vti: Update the ipv4 side to use it's own receive hook.")
Fixes: fa9ad96d4905 ("vti6: Update the ipv6 side to use its own receive hook.")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/ipv4/ip_vti.c      |  3 ++
 net/ipv6/ip6_vti.c     |  2 ++
 net/ipv6/xfrm6_input.c |  3 ++
 net/xfrm/xfrm_inout.h  |  4 +++
 net/xfrm/xfrm_input.c  | 64 ++++++++++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_iptfs.c  | 31 +++++++++++---------
 6 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index c4f14d42d..e7b836aba 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -63,6 +63,9 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
 
+		/* Keep the tunnel alive until xfrm_input() is done with the skb. */
+		dev_hold(tunnel->dev);
+
 		if (update_skb_dev)
 			skb->dev = tunnel->dev;
 
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index c79027839..c3666dc53 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -319,6 +319,8 @@ static int vti6_input_proto(struct sk_buff *skb, int nexthdr, __be32 spi,
 			goto discard;
 		}
 
+		/* Keep the tunnel alive until xfrm_input() is done with the skb. */
+		dev_hold(t->dev);
 		rcu_read_unlock();
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = t;
diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
index 07edef258..d27bc24f1 100644
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -15,6 +15,7 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv6.h>
 #include <net/ipv6.h>
+#include <net/ip6_tunnel.h>
 #include <net/xfrm.h>
 #include <net/protocol.h>
 #include <net/gro.h>
@@ -23,6 +24,8 @@ int xfrm6_rcv_spi(struct sk_buff *skb, int nexthdr, __be32 spi,
 		  struct ip6_tnl *t)
 {
 	XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = t;
+	if (t)
+		dev_hold(t->dev);
 	XFRM_SPI_SKB_CB(skb)->family = AF_INET6;
 	XFRM_SPI_SKB_CB(skb)->daddroff = offsetof(struct ipv6hdr, daddr);
 	return xfrm_input(skb, nexthdr, spi, 0);
diff --git a/net/xfrm/xfrm_inout.h b/net/xfrm/xfrm_inout.h
index efc5e6b2e..8bd05f62a 100644
--- a/net/xfrm/xfrm_inout.h
+++ b/net/xfrm/xfrm_inout.h
@@ -6,6 +6,10 @@
 #ifndef XFRM_INOUT_H
 #define XFRM_INOUT_H 1
 
+void xfrm_input_drop(struct sk_buff *skb, unsigned int family,
+		     enum skb_drop_reason reason);
+void xfrm_input_consume(struct sk_buff *skb, unsigned int family);
+
 static inline void xfrm4_extract_header(struct sk_buff *skb)
 {
 	const struct iphdr *iph = ip_hdr(skb);
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index cfd5cea03..e3b6c486f 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -120,6 +120,56 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
 	return ret;
 }
 
+/*
+ * The cached tunnel holds one reference on its net_device.
+ * The address family determines the tunnel type.
+ */
+static struct net_device *xfrm_input_take_tunnel_dev(struct sk_buff *skb,
+						     unsigned int family)
+{
+	void *slot = (void *)XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4;
+	struct net_device *dev;
+
+	if (!slot)
+		return NULL;
+
+	switch (family) {
+	case AF_INET:
+		dev = ((struct ip_tunnel *)slot)->dev;
+		break;
+	case AF_INET6:
+		dev = ((struct ip6_tnl *)slot)->dev;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return NULL;
+	}
+
+	XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = NULL;
+	return dev;
+}
+
+void xfrm_input_drop(struct sk_buff *skb, unsigned int family,
+		     enum skb_drop_reason reason)
+{
+	struct net_device *dev = xfrm_input_take_tunnel_dev(skb, family);
+
+	kfree_skb_reason(skb, reason);
+	if (dev)
+		dev_put(dev);
+}
+EXPORT_SYMBOL(xfrm_input_drop);
+
+void xfrm_input_consume(struct sk_buff *skb, unsigned int family)
+{
+	struct net_device *dev = xfrm_input_take_tunnel_dev(skb, family);
+
+	consume_skb(skb);
+	if (dev)
+		dev_put(dev);
+}
+EXPORT_SYMBOL(xfrm_input_consume);
+
 struct sec_path *secpath_set(struct sk_buff *skb)
 {
 	struct sec_path *sp, *tmp = skb_ext_find(skb, SKB_EXT_SEC_PATH);
@@ -468,6 +518,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	const struct xfrm_state_afinfo *afinfo;
 	struct net *net = dev_net(skb->dev);
 	struct net_device *dev = skb->dev;
+	struct net_device *tdev;
 	int err;
 	__be32 seq;
 	__be32 seq_hi;
@@ -747,8 +798,11 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			sp->olen = 0;
 		if (skb_valid_dst(skb))
 			skb_dst_drop(skb);
-		/* gro_cells_receive() reads skb->dev. */
+		/* gro_cells_receive() may free the skb. */
+		tdev = xfrm_input_take_tunnel_dev(skb, family);
 		gro_cells_receive(&gro_cells, skb);
+		if (tdev)
+			dev_put(tdev);
 		if (async)
 			dev_put(dev);
 		rcu_read_unlock();
@@ -758,6 +812,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		if (xo)
 			xfrm_gro = xo->flags & XFRM_GRO;
 
+		tdev = xfrm_input_take_tunnel_dev(skb, family);
 		err = -EAFNOSUPPORT;
 		afinfo = xfrm_state_afinfo_get_rcu(family);
 		if (likely(afinfo))
@@ -771,6 +826,8 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			gro_cells_receive(&gro_cells, skb);
 		}
 
+		if (tdev)
+			dev_put(tdev);
 		if (async)
 			dev_put(dev);
 		rcu_read_unlock();
@@ -781,9 +838,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	spin_unlock(&x->lock);
 drop:
 	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
+	tdev = xfrm_input_take_tunnel_dev(skb, family);
+	kfree_skb(skb);
+	if (tdev)
+		dev_put(tdev);
 	if (async)
 		dev_put(dev);
-	kfree_skb(skb);
 	return 0;
 }
 EXPORT_SYMBOL(xfrm_input);
diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 5ea3e77dd..363962308 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -717,8 +717,9 @@ static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
 			      IPTFS_HRTIMER_MODE);
 	}
 
-	if (free)
-		kfree_skb(xtfs->ra_newskb);
+	if (free && xtfs->ra_newskb)
+		xfrm_input_drop(xtfs->ra_newskb, xtfs->x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	xtfs->ra_newskb = NULL;
 }
 
@@ -1213,14 +1214,11 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 	skb_abort_seq_read(skbseq);
 
 	if (defer) {
-		consume_skb(defer);
+		xfrm_input_consume(defer, x->props.family);
 	} else if (!first_skb) {
-		/* skb is the original passed in skb, but we didn't get far
-		 * enough to process it as the first_skb, if we had it would
-		 * either be save in ra_newskb, trimmed and sent on as an skb or
-		 * placed in defer to be freed.
-		 */
-		kfree_skb(skb);
+		/* The original skb was not consumed as the first inner packet. */
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 	return true;
 }
@@ -1307,7 +1305,8 @@ static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
 	if (!consumed) {
 		skb = skbseq.root_skb;
 		skb_abort_seq_read(&skbseq);
-		kfree_skb(skb);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 }
 
@@ -1709,7 +1708,8 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 	spin_unlock(&xtfs->drop_lock);
 
 	if (skb)
-		kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
 
 	if (count) {
 		list_for_each_entry_safe(skb, next, &list, list) {
@@ -1760,7 +1760,8 @@ static int iptfs_input(struct xfrm_state *x, struct sk_buff *skb)
 
 	list_for_each_entry_safe(skb, next, &freelist, list) {
 		skb_list_del_init(skb);
-		kfree_skb(skb);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 done:
 	/* We always have dealt with the input SKB, either we are re-using it,
@@ -2761,11 +2762,13 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	hrtimer_cancel(&xtfs->drop_timer);
 
 	if (xtfs->ra_newskb)
-		kfree_skb(xtfs->ra_newskb);
+		xfrm_input_drop(xtfs->ra_newskb, xtfs->x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 
 	for (s = xtfs->w_saved, se = s + xtfs->w_savedlen; s < se; s++) {
 		if (s->skb)
-			kfree_skb(s->skb);
+			xfrm_input_drop(s->skb, x->props.family,
+					SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 
 	kfree_sensitive(xtfs->w_saved);
-- 
2.54.0 (Apple Git-157)

[PATCH net v8 0/3] xfrm: fix vti tunnel device use-after-free across async crypto resumption

[Qihang]

This replaces the earlier vti/vti6 series and the reference-transfer
rework.

vti_input() and vti6_input_proto() cache the tunnel pointer in the skb
control buffer without taking a reference. With async crypto, the
receive callback runs from crypto completion and may run after the
tunnel device has been torn down, causing a use-after-free when the
callback dereferences the cached tunnel.

The input hook takes a reference on the tunnel device when caching the
tunnel. A successful receive callback leaves the cached tunnel in
place; xfrm_input() releases the reference on every exit, including
after delivery and on drop paths. The error callback only updates the
error counters, so the common drop path releases the tunnel reference
after the skb has been freed. IPTFS, which can free or consume the
outer skb itself, releases the reference through xfrm_input_drop() or
xfrm_input_consume(). Patch 1 makes the two exits that still used
skb->dev after the async reference was dropped use it first. Patch 3
sets the address family before the early invalid-state drop, which now
has to release the cached tunnel.

Patch 2 is new. The IPTFS drop timer is cancelled when an in-progress
reassembly completes, although the same timer ages out the reorder
window. A window that is never drained holds its queued packets and,
with patch 3, the tunnel reference they carry, so the device could not
be unregistered. Patch 2 rearms the timer for the window's own deadline
once the reassembly is done, so that a later reassembly does not
inherit the earlier expiry.

Patch 2 is deliberately minimal. It only bounds the stale window, which
is what 3/3 needs so that the reference it adds cannot pin the device; a
wider rework of the IPTFS timer is orthogonal and can land on its own.

Rebased onto net main fafbc4051, which already carries the iptfs skb
leak fix on the payload error path. That fix moved the done label ahead
of the delivery loop, so the inner packets parsed before a validation
failure are now delivered. The drain that the previous revision added
to 3/3 is therefore gone.

The issue was found by source audit of the VTI input path (LLM-assisted
review): the tunnel pointer cached in the skb control buffer is
dereferenced by the receive callback, which with async crypto can run
after the device has been torn down, and nothing keeps the device alive
across that window.  It was confirmed with a KASAN reproducer on arm64
(async ESP; deleting the VTI device inside the crypto window gives
"slab-use-after-free in vti_rcv_cb").

Changes since v7:
- rebased onto net main fafbc4051.  The drain of @sublist added in v5 is
  dropped: "xfrm: iptfs: fix skb leak on error in
  __input_process_payload()" is in net now and delivers those packets.
- new patch 2: hand the IPTFS drop timer back to the reorder window when a
  reassembly completes, so a stalled window cannot pin the state (and the
  tunnel reference added by patch 3) forever; reported by the AI review
  of v7.
- the former 2/3 (address family before the invalid-state check) is
  folded into 3/3, because the early drop path in 3/3 needs the family
  to release the cached tunnel. This gives a clean bisect: no
  intermediate commit sets the family without using it.
- the sentence in 3/3 that claimed the parked skbs are always released by
  the drop timer is gone; patch 2 is what makes that true.

Changes since v6:
- patch 1 also drops the skb->dev reference that is held across an async
  resumption only after gro_cells_receive() in the decaps success exit;
  that exit still used skb->dev after the reference was dropped (found
  by the netdev AI review).

Changes since v5:
- cover letter only: record how the issue was found.

Changes since v4:
- IPTFS: when an inner packet fails validation, __input_process_payload()
  can reach the drop path before the inner packets queued for delivery
  are sent. Drop those queued packets as well so the outer skb's tunnel
  device reference is not leaked (found by the Sashiko review of v4 3/3).

Changes since v3:
- the cached pointer is always a tunnel; the address family selects its
  type, and a non-NULL tunnel slot denotes one held device reference
- xfrm_input() takes the tunnel device out of the skb before
  gro_cells_receive()/transport_finish(), which may consume the skb
- the device reference is dropped only after the skb has been freed,
  including on error paths
- IPTFS passes the state's address family explicitly instead of
  inferring it from the secpath; consumed skbs keep consume_skb()
  semantics and drop paths keep their skb drop reason
- drop the xfrmi changes: it re-looks up the interface and does not
  participate in the reference transfer
- merge the reference and IPTFS changes into one patch so the series
  remains bisectable
- the gro_cells NAPI window is pre-existing and is documented, not
  changed

Runtime testing (aarch64 QEMU; KASAN for UAF detection):
- VTI async success and error: RTM_DELLINK completes without a KASAN
  report; the unfixed tree reports slab-use-after-free in vti_rcv_cb.
- Async resume with the SA deleted during the crypto window: the
  reference is released and the device is removed; with patch 3
  reverted, xfrm_input_take_tunnel_dev() warns and
  unregister_netdevice() waits for vti0.
- IPTFS outer-skb free: RTM_DELLINK completes; with the
  !first_skb xfrm_input_drop() reverted to kfree_skb(), the reference
  leaks and unregister_netdevice() waits for vti0.

The IPTFS drop-timer/reorder/state-teardown paths, including the
window-pin sequence behind patch 2, are covered by code audit, not by
dedicated runtime triggers.

v8 was build-tested on fafbc4051: the arm64 objects touched by the
series (xfrm, xfrm_iptfs, ip_vti, ip6_vti, xfrm6_input) compile with
XFRM, IPTFS, VTI and ESP enabled.

Tooling: the source audit, the patch text and parts of these messages
were produced with an AI coding assistant under my direction. I reviewed
the result and stand behind every line of it; the build and runtime
results above were obtained from earlier revisions of these changes.

Previous versions:
  https://lore.kernel.org/netdev/20261027105031.19928-1-q.h.hack.winter@gmail.com/ (v7)
  https://lore.kernel.org/netdev/20261022032129.16118-1-q.h.hack.winter@gmail.com/ (v6)
  https://lore.kernel.org/netdev/20261019153247.89957-1-q.h.hack.winter@gmail.com/ (v5)
  https://lore.kernel.org/netdev/20261018090638.55686-1-q.h.hack.winter@gmail.com/ (v4)
  https://lore.kernel.org/netdev/20261013074610.17808-1-q.h.hack.winter@gmail.com/ (v2)
  https://lore.kernel.org/netdev/20260930090813.73901-1-q.h.hack.winter@gmail.com/ (v1)

Qihang (3):
  xfrm: use skb->dev before dropping the reference held across async
  xfrm: iptfs: hand the drop timer back to the reorder window
  xfrm: hold the tunnel device reference across the input path

 net/ipv4/ip_vti.c      |  3 ++
 net/ipv6/ip6_vti.c     |  2 ++
 net/ipv6/xfrm6_input.c |  3 ++
 net/xfrm/xfrm_inout.h  |  4 +++
 net/xfrm/xfrm_input.c  | 69 +++++++++++++++++++++++++++++++++++++++---
 net/xfrm/xfrm_iptfs.c  | 40 +++++++++++++++---------
 6 files changed, 103 insertions(+), 18 deletions(-)

-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v8 2/3] xfrm: iptfs: hand the drop timer back to the reorder window

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] xfrm: iptfs: negative relative timeout in __iptfs_reassem_done()
- [Medium] The patch re-arms the shared drop_timer for the reorder
  window's slot-0 deadline in __iptfs_reassem_done()…
- [Low] The commit message overstates the failure mode.
- [Low] The iptfs_drop_timer() kernel-doc
  (net/xfrm/xfrm_iptfs.c:1669-1670) still says the drop timer is armed…

--

xfrm: iptfs: hand the drop timer back to the reorder window

This patch changes __iptfs_reassem_done() so that, after it cancels the
shared drop_timer as before, it re-arms the timer for the slot 0 drop
time when the reorder window still holds saved packets. The goal is to
stop out-of-order packets from being stranded in the window, where they
pin the xfrm_state, once an in-progress reassembly finishes.

Link: https://lore.kernel.org/netdev/20260824072851.301644-3-lilly@aronleigh.au/

> If the missing packet never arrives, the window is never drained: the
> queued packets are not delivered, and each one keeps the xfrm_state
> reference it took from its secpath, so the state cannot be destroyed.

[Severity: Low]
Is this accurate while traffic is still flowing on the SA?

A packet with seq - w_wantseq >= nslots goes through
__reorder_future_shifts(). That function shifts the window and delivers
the saved packets:

net/xfrm/xfrm_iptfs.c:__reorder_future_shifts() {
    ...
	/* Remove the entries */
	__vec_shift(xtfs, beyond);

	/* Advance want seq */
	xtfs->w_wantseq += beyond;
    ...
}

With the default window of 3 this happens within a few packets. Under
active traffic, the effect looks like a delivery stall past the drop
time. The permanent stall and the pinned xfrm_state only seem to happen
once input on the SA stops, either because the tunnel is idle or because
the SA was deleted.

The rest of the description matches the code. Once
__iptfs_reassem_done() cancels the timer, iptfs_set_window_drop_times()
does not re-arm it, because the walk stops at an occupied lower slot.

Could the commit message describe the trigger conditions more precisely?

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 0313a43aa8a97..5ea3e77dd2c8c 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -708,6 +708,15 @@ static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
>  
>  	/* We don't care if it works locking takes care of things */
>  	hrtimer_try_to_cancel(&xtfs->drop_timer);
> +	/* The timer also ages out the reorder window, if it has any entries. */
> +	if (xtfs->w_savedlen) {
> +		time64_t now = ktime_get_raw_fast_ns();
> +
> +		hrtimer_start(&xtfs->drop_timer,
> +			      xtfs->w_saved[0].drop_time - now,
> +			      IPTFS_HRTIMER_MODE);
> +	}

[Severity: High]
What happens here when now is already past w_saved[0].drop_time?

w_saved[0].drop_time is a u64, so the subtraction wraps and becomes a
negative ktime_t. IPTFS_HRTIMER_MODE is a relative mode, so
__hrtimer_start_range_ns() adds the base clock time to it with
ktime_add_safe():

kernel/time/hrtimer.c:ktime_add_safe() {
	ktime_t res = ktime_add_unsafe(lhs, rhs);
    ...
	if (res < 0 || res < lhs || res < rhs)
		res = ktime_set(KTIME_SEC_MAX, 0);
    ...
}

With lhs < 0 and rhs >= 0, res < rhs is always true. The expiry is
clamped to KTIME_SEC_MAX instead of the timer firing right away.

The drop_timer then stays queued essentially forever, and
iptfs_drop_timer() never runs to age out w_saved. Because the timer is
queued, the !hrtimer_is_queued(&xtfs->drop_timer) checks in
__input_process_payload() and iptfs_set_window_drop_times() also stop
arming it. A reassembly started afterwards gets no drop_time_ns budget
either. Only the next __iptfs_reassem_done() cancels the timer, and
that call can hit the same case again while slot 0 is still past due.
Once input on the SA stops, the saved skbs and the xfrm_state
references held by their secpaths are stranded. That is the leak this
patch sets out to fix.

The precondition does not look rare:

  - The soft drop_timer has expired, but HRTIMER_SOFTIRQ has not run
    yet when NET_RX_SOFTIRQ finishes a reassembly. The timer is still
    queued, hrtimer_try_to_cancel() removes it, and slot 0 is already
    past its drop time.
  - iptfs_drop_timer() is already running on another CPU and is
    spinning on drop_lock.
  - drop_timer runs on CLOCK_MONOTONIC while the drop times come from
    ktime_get_raw_fast_ns(), and the two can drift apart under NTP
    adjustment.
  - XFRMA_IPTFS_DROP_TIME is configured as 0, so every saved drop_time
    is already in the past by the time it is used here.

__reorder_drop() makes sure slot 0's drop_time is later than now before
it passes w_saved[0].drop_time - now to hrtimer_start(). Should this
path guard against an expired slot 0 in the same way? For example, it
could clamp the relative timeout to 0 so that iptfs_drop_timer() runs
right away.

[Severity: Medium]
Can a reassembly that starts right after this hand-off inherit the slot
0 deadline instead of getting its own drop_time_ns budget?

In __input_process_payload(), the start-of-reassembly code only arms the
timer when it is not already queued:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
			xtfs->ra_newskb = skb;
			xtfs->ra_wantseq = seq + 1;
			if (!hrtimer_is_queued(&xtfs->drop_timer)) {
				/* softirq blocked lest the timer fire and interrupt us */
				hrtimer_start(&xtfs->drop_timer,
					      xtfs->drop_time_ns,
					      IPTFS_HRTIMER_MODE);
    ...
}

After this patch the timer is queued for w_saved[0].drop_time. That value
was set when an earlier packet was saved, so it is always earlier than
now + drop_time_ns and may be nearly due.

__reorder_this() can then drain the window with __vec_shift() without
touching drop_timer. When the stale deadline fires, iptfs_drop_timer()
drops ra_newskb unconditionally:

	/* Drop any in progress packet */
	skb = xtfs->ra_newskb;
	xtfs->ra_newskb = NULL;

One possible sequence with the default window of 3:

  wantseq = W, reassembly from W-1 in progress, timer at T(W-1) + D
  W+3 arrives at t3 and is saved in slot 2
    slots 0..2 get drop_time t3 + D
    timer already queued, so it is not re-armed
  W arrives at t1 > t3, __reorder_this() delivers it and shifts by 1
    iptfs_input_ordered(W) completes the old reassembly
      __iptfs_reassem_done() re-arms the timer for t3 + D
  W+1 arrives and is delivered
    its tail starts a new reassembly with ra_wantseq = W+2
    timer is queued, so no budget of its own is armed
  W+2 arrives before t3 + D
    __reorder_this() delivers W+2 and W+3, window is now empty
    drop_timer is not touched
  at t3 + D the timer fires
    w_savedlen == 0, so nothing is dropped from the window
    ra_newskb is freed anyway
  W+4 arrives with ra_newskb == NULL
    iptfs_reassem_cont() skips the fragment

Before this patch, completing the reassembly at W cancelled the timer.
The reassembly started by W+1 then armed its own budget and survived.
The old code could already hit this when a timer armed by the window was
queued at reassembly start. This patch adds a new path into it on every
reassembly completion while the window is non-empty.

There is also a narrower race. iptfs_input() delivers the in-order list
outside drop_lock:

	spin_lock(&xtfs->drop_lock);
	iptfs_input_reorder(xtfs, skb, &list, &freelist);
	spin_unlock(&xtfs->drop_lock);

	list_for_each_entry_safe(skb, next, &list, list) {
		skb_list_del_init(skb);
		iptfs_input_ordered(x, skb);
	}

The soft hrtimer can run on another CPU, either because the hrtimer base
migrated or because hrtimer_try_to_cancel() returned -1 while the
callback was running.

Could the callback then take drop_lock between two packets of the same
batch, and drop a reassembly whose continuation is the next packet in
the list?

In both cases the invariant in the iptfs_drop_timer() kernel-doc ("either
it's the reassembly timer, or slot 0 is going to be dropped ... and slot
0 holds the continuation fragment") no longer holds.

The commit message says the slot 0 hand-off avoids the case where "a
reassembly started in between would then not arm its timer and could be
dropped before its budget ran out". Isn't that outcome still reachable
with this approach?

[Severity: Low]
This isn't a bug, but the iptfs_drop_timer() kernel-doc still lists only
two places where the timer is armed:

 * The drop timer is set when we start an in progress reassembly, and also when
 * we save a future packet in the window saved array.

Should this comment mention the new arming site in
__iptfs_reassem_done()? That site runs on both completion and abort, and
it uses w_saved[0].drop_time - now instead of drop_time_ns.

The comment already left out the existing re-arm in __reorder_drop().
Later in the same comment, the argument that dropping ra_newskb
unconditionally is safe depends on which deadline the timer holds.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107111617.21347-1-q.h.hack.winter%40gmail.com

Re: [PATCH net v8 3/3] xfrm: hold the tunnel device reference across the input path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch adds a reference that leaks, and the leak fires a
  WARN on the error path that the async-teardown fix is meant to cover.

--

xfrm: hold the tunnel device reference across the input path

This patch makes vti_input(), vti6_input_proto() and xfrm6_rcv_spi() take
a reference on the tunnel net_device when they cache the tunnel pointer in
the skb control buffer. xfrm_input() releases the reference on delivery
and on drop, and IPTFS releases it through new helpers,
xfrm_input_drop() and xfrm_input_consume().

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index cfd5cea033127..e3b6c486f0e9c 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c

[ ... ]

> @@ -781,9 +838,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  	spin_unlock(&x->lock);
>  drop:
>  	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
> +	tdev = xfrm_input_take_tunnel_dev(skb, family);
> +	kfree_skb(skb);
> +	if (tdev)
> +		dev_put(tdev);

[Severity: High]
Can this leak the tunnel device reference taken by dev_hold() in
vti_input() or vti6_input_proto()?

At the top of xfrm_input(), in the resume/GRO/offload branch, the
invalid-state check jumps to drop before family is assigned:

    if (unlikely(x->km.state != XFRM_STATE_VALID)) {
        ...
        if (encap_type == -1)
            async = 1;
        goto drop;
    }

    family = x->props.family;

At that point family still has its initial value, AF_UNSPEC. The slot is
non-NULL, so xfrm_input_take_tunnel_dev() falls into its default case:

    default:
        WARN_ON_ONCE(1);
        return NULL;

The slot is not cleared and tdev stays NULL. The skb is freed and the
dev_put() is skipped.

One way to reach this is async crypto that completes after the SA has
been deleted or has expired, for example during an IKE rekey:

vti_input()
  dev_hold(tunnel->dev)
  xfrm_input()
    esp_input() returns -EINPROGRESS
<SA deleted, km.state no longer XFRM_STATE_VALID>
xfrm_input_resume()
  xfrm_input(skb, nexthdr, 0, -1)
    goto drop with family == AF_UNSPEC

Two other paths look the same. One is an offload skb that enters through
vti_input() with encap_type 0. The other is the IPTFS outer skb that is
passed to xfrm_input(skb, 0, 0, -2) after the SA has become invalid.

If the reference leaks, a later unregister of the vti/vti6 device or a
netns teardown would wait forever in netdev_wait_allrefs_any(). The
WARN_ON_ONCE() would also panic the machine with panic_on_warn set.

The commit message says:

  "The early drop for a state that is no longer valid releases
  the reference too, and needs the address family for that, so set the
  family before the validity check."

The diff doesn't move the family = x->props.family assignment ahead of
the km.state check, though. Was that hunk left out of the patch?

>  	if (async)
>  		dev_put(dev);
> -	kfree_skb(skb);
>  	return 0;
>  }
>  EXPORT_SYMBOL(xfrm_input);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107111617.21347-1-q.h.hack.winter%40gmail.com