[PATCH net v3 1/2] xfrm: iptfs: track independent drop deadlines

[PATCH net v3 1/2] xfrm: iptfs: track independent drop deadlines

[Roshan Kumar]

IP-TFS uses one hrtimer for two independently queued states: an
incomplete inner packet and the receive reorder window. Completing or
aborting reassembly cancels that shared timer unconditionally, so packets
in the reorder window can remain queued indefinitely.

Merely leaving the timer armed is insufficient. If it was armed for a
completed reassembly, its old deadline can expire a subsequent reassembly
before that packet's own drop interval has elapsed. A reassembly created
from a runt also does not arm the timer at all.

Record an absolute deadline for an in-progress reassembly. Whenever either
kind of queued state changes, arm the shared timer for the earliest active
deadline. On expiry, drop only state whose own deadline has passed and
rearm the timer for anything that remains.

Reported-by: Lilly Aronleigh <lilly@aronleigh.au>
Link: https://lore.kernel.org/netdev/20260824072851.301644-3-lilly@aronleigh.au/
Link: https://lore.kernel.org/netdev/apaRiWQn54Pr9hpm@secunet.com/
Fixes: 075694765446 ("xfrm: iptfs: handle received fragmented inner packets")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Roshan Kumar <roshaen09@gmail.com>
---
 net/xfrm/xfrm_iptfs.c | 81 +++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 29 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 6920940a35b4..e538cc98e257 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -143,6 +143,7 @@ struct skb_wseq {
  * @drop_time_ns: timer intervan in nanoseconds.
  * @ra_newskb: new pkt being reassembled.
  * @ra_wantseq: expected next sequence for reassembly.
+ * @ra_drop_time: deadline for dropping @ra_newskb.
  * @ra_runt: last pkt bytes from very end of last skb.
  * @ra_runtlen: size of ra_runt.
  */
@@ -172,6 +173,7 @@ struct xfrm_iptfs_data {
 	/* Tunnel input reassembly */
 	struct sk_buff *ra_newskb; /* new pkt being reassembled */
 	u64 ra_wantseq;		   /* expected next sequence */
+	u64 ra_drop_time;	   /* reassembly drop deadline */
 	u8 ra_runt[6];		   /* last pkt bytes from last skb */
 	u8 ra_runtlen;		   /* count of ra_runt */
 };
@@ -703,15 +705,38 @@ static void iptfs_complete_inner_skb(struct xfrm_state *x, struct sk_buff *skb)
 	}
 }
 
+/* Arm the shared timer for the earliest reassembly or reorder deadline. */
+static void iptfs_reset_drop_timer(struct xfrm_iptfs_data *xtfs)
+{
+	u64 expires = 0;
+	u64 now;
+
+	assert_spin_locked(&xtfs->drop_lock);
+
+	if (xtfs->ra_newskb)
+		expires = xtfs->ra_drop_time;
+	if (xtfs->w_savedlen &&
+	    (!expires || xtfs->w_saved[0].drop_time < expires))
+		expires = xtfs->w_saved[0].drop_time;
+	if (!expires) {
+		hrtimer_try_to_cancel(&xtfs->drop_timer);
+		return;
+	}
+
+	now = ktime_get_raw_fast_ns();
+	hrtimer_start(&xtfs->drop_timer, expires > now ? expires - now : 0,
+		      IPTFS_HRTIMER_MODE);
+}
+
 static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
 {
 	assert_spin_locked(&xtfs->drop_lock);
 
-	/* We don't care if it works locking takes care of things */
-	hrtimer_try_to_cancel(&xtfs->drop_timer);
 	if (free)
 		kfree_skb(xtfs->ra_newskb);
 	xtfs->ra_newskb = NULL;
+	xtfs->ra_drop_time = 0;
+	iptfs_reset_drop_timer(xtfs);
 }
 
 /**
@@ -845,6 +870,9 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 			goto abandon;
 		}
 		xtfs->ra_newskb = newskb;
+		xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
+				     xtfs->drop_time_ns;
+		iptfs_reset_drop_timer(xtfs);
 
 		/* Copy the runt data into the buffer, but leave data
 		 * pointers the same as normal non-runt case. The extra `rrem`
@@ -1162,12 +1190,9 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 
 			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);
-			}
+			xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
+					     xtfs->drop_time_ns;
+			iptfs_reset_drop_timer(xtfs);
 
 			spin_unlock(&xtfs->drop_lock);
 
@@ -1336,7 +1361,7 @@ static u32 __reorder_drop(struct xfrm_iptfs_data *xtfs, struct list_head *list)
 	u32 scount = 0;
 
 	if (xtfs->w_saved[0].drop_time > now)
-		goto set_timer;
+		return 0;
 
 	++xtfs->w_wantseq;
 
@@ -1363,13 +1388,6 @@ static u32 __reorder_drop(struct xfrm_iptfs_data *xtfs, struct list_head *list)
 		__vec_shift(xtfs, count);
 	}
 
-	if (xtfs->w_savedlen) {
-set_timer:
-		/* Drifting is OK */
-		hrtimer_start(&xtfs->drop_timer,
-			      xtfs->w_saved[0].drop_time - now,
-			      IPTFS_HRTIMER_MODE);
-	}
 	return scount;
 }
 
