Re: [PATCH v2] xfrm: iptfs: hold a device reference while packets are queued
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-24 08:48:24
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider. Critical: 0 · High: 2 · 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] Unbounded net_device reference retention: the patch pins the ingress net_device for every skb parked in the reorder window… - [High] Refcount/UAF race: the newly added device reference for the in- progress reassembly skb is released before the skb's last use. - [Medium] The KASAN backtrace pasted in the commit message cannot have been produced by this code. - [Low] Undisclosed behavioural/hot-path change and split ownership convention. - [Low] Coding style (checkpatch error) in the modified hunk: iptfs_input_reorder() now braces the first arm of the if/else-if chain… -- xfrm: iptfs: hold a device reference while packets are queued This takes a net_device reference on skb->dev when an outer packet is parked in the IPTFS reorder window (w_saved) or kept as the in-progress reassembly skb (ra_newskb), and drops it when the retained skb is delivered or freed. Release sites are added in iptfs_drop_timer(), iptfs_input(), __iptfs_reassem_done() and iptfs_destroy_state(). A few questions about the commit message and the reference accounting below.
BUG: KASAN: use-after-free in xfrm_input+0x45d6/0x59c0 iptfs_complete_inner_skb __input_process_payload iptfs_input_ordered iptfs_drop_timer
[Severity: Medium]
Can this backtrace be produced by the current code? The frame order
presents iptfs_complete_inner_skb() as the caller of xfrm_input(), but
iptfs_complete_inner_skb() in net/xfrm/xfrm_iptfs.c only resets the
network and transport headers, sets ip_summed and fixes up DSCP/ECN and
skb->protocol:
net/xfrm/xfrm_iptfs.c:iptfs_complete_inner_skb() {
skb_reset_network_header(skb);
skb_reset_transport_header(skb);
skb->ip_summed = CHECKSUM_NONE;
...
skb->protocol = htons(ETH_P_IPV6);
}
}
The only xfrm_input() call on that path is in the delivery loop of
__input_process_payload():
if (xfrm_input(skb, 0, 0, -2))
kfree_skb(skb);
Both Reported-by: tags also name the patch author and there is no
Closes: tag or reproducer reference. Could the trace be replaced with
one that matches the code, or with a reproducer?
quoted hunk ↗ jump to hunk
diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c index 6920940a35b49..ce5c6170d249d 100644 --- a/net/xfrm/xfrm_iptfs.c +++ b/net/xfrm/xfrm_iptfs.c@@ -709,6 +709,8 @@ 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); + if (xtfs->ra_newskb) + dev_put(xtfs->ra_newskb->dev); if (free) kfree_skb(xtfs->ra_newskb); xtfs->ra_newskb = NULL;
[Severity: High]
The dev_put() here runs for both values of free, but in the free ==
false case the skb is not freed, it is handed on for delivery:
net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
...
} else {
/* We are done with packet reassembly! */
iptfs_reassem_done(xtfs); /* __iptfs_reassem_done(xtfs, false) */
iptfs_complete_inner_skb(xtfs->x, newskb);
list_add_tail(&newskb->list, list);
}
...
}
and the list is then delivered in __input_process_payload():
if (xfrm_input(skb, 0, 0, -2))
kfree_skb(skb);
xfrm_input() dereferences the device immediately:
net/xfrm/xfrm_input.c:xfrm_input() {
struct net *net = dev_net(skb->dev);
struct net_device *dev = skb->dev;
...
}
ra_newskb->dev was copied from an older outer packet in
iptfs_alloc_skb() ("xfrm_input resume needs dev and xfrm ext from tunnel
pkt"), so it need not be the device of the packet currently being
processed, and in the reorder_win_size == 0 fast path no reference is
held on the current outer skb either.
Can the dev_put() here therefore drop the last reference, so that
dev_net(skb->dev) in xfrm_input() reads a freed net_device? Would
moving it under the free branch, matching the ordering used in the new
iptfs_input() loop, keep the reference alive until after delivery?
struct net_device *dev = skb->dev;
skb_list_del_init(skb);
iptfs_input_ordered(x, skb);
dev_put(dev);
quoted hunk ↗ jump to hunk
@@ -845,6 +847,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq, goto abandon; } xtfs->ra_newskb = newskb; + dev_hold(newskb->dev); /* Copy the runt data into the buffer, but leave data * pointers the same as normal non-runt case. The extra `rrem`@@ -1161,6 +1164,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data, spin_lock(&xtfs->drop_lock); xtfs->ra_newskb = skb; + dev_hold(skb->dev); xtfs->ra_wantseq = seq + 1; if (!hrtimer_is_queued(&xtfs->drop_timer)) { /* softirq blocked lest the timer fire and interrupt us */@@ -1475,6 +1479,7 @@ static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs, } xtfs->w_saved[index].skb = inskb; + dev_hold(inskb->dev); xtfs->w_savedlen = max(savedlen, index + 1); iptfs_set_window_drop_times(xtfs, index); }
[Severity: High]
The commit message says:
"The drop timer and the reassembly queues are bounded by the
configured drop time, so the extra reference delays device
unregistration by at most that amount."
Does that bound hold for the window slots? __iptfs_reassem_done()
cancels the drop timer unconditionally, and that single hrtimer is
shared between reassembly and the reorder window:
hrtimer_try_to_cancel(&xtfs->drop_timer);
Once cancelled, iptfs_set_window_drop_times() only re-arms it when the
walk back reaches an empty slot 0:
if (savedlen > index + 1) {
/* we are below another, our drop time and the timer are already set */
return;
}
...
if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
IPTFS_HRTIMER_MODE);
So with an occupied lower slot, is the timer ever re-armed?
For example, with wantseq == N: packet N starts a fragmented inner
packet (ra_newskb set, timer armed), N+2 and N+4 arrive out of order and
are saved with dev_hold(), then N+1 arrives in order and __reorder_this()
flushes N+1 and N+2 and shifts, leaving N+4 in slot 0. If the
reassembly completes while processing N+1, iptfs_reassem_done() cancels
the timer while N+4 and its device reference are still parked. If the
peer stops sending, does unregister_netdevice() for that ingress device
then spin forever in netdev_wait_allrefs_any(), which also blocks the
global cleanup_net worker?
The runt path above looks similar: iptfs_reassem_cont() takes
dev_hold(newskb->dev) on the path entered from iptfs_input_save_runt(),
which only sets ra_runtlen and ra_wantseq and does not start the drop
timer, unlike the __input_process_payload() site which does. Is anything
other than iptfs_destroy_state() guaranteed to release that reference?
On the bound itself, drop_time_ns comes from XFRMA_IPTFS_DROP_TIME as
u32 microseconds without a clamp, so even in the well behaved case the
delay can be around 71 minutes. Is that worth mentioning in the
changelog?
quoted hunk ↗ jump to hunk
@@ -1610,6 +1615,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; + dev_hold(inskb->dev); iptfs_set_window_drop_times(xtfs, xtfs->w_savedlen - 1); /* if we don't have a slot0 then we must wait for it */@@ -1641,8 +1647,10 @@ static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs, } wantseq = xtfs->w_wantseq; - if (likely(inseq == wantseq)) + if (likely(inseq == wantseq)) { + dev_hold(inskb->dev); __reorder_this(xtfs, inskb, list); + }
[Severity: Low] This isn't a bug, but the changelog only mentions taking a reference "when a packet is placed in the reorder window or kept as the in-progress reassembly skb". This hold is on the in-order packet, which is never retained past iptfs_input(): it is released a few lines later in the delivery loop of iptfs_input(). Could the changelog mention that every in-order received packet now does a dev_hold()/dev_put() pair when reorder_win_size is non-zero, while the reorder_win_size == 0 fast path takes none? The ownership convention also differs between neighbouring helpers: iptfs_input_reorder() takes the hold for __reorder_this(), the callees take it in __reorder_future_fits() and __reorder_future_shifts(), and __reorder_past() plus the duplicate-future case take none, with freelist entries freed without dev_put(). Releases are spread over both delivery loops, __iptfs_reassem_done() and iptfs_destroy_state(). Would a short comment stating who owns the reference help, given the v2 note already records one near-miss leak from this ambiguity?
else if (inseq < wantseq)
^^^^ [Severity: Low] checkpatch reports an error for this: "else should follow close brace '}'". CodingStyle also asks that all arms of the chain use braces once one arm does.
__reorder_past(xtfs, inskb, freelist); else if ((inseq - wantseq) < nslots)
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921084743.817859-1-roshaen09%40gmail.com