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

[netdev-bot+sashiko@kernel.org]

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