@@ -1423,10 +1441,7 @@ static void iptfs_set_window_drop_times(struct xfrm_iptfs_data *xtfs, int index)
 	while (index-- > 0 && !s[index].skb)
 		s[index].drop_time = drop_time;
 
-	/* If we walked all the way back, schedule the drop timer if needed */
-	if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
-		hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
-			      IPTFS_HRTIMER_MODE);
+	iptfs_reset_drop_timer(xtfs);
 }
 
 static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs,
@@ -1660,16 +1675,15 @@ static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs,
  * 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.
  *
- * NOTE packets in the save window are always newer WRT drop times as
- * they get further in the future. i.e. for:
+ * Packets in the save window are always newer WRT drop times as they get
+ * further in the future. i.e. for:
  *
  *    if slots (S0, S1, ... Sn) and `Dn` is the drop time for slot `Sn`,
  *    then D(n-1) <= D(n).
  *
- * So, regardless of why the timer is firing we can always discard any inprogress
- * fragment; either it's the reassembly timer, or slot 0 is going to be
- * dropped as S0 must have the most recent drop time, and slot 0 holds the
- * continuation fragment of the in progress packet.
+ * Reassembly and slot 0 keep independent deadlines. The shared timer is armed
+ * for the earlier one, and this callback expires only the state whose deadline
+ * has passed before rearming for any state that remains.
  *
  * Returns HRTIMER_NORESTART.
  */
@@ -1679,6 +1693,7 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 	struct list_head list;
 	struct xfrm_iptfs_data *xtfs;
 	struct xfrm_state *x;
+	u64 now;
 	u32 count;
 
 	xtfs = container_of(me, typeof(*xtfs), drop_timer);
@@ -1687,15 +1702,22 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 	INIT_LIST_HEAD(&list);
 
 	spin_lock(&xtfs->drop_lock);
+	now = ktime_get_raw_fast_ns();
 
-	/* Drop any in progress packet */
-	skb = xtfs->ra_newskb;
-	xtfs->ra_newskb = NULL;
+	/* Drop an in-progress packet only after its own deadline. */
+	if (xtfs->ra_newskb && xtfs->ra_drop_time <= now) {
+		skb = xtfs->ra_newskb;
+		xtfs->ra_newskb = NULL;
+		xtfs->ra_drop_time = 0;
+	} else {
+		skb = NULL;
+	}
 
 	/* Now drop as many packets as we should from the reordering window
 	 * saved array
 	 */
 	count = xtfs->w_savedlen ? __reorder_drop(xtfs, &list) : 0;
+	iptfs_reset_drop_timer(xtfs);
 
 	spin_unlock(&xtfs->drop_lock);
 
@@ -2702,6 +2724,7 @@ static int iptfs_clone_state(struct xfrm_state *x, struct xfrm_state *orig)
 	xtfs->w_savedlen = 0;
 	xtfs->ra_newskb = NULL;
 	xtfs->ra_wantseq = 0;
+	xtfs->ra_drop_time = 0;
 	xtfs->ra_runtlen = 0;
 
 	__module_get(x->mode_cbs->owner);
-- 
2.43.0

Re: [PATCH net v3 1/2] xfrm: iptfs: track independent drop deadlines

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 1 · 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).

- [Medium] iptfs_reset_drop_timer() now calls hrtimer_start()
  unconditionally every time any queued state changes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  code the patch touches and relies on.
- [High] This problem wasn't introduced by this patch, but it is serious.

--

xfrm: iptfs: track independent drop deadlines

This patch records an absolute drop deadline for an in-progress IP-TFS
reassembly. It adds iptfs_reset_drop_timer(), which arms the shared
drop_timer for the earlier of the reassembly deadline and the reorder
window slot 0 deadline. On expiry, iptfs_drop_timer() now drops only the
state whose own deadline has passed and rearms for anything that remains.

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

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..e538cc98e2572 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c

