Re: [PATCH net v5 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[Omar Ramadan <omar@blockcast.net>]

On Mon, 09 Nov 2026 14:49:16 +0000, netdev-bot+sashiko@kernel.org wrote:
> - [High] amt_tunnel_expire() (drivers/net/amt.c:1412) runs on amt_wq.
> - [High] amt_del_group() (drivers/net/amt.c:293) used to free the group
>   node with kfree_rcu().

v6 has superseded this posting; this patch is v6 2/6:
https://lore.kernel.org/netdev/20261107004935.2627329-3-omar@blockcast.net/

Answers below are against v6 and the v7 I am about to post.

> Can this flush deadlock amt_wq? amt_tunnel_expire() runs on amt_wq, and
> so do group_gc_wq and source_gc_wq.

Fixed in v6, and v7 keeps it. Both flushes are gone:
amt_tunnel_expire() and amt_dev_stop() call __amt_group_gc_work()
directly, and the group GC calls __amt_source_gc_work() directly
instead of flushing source_gc_wq. The cancel_delayed_work_sync() calls
left in the GC queue nothing: each takes a pending timer work off its
timer or queue, or waits for a running one on a barrier that
max_active does not count.

> What limits the number of deleted group nodes waiting on group_gc_list?

Nothing, and v6 still has this. max_groups bounds live groups, not
parked ones, and before this series a deleted group went to
kfree_rcu(), so the series introduces the regression.

v7 counts the nodes parked on both GC lists, under the one lock that
now protects both, and queues the drain of both lists at once when the
count reaches AMT_GC_KICK (2048). A parked group keeps its 256 source
hash heads, so it is a kmalloc-4k object, and 2048 of them is about
8 MiB; the kick keeps the backlog near that instead of letting it grow
for up to 30 seconds. A periodic drain that ends with that many parked
again re-arms at 0 rather than after the interval.

> When the GC finally runs, __amt_group_gc_work() walks the whole spliced
> list with no cond_resched():

v7 adds cond_resched() after each node in both drains. They run only in
process context, with no spinlock or RCU read-side section held. The
kick also bounds what amt_dev_stop() frees in its drain under RTNL.

> Is 0 safe to use here as the "never refreshed" value for last_seen?

Fixed in v6: 2/6 stores "jiffies ?: 1", and 6/6, where every tunnel is
stamped at creation, drops the special case.

> Could the commit message describe each of these failures? Alternatively,
> could they be split into separate patches, each with its own Fixes tag,

v7 keeps one patch, since the tunnel and node fixes share the same
helpers and the same tunnel->lock rules, but its message now names each
failure: (1) as the amt_dev_stop()/amt_tunnel_expire() double
list_del_rcu() and free, (3) and (4) as frees after only
cancel_delayed_work(), and two more races in the timer work: parent
pointers read before tunnel->lock or RCU, and a second destroy or
delete after waiting on tunnel->lock. It carries both tags:

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Fixes: bc54e49c140b ("amt: add multicast(IGMP) report message handler")

Both went into v5.16, so stable reach is unchanged.

On (2): every walker of a group's source hash, the report handlers and
amt_group_work(), holds tunnel->lock, as does every caller of
amt_destroy_source(), so I don't find a reader that can follow
snode->node onto the GC list. v7 still gives sources and groups a
gc_node of their own, and says it is hardening.