Re: [PATCH net v3] xfrm: iptfs: avoid canceling reorder-window drop timer

[Steffen Klassert]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net v3] xfrm: iptfs: avoid canceling reorder-window drop timer ---
commit c07a5e7c7c7f82df32e93a21827b1fe442840cc7
Author: Lilly Aronleigh <lilly@xxxxxxxxxxxx>

xfrm: iptfs: avoid canceling reorder-window drop timer

This changes __iptfs_reassem_done() so that the shared xtfs->drop_timer is
only canceled when the reorder window has no saved packets, with the aim of
keeping the reorder-window timeout armed. Two observations below, one about
the resulting timer deadline and one about whitespace.

Link: https://lore.kernel.org/netdev/CAFrrV-O1fesaza+5_WqH8OciXRu9KKE4UsB6RcMamDu0j2-nBA@xxxxxxxxxxxxxx/T/#u

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 597aedeac26eb..c0d712b594a37 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -699,8 +699,12 @@ 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);
> +        /*
> +         * The drop timer also drives the reorder window timeout. Locking makes
> +         * a failed cancel harmless.
> +         */
> +        if (!xtfs->w_savedlen)
> +                hrtimer_try_to_cancel(&xtfs->drop_timer);

Discovered by: opus-5 · Missed by: gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
When the cancel is skipped, the timer stays queued with the expiry that
belonged to the reassembly that just finished, not with the reorder
window's deadline.  Can a subsequent reassembly then inherit that stale
expiry?

Both arming sites decline to arm when the timer is 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)) {
		hrtimer_start(&xtfs->drop_timer,
			      xtfs->drop_time_ns,
			      IPTFS_HRTIMER_MODE);
	}
	...
}

net/xfrm/xfrm_iptfs.c:iptfs_set_window_drop_times() {
	...
	if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
		hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
			      IPTFS_HRTIMER_MODE);
}

The sequence, with D = xtfs->drop_time_ns, all transitions under drop_lock:

t_r: an ordered payload ends in a partial inner packet, so ra_newskb is
     set and the timer is armed for t_r + D.

t_w: a future sequence is saved, iptfs_set_window_drop_times() records
     w_saved[].drop_time = t_w + D but does not re-arm because the timer
     is queued.

t_c (< t_r + D): the missing sequence arrives, __reorder_this() flushes it
     plus consecutive saved packets but at least one saved entry remains,
     and the flushed packet completes reassembly.  With this patch
     w_savedlen != 0, so the cancel is skipped and the timer remains armed
     at t_r + D, which is earlier than the window's own deadline.

t_n (~ t_c): the same flush ends in another partial inner packet, so a new
     reassembly starts, but __input_process_payload() sees
     hrtimer_is_queued() == true and does not arm t_n + D.

t_r + D: the callback fires.

At that point iptfs_drop_timer() takes the in-progress skb unconditionally:

net/xfrm/xfrm_iptfs.c:iptfs_drop_timer() {
	...
	/* Drop any in progress packet */
	skb = xtfs->ra_newskb;
	xtfs->ra_newskb = NULL;
	...
	if (skb)
		kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
	...
}

while __reorder_drop() just re-arms for the window, since its own deadline
has not passed yet:

net/xfrm/xfrm_iptfs.c:__reorder_drop() {
	if (xtfs->w_saved[0].drop_time > now)
		goto set_timer;
	...
set_timer:
		hrtimer_start(&xtfs->drop_timer,
			      xtfs->w_saved[0].drop_time - now,
			      IPTFS_HRTIMER_MODE);
}

So the only casualty is the freshly started reassembly, which still had
D - (t_c - t_r) of its budget left.  If its continuation sequence arrives
afterwards, iptfs_reassem_cont() drops the leading fragment silently:

	if (!runtlen && !xtfs->ra_newskb)
		return data + min(blkoff, remaining);

with no statistic incremented, so a valid inner packet is lost that would
have been reassembled before this change.  Before the patch the
unconditional cancel left the timer unqueued at reassembly completion, so
each new reassembly armed its own deadline.

Would re-programming the timer to the remaining window deadline be closer to
what is wanted here, for example cancelling and then:

	hrtimer_start(&xtfs->drop_timer,
		      xtfs->w_saved[0].drop_time - ktime_get_raw_fast_ns(),
		      IPTFS_HRTIMER_MODE);

Related to this, the resulting state (timer armed with a deadline belonging
to neither the current reassembly nor slot 0) does not match the
iptfs_drop_timer() kernel-doc:

 * 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.

Should that comment be updated, and should the commit message mention the
new case?  Also, the new comment says "Locking makes a failed cancel
harmless", but the concern with the shared timer here is the leftover
deadline rather than a cancel that fails, so is that rationale the right one
for this change?

Discovered by: opus-5 · Missed by: gpt-5-6-sol, sashiko-gemini · Confirmed by: gpt-5-6-sol
This isn't a bug, but the six added lines in __iptfs_reassem_done() are
indented with spaces (eight for the comment and the if, sixteen for the
hrtimer_try_to_cancel() call) while the surrounding lines of the same
function use tabs, so the function ends up with mixed indentation.
checkpatch.pl flags this as "code indent should use tabs where possible" and
"please, no spaces at the start of a line".

The added multi-line comment also opens with a bare /* line, while the other
multi-line comments in net/xfrm/xfrm_iptfs.c start the text on the /* line.

>  	if (free)
>  		kfree_skb(xtfs->ra_newskb);
>  	xtfs->ra_newskb = NULL;