[ ... ]

> @@ -703,15 +705,38 @@ static void iptfs_complete_inner_skb(struct xfrm_state *x, struct sk_buff *skb)
>  	}
>  }
>  
> +/* Arm the shared timer for the earliest reassembly or reorder deadline. */
> +static void iptfs_reset_drop_timer(struct xfrm_iptfs_data *xtfs)
> +{
> +	u64 expires = 0;
> +	u64 now;
> +
> +	assert_spin_locked(&xtfs->drop_lock);
> +
> +	if (xtfs->ra_newskb)
> +		expires = xtfs->ra_drop_time;
> +	if (xtfs->w_savedlen &&
> +	    (!expires || xtfs->w_saved[0].drop_time < expires))
> +		expires = xtfs->w_saved[0].drop_time;
> +	if (!expires) {
> +		hrtimer_try_to_cancel(&xtfs->drop_timer);
> +		return;
> +	}
> +
> +	now = ktime_get_raw_fast_ns();
> +	hrtimer_start(&xtfs->drop_timer, expires > now ? expires - now : 0,
> +		      IPTFS_HRTIMER_MODE);

[Severity: Medium]
Can an overdue deadline keep getting pushed back here?

hrtimer_start() is now called every time, so it replaces any expiry that
is already queued. Once the earliest deadline has passed, the timer is
restarted with a relative delay of 0.

IPTFS_HRTIMER_MODE is HRTIMER_MODE_REL_SOFT. On CONFIG_TIME_LOW_RES
builds (m68k, parisc, csky), hrtimer_update_lowres() adds one jiffy to
every relative expiry:

kernel/time/hrtimer.c:hrtimer_update_lowres() {
    ...
	timer->is_rel = mode & HRTIMER_MODE_REL;
	if (timer->is_rel)
		tim = ktime_add_safe(tim, hrtimer_resolution);
    ...
}

So each call after the deadline moves expiry to now + 1 jiffy. The reset
runs in these places:

- for each new most-future reorder packet, via
  iptfs_set_window_drop_times()
- for each reassembly start, in __input_process_payload() and
  iptfs_reassem_cont()
- for each reassembly completion or abort, via __iptfs_reassem_done()

Say the expected sequence is missing and these events arrive more often
than once per jiffy. Would the overdue timer then never fire?

If so, the saved packets and the overdue reassembly would wait until
traffic pauses or the window fills. At that point
__reorder_future_shifts() forces progress, which can take up to
reorder_win_size arrivals (up to 65535).

The old code only started the timer when !hrtimer_is_queued(), so it
never replaced a queued expiry.

> +}
> +

[ ... ]

> @@ -1162,12 +1190,9 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  
>  			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);
> -			}
> +			xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
> +					     xtfs->drop_time_ns;
> +			iptfs_reset_drop_timer(xtfs);
>  
>  			spin_unlock(&xtfs->drop_lock);
>  

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can
xtfs->ra_newskb be overwritten here while another reassembly is still
in progress?

For a single SA, iptfs_input_ordered() looks like it can run on two CPUs
at once. The timer path takes the flushed packets under drop_lock and
processes them after releasing it:

iptfs_drop_timer()
    spin_lock(&xtfs->drop_lock)
    __reorder_drop()
    spin_unlock(&xtfs->drop_lock)
    iptfs_input_ordered()

iptfs_input() does the same with the in-order packets returned by
iptfs_input_reorder(). Nothing else serialises the two paths:

- xfrm_input() has already released x->lock before calling
  xfrm_inner_mode_input().
- drop_timer is not pinned, so get_target_base() can move it to another
  CPU via get_nohz_timer_target().

iptfs_input_ordered() checks ra_newskb and ra_runtlen without holding
the lock. It takes drop_lock only around iptfs_reassem_cont():

	if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
		spin_lock(&xtfs->drop_lock);

Take the timer on CPU1 flushing seq N+2, which starts a fragmented inner
packet, and advancing w_wantseq to N+3. At the same time, CPU0 receives
and processes N+3:

CPU1                                CPU0
iptfs_drop_timer()
  iptfs_input_ordered(N+2)          iptfs_input_ordered(N+3)
                                      sees ra_newskb == NULL,
                                      iptfs_reassem_cont() drops
                                      the continuation
    __input_process_payload()         __input_process_payload()
      xtfs->ra_newskb = skbA;           xtfs->ra_newskb = skbB;

Is skbA leaked in this case? Nothing ever frees it or times it out. Also,
ra_wantseq and ra_drop_time would now describe the wrong packet.

