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

[netdev-bot+sashiko@kernel.org]

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