The later patch in this series, "xfrm: iptfs: hold a device reference
while packets are queued", adds netdev_hold() for ra_newskb. With that
patch, the leaked skb would also hold a net_device reference and block
unregistration of that device.

[ ... ]

> @@ -1363,13 +1388,6 @@ static u32 __reorder_drop(struct xfrm_iptfs_data *xtfs, struct list_head *list)
>  		__vec_shift(xtfs, count);

[Severity: High]
This isn't a bug introduced by this patch, but this patch edits
__reorder_drop(). iptfs_reset_drop_timer() also now bases the timer on
w_saved[0].drop_time. Is the window layout that __reorder_drop() leaves
behind correct when the walk stops early?

The rest of the reorder code assumes w_saved[i] holds sequence
w_wantseq + 1 + i. __reorder_future_fits() uses index = distance - 1.
__reorder_this() shifts by count + 1 so that the new wantseq's own empty
slot is discarded:

	/* Shift handled slots plus final empty slot into slot 0. */
	__vec_shift(xtfs, count + 1);

__reorder_drop() increments w_wantseq and walks slots 0..k-1. It stops at
empty slot k if that slot's drop_time is still in the future:

		for (; s < se && !s->skb; s++) {
			if (s->drop_time > now)
				goto outerdone;
		}

It then adds k to w_wantseq, so the new wantseq is the sequence held in
slot k. However, it shifts by only count.

Doesn't that leave the empty wantseq slot at w_saved[0], with every saved
skb one slot too high?

For example, start with w_wantseq=10 and
w_saved=[skb11(T1), empty(T2), skb13(T2)]:

  timer fires at T1: w_wantseq=11, skb11 sent, walk stops at slot 1,
    count=1, w_wantseq=12, w_saved=[empty(T2), skb13]
  seq 12 arrives: __reorder_this() sets w_wantseq=13, count=0, shifts
    by 1, w_saved=[skb13] (now in the seq 14 position)
  seq 14 arrives: __reorder_future_fits() computes index 0, finds skb13
    there and frees seq 14 as a "dup of a future"

skb13 is then held until T2 (1s by default). If nothing arrives before
T2, __reorder_drop() sends skb13 with count=2 and sets w_wantseq=15. A
later seq 14 is then discarded by iptfs_input_reorder() as a past packet.

The later patch "xfrm: iptfs: hold a device reference while packets are
queued" does not change this logic either.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930050307.1978654-1-roshaen09%40gmail.com

[PATCH net v3 2/2] xfrm: iptfs: hold a device reference while packets are queued

[Roshan Kumar]

IP-TFS can retain received skbs after the input call that owns their
skb->dev reference returns. Out-of-order outer packets are stored in the
reorder window, and an incomplete inner packet is kept for reassembly.
Unregistering the ingress device while either skb is queued leaves a stale
device pointer for later timer or receive-path processing.

Take a device reference when an skb enters the reorder window or becomes
the in-progress reassembly packet. Transfer ownership of that reference
with the skb when the reorder window releases it, and put it only after
ordered processing has finished. In particular, keep the reassembly
reference through xfrm_input(), which reads skb->dev. Release references
on all completion, timeout, abort, and state-destruction paths.

An in-order packet also takes this reference before drop_lock is released,
even when it was never stored in the window, because device unregistration
can race with its subsequent ordered processing. Freelist entries are freed
within the input call and do not own a reference.

A KASAN kernel reproduces the stale access by queuing an out-of-order
packet through a TUN device, closing the device, and allowing the drop
timer to process the packet. With unprivileged user namespaces enabled,
an unprivileged process can create the required TUN and XFRM state in a
private user and network namespace and trigger the same KASAN report.

Reported-by: Shubham Antil <shubham@octane.security>
Link: https://lore.kernel.org/netdev/20260921084743.817859-1-roshaen09@gmail.com/
Reported-by: Giovanni Vignone <gio@octane.security>
Link: https://lore.kernel.org/netdev/20260921084743.817859-1-roshaen09@gmail.com/
Reported-by: Robert van Eijk <robert@octane.security>
Link: https://lore.kernel.org/netdev/20260921084743.817859-1-roshaen09@gmail.com/
Reported-by: Paolo Gentry <paolo@octane.security>
Link: https://lore.kernel.org/netdev/20260921084743.817859-1-roshaen09@gmail.com/
Link: https://lore.kernel.org/netdev/179023970333.2160803.2776986020964028023@kernel.org/
Fixes: 6c82d2433671 ("xfrm: iptfs: add basic receive packet (tunnel egress) handling")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Roshan Kumar <roshaen09@gmail.com>
---
 net/xfrm/xfrm_iptfs.c | 70 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index e538cc98e257..954d28e7ec2e 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -730,10 +730,16 @@ static void iptfs_reset_drop_timer(struct xfrm_iptfs_data *xtfs)
 
 static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
 {
+	struct sk_buff *skb = xtfs->ra_newskb;
+
 	assert_spin_locked(&xtfs->drop_lock);
 
-	if (free)
-		kfree_skb(xtfs->ra_newskb);
+	if (free && skb) {
+		struct net_device *dev = skb->dev;
+
+		kfree_skb(skb);
+		netdev_put(dev, NULL);
+	}
 	xtfs->ra_newskb = NULL;
 	xtfs->ra_drop_time = 0;
 	iptfs_reset_drop_timer(xtfs);
@@ -751,10 +757,15 @@ static void iptfs_reassem_abort(struct xfrm_iptfs_data *xtfs)
 /**
  * iptfs_reassem_done() - In-progress packet is complete, clear the state.
  * @xtfs: xtfs state
+ *
+ * Return: device with the reassembly reference still held.
  */
-static void iptfs_reassem_done(struct xfrm_iptfs_data *xtfs)
+static struct net_device *iptfs_reassem_done(struct xfrm_iptfs_data *xtfs)
 {
+	struct net_device *dev = xtfs->ra_newskb->dev;
+
 	__iptfs_reassem_done(xtfs, false);
+	return dev;
 }
 
 /**
@@ -766,6 +777,7 @@ static void iptfs_reassem_done(struct xfrm_iptfs_data *xtfs)
  * @data: offset into sequential packet data
  * @blkoff: packet blkoff value
  * @list: list of skbs to enqueue completed packet on
+ * @dev_to_put: device reference to release after processing @list
  *
  * Process an IPTFS payload that has a non-zero `blkoff` or when we are
  * expecting the continuation b/c we have a runt or in-progress packet.
@@ -774,7 +786,8 @@ static void iptfs_reassem_done(struct xfrm_iptfs_data *xtfs)
  */
 static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 			      struct skb_seq_state *st, struct sk_buff *skb,
-			      u32 data, u32 blkoff, struct list_head *list)
+			      u32 data, u32 blkoff, struct list_head *list,
+			      struct net_device **dev_to_put)
 {
 	struct iptfs_skb_frag_walk _fragwalk;
 	struct iptfs_skb_frag_walk *fragwalk = NULL;
@@ -870,6 +883,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 			goto abandon;
 		}
 		xtfs->ra_newskb = newskb;
+		netdev_hold(newskb->dev, NULL, GFP_ATOMIC);
 		xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
 				     xtfs->drop_time_ns;
 		iptfs_reset_drop_timer(xtfs);
@@ -957,7 +971,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 		xtfs->ra_wantseq++;
 	} else {
 		/* We are done with packet reassembly! */
-		iptfs_reassem_done(xtfs);
+		*dev_to_put = iptfs_reassem_done(xtfs);
 		iptfs_complete_inner_skb(xtfs->x, newskb);
 		list_add_tail(&newskb->list, list);
 	}
@@ -1189,6 +1203,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 			spin_lock(&xtfs->drop_lock);
 
 			xtfs->ra_newskb = skb;
+			netdev_hold(skb->dev, NULL, GFP_ATOMIC);
 			xtfs->ra_wantseq = seq + 1;
 			xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
 					     xtfs->drop_time_ns;
@@ -1250,6 +1265,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
  */
 static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
 {
+	struct net_device *dev_to_put = NULL;
 	struct ip_iptfs_cc_hdr iptcch;
 	struct skb_seq_state skbseq;
 	struct list_head sublist; /* rename this it's just a list */
@@ -1311,7 +1327,8 @@ static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
 		/* check again after lock */
 		if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
 			data = iptfs_reassem_cont(xtfs, seq, &skbseq, skb, data,
-						  blkoff, &sublist);
+						  blkoff, &sublist,
+						  &dev_to_put);
 		}
 
 		spin_unlock(&xtfs->drop_lock);
@@ -1325,6 +1342,8 @@ static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
 		skb_abort_seq_read(&skbseq);
 		kfree_skb(skb);
 	}
+	if (dev_to_put)
+		netdev_put(dev_to_put, NULL);
 }
 
 /* ------------------------------- */
@@ -1490,6 +1509,7 @@ static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs,
 	}
 
 	xtfs->w_saved[index].skb = inskb;
+	netdev_hold(inskb->dev, NULL, GFP_ATOMIC);
 	xtfs->w_savedlen = max(savedlen, index + 1);
 	iptfs_set_window_drop_times(xtfs, index);
 }
@@ -1625,6 +1645,7 @@ static void __reorder_future_shifts(struct xfrm_iptfs_data *xtfs,
 	/* We've shifted. plug the packet in at the end. */
 	xtfs->w_savedlen = nslots - 1;
 	xtfs->w_saved[xtfs->w_savedlen - 1].skb = inskb;
+	netdev_hold(inskb->dev, NULL, GFP_ATOMIC);
 	iptfs_set_window_drop_times(xtfs, xtfs->w_savedlen - 1);
 
 	/* if we don't have a slot0 then we must wait for it */
@@ -1638,7 +1659,8 @@ static void __reorder_future_shifts(struct xfrm_iptfs_data *xtfs,
 }
 
 /* Receive a new packet into the reorder window. Return a list of ordered
- * packets from the window.
+ * packets from the window. Packets on @list or in w_saved own a device
+ * reference; packets on @freelist do not.
  */
 static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs,
 				struct sk_buff *inskb, struct list_head *list,
@@ -1656,14 +1678,16 @@ static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs,
 	}
 	wantseq = xtfs->w_wantseq;
 
-	if (likely(inseq == wantseq))
+	if (likely(inseq == wantseq)) {
+		netdev_hold(inskb->dev, NULL, GFP_ATOMIC);
 		__reorder_this(xtfs, inskb, list);
-	else if (inseq < wantseq)
+	} else if (inseq < wantseq) {
 		__reorder_past(xtfs, inskb, freelist);
-	else if ((inseq - wantseq) < nslots)
+	} else if ((inseq - wantseq) < nslots) {
 		__reorder_future_fits(xtfs, inskb, freelist);
-	else
+	} else {
 		__reorder_future_shifts(xtfs, inskb, list);
+	}
 }
 
 /**
@@ -1721,13 +1745,20 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 
 	spin_unlock(&xtfs->drop_lock);
 
-	if (skb)
+	if (skb) {
+		struct net_device *dev = skb->dev;
+
 		kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
+		netdev_put(dev, NULL);
+	}
 
 	if (count) {
 		list_for_each_entry_safe(skb, next, &list, list) {
+			struct net_device *dev = skb->dev;
+
 			skb_list_del_init(skb);
 			iptfs_input_ordered(x, skb);
+			netdev_put(dev, NULL);
 		}
 	}
 
@@ -1767,8 +1798,11 @@ static int iptfs_input(struct xfrm_state *x, struct sk_buff *skb)
 	spin_unlock(&xtfs->drop_lock);
 
 	list_for_each_entry_safe(skb, next, &list, list) {
+		struct net_device *dev = skb->dev;
+
 		skb_list_del_init(skb);
 		iptfs_input_ordered(x, skb);
+		netdev_put(dev, NULL);
 	}
 
 	list_for_each_entry_safe(skb, next, &freelist, list) {
@@ -2774,12 +2808,20 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 
 	hrtimer_cancel(&xtfs->drop_timer);
 
-	if (xtfs->ra_newskb)
+	if (xtfs->ra_newskb) {
+		struct net_device *dev = xtfs->ra_newskb->dev;
+
 		kfree_skb(xtfs->ra_newskb);
+		netdev_put(dev, NULL);
+	}
 
 	for (s = xtfs->w_saved, se = s + xtfs->w_savedlen; s < se; s++) {
-		if (s->skb)
+		if (s->skb) {
+			struct net_device *dev = s->skb->dev;
+
 			kfree_skb(s->skb);
+			netdev_put(dev, NULL);
+		}
 	}
 
 	kfree_sensitive(xtfs->w_saved);
-- 
2.43.0

Re: [PATCH net v3 2/2] xfrm: iptfs: hold a device reference while packets are queued

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 0 (1) · 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] The patch stops the stale skb->dev use-after-free by pinning the
  ingress net_device with netdev_hold().
- [High] The new reference counting depends on xtfs->ra_newskb having a
  single owner, but nothing serializes ordered processing.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  remotely triggerable memory leak.
- [Medium] This problem wasn't introduced by this patch, but there is an
  off-by-one in __reorder_drop() (net/xfrm/xfrm_iptfs.c).

--

xfrm: iptfs: hold a device reference while packets are queued

This patch takes a net_device reference whenever an skb is stored in the
IP-TFS reorder window or becomes the in-progress reassembly skb. The
reference is released after ordered processing, and on the timeout, abort
and state destruction paths. The goal is to stop a stale skb->dev access
after the ingress device is unregistered.

Link: https://lore.kernel.org/netdev/20260921084743.817859-1-roshaen09@gmail.com/
Link: https://lore.kernel.org/netdev/179023970333.2160803.2776986020964028023@kernel.org/

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index e538cc98e2572..954d28e7ec2e5 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c

[ ... ]

> @@ -957,7 +971,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
>  		xtfs->ra_wantseq++;
>  	} else {
>  		/* We are done with packet reassembly! */
> -		iptfs_reassem_done(xtfs);
> +		*dev_to_put = iptfs_reassem_done(xtfs);
>  		iptfs_complete_inner_skb(xtfs->x, newskb);
>  		list_add_tail(&newskb->list, list);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the completed reassembly skb
put on the list here can be leaked. The list is the sublist local in
iptfs_input_ordered(), which is then passed to __input_process_payload().

In __input_process_payload(), three error exits in the parse loop jump to
the done label:

	if (skb_copy_seq_read(skbseq, data, hbytes, iphlen)) {
		XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
		goto done;
	}
	...
			if (iplen < iphlen || iphlen < sizeof(*iph)) {
				XFRM_INC_STATS(net,
					       LINUX_MIB_XFRMINHDRERROR);
				goto done;
			}
	...
		} else {
			XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
			goto done;
		}

The done label comes after the send loop:

	/* Send the packets! */
	list_for_each_entry_safe(skb, next, sublist, list) {
		skb_list_del_init(skb);
		if (xfrm_input(skb, 0, 0, -2))
			kfree_skb(skb);
	}
done:

In that case, are the skbs already on sublist leaked?
__input_process_payload() returns true, so iptfs_input_ordered() frees
nothing, and nothing walks sublist afterwards.

Three kinds of skb would leak:

  - the reassembled skb queued here
  - inner packets built by iptfs_pskb_extract_seq() or
    iptfs_pskb_add_frags()
  - the reused root skb, which sits on sublist with defer == NULL and
    first_skb != NULL, so the cleanup after done does not free it

One ESP payload that holds a valid inner IPv4 packet followed by a byte with
version nibble 5 seems to leak one skb per packet. A peer on the SA can
repeat this.

With this patch applied, dev_to_put still drops the device reference of a
leaked reassembled skb, so only the skb memory leaks.

> @@ -1189,6 +1203,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  			spin_lock(&xtfs->drop_lock);
>  
>  			xtfs->ra_newskb = skb;
> +			netdev_hold(skb->dev, NULL, GFP_ATOMIC);
>  			xtfs->ra_wantseq = seq + 1;
>  			xtfs->ra_drop_time = ktime_get_raw_fast_ns() +
>  					     xtfs->drop_time_ns;

[Severity: High]
Can this overwrite an existing ra_newskb and leak its device reference?

xfrm_input() calls xfrm_inner_mode_input() after spin_unlock(&x->lock).
iptfs_input(), iptfs_drop_timer() and the reorder_win_size == 0 fast path
all run iptfs_input_ordered() outside drop_lock. So two CPUs can do ordered
processing for the same SA at the same time.

In iptfs_input_ordered(), this check runs before __input_process_payload(),
and drop_lock is released in between:

	if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {

Say CPU0 handles seq N and CPU1 handles seq N+1, both with blkoff == 0 and a
truncated inner packet. Both can see ra_newskb == NULL and both reach this
"Start reassembly" block:

CPU0                                  CPU1
xtfs->ra_newskb = skbA;
netdev_hold(skbA->dev, ...);
                                      xtfs->ra_newskb = skbB;
                                      netdev_hold(skbB->dev, ...);

The runt path in iptfs_reassem_cont() has the same problem. It also assigns
ra_newskb and takes a reference without checking for an existing one. In
addition, iptfs_input_save_runt() updates ra_runtlen and ra_wantseq without
holding drop_lock.

Before this patch, the overwrite leaked skbA. With it, every release path
only sees the current ra_newskb:

  - __iptfs_reassem_done()
  - iptfs_reassem_done()
  - iptfs_drop_timer()
  - iptfs_destroy_state()

Wouldn't skbA's device reference then leak for good? That would leave
unregister_netdevice() and netns cleanup waiting forever.

The commit message says references are released "on all completion,
timeout, abort, and state-destruction paths", but this overwrite path does
not seem to be covered. Should the existing ra_newskb be aborted before it is
replaced, or should ordered processing be serialized?

[ ... ]

> @@ -1490,6 +1509,7 @@ static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs,
>  	}
>  
>  	xtfs->w_saved[index].skb = inskb;
> +	netdev_hold(inskb->dev, NULL, GFP_ATOMIC);
>  	xtfs->w_savedlen = max(savedlen, index + 1);
>  	iptfs_set_window_drop_times(xtfs, index);
>  }

[Severity: High]
Once this reference is held, what releases it when the ingress device is
unregistered?

xfrm_iptfs.c has no netdevice notifier. The only xfrm NETDEV_UNREGISTER
handling is xfrm_dev_unregister():

net/xfrm/xfrm_device.c:xfrm_dev_unregister() {
	xfrm_dev_state_flush(dev_net(dev), dev, true);
	xfrm_dev_policy_flush(dev_net(dev), dev, true);
}

That only flushes offloaded states whose xso.dev matches, and
xfrm_net_pre_exit() only flushes policies.

This reference, like the ones taken in __reorder_future_shifts(),
iptfs_reassem_cont() and __input_process_payload(), is only released by the
drop timer, by ordered processing, or by iptfs_destroy_state(). Would
unregister_netdevice() then wait in netdev_wait_allrefs_any(), printing
"waiting for X to become free", until the drop deadline passes?

The deadline comes from XFRMA_IPTFS_DROP_TIME, which is a plain NLA_U32 with
no range check:

iptfs_user_init() {
	...
	if (attrs[XFRMA_IPTFS_DROP_TIME])
		xtfs->drop_time_ns =
			(u64)nla_get_u32(attrs[XFRMA_IPTFS_DROP_TIME]) *
			NSECS_IN_USEC;
	...
}

So the deadline can be as long as about 4295 seconds.

The stall can also last about twice that. When the timer fires,
iptfs_drop_timer()->iptfs_input_ordered()->__input_process_payload() can
start a new reassembly. That takes a new netdev_hold() on the same
unregistering device, with a fresh deadline of now + drop_time.

During netns teardown, default_device_exit_batch() runs before the xfrm state
cleanup, so cleanup_net() would block on these references. cleanup_net()
runs on the single-threaded netns workqueue. Could this stall namespace
destruction for the whole host?

The commit message says an unprivileged user can build this setup in a user
and network namespace. That user also picks the drop time.

Other code avoids pinning the device. IPv4 defrag stores the ifindex and
looks the device up again with dev_get_by_index_rcu(). nf_queue flushes
queued entries from a netdev notifier. Would one of these approaches work
here?

The commit message also does not mention this change in unregistration
behaviour.

[ ... ]

> @@ -1721,13 +1745,20 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
>  
>  	spin_unlock(&xtfs->drop_lock);
>  

[ ... ]

>  	if (count) {
>  		list_for_each_entry_safe(skb, next, &list, list) {
> +			struct net_device *dev = skb->dev;
> +
>  			skb_list_del_init(skb);
>  			iptfs_input_ordered(x, skb);
> +			netdev_put(dev, NULL);
>  		}
>  	}

[Severity: Medium]
This is a pre-existing issue, but is there an off-by-one in
__reorder_drop(), which builds the list processed here?

__reorder_future_fits() uses index = inseq - w_wantseq - 1, and
__reorder_this() shifts by count + 1. So w_saved[0] stands for
w_wantseq + 1.

When __reorder_drop() stops at an empty slot that has not expired, it runs
this:

	count = s - xtfs->w_saved;
	if (count) {
		xtfs->w_wantseq += count;

		/* Shift handled slots plus final empty slot into slot 0. */
		__vec_shift(xtfs, count);
	}

w_wantseq was already incremented once before the loop, so it now equals the
sequence number of that empty slot. The shift, however, leaves the empty slot
at index 0. Doesn't that put every saved entry one index too high?

For example, take w_wantseq = 10 and w_saved = [11, NULL, 13], where the slot
for 11 expires first:

  - __reorder_drop() sends 11 and stops at the empty slot for 12. It sets
    w_wantseq = 12 and shifts by 1, which leaves [NULL, 13].

  - Packet 12 arrives. __reorder_this() finds w_saved[0] empty and flushes
    nothing. It shifts by 1 and sets w_wantseq = 13, so 13 now sits at
    index 0.

  - Packet 13 is not delivered, even though it is the expected sequence.

  - Packet 14 arrives. __reorder_future_fits() finds index 0 occupied and
    frees 14 as "a dup of a future".

Can ordinary reordering cause legitimate packets to be dropped this way, or
held until the next timeout?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930050307.1978654-1-roshaen09%40gmail.com