* Re: [PATCH net] amt: fix tunnel list corruption on device stop
2026-08-22 4:54 [PATCH net] amt: fix tunnel list corruption on device stop Cen Zhang (Microsoft)
@ 2026-08-24 16:30 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-08-24 16:30 UTC (permalink / raw)
To: blbllhy
Cc: Simon Horman, ap420073, andrew+netdev, davem, edumazet, kuba,
pabeni, netdev, linux-kernel, AutonomousCodeSecurity, xmei5,
tgopinath, kys, stable
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260822045407.28983-1-blbllhy@gmail.com?part=1
---
amt: fix tunnel list corruption on device stop
This changes amt_dev_stop() to unlink tunnels under amt->lock using
list_del_init(), adds a list_empty() ownership check to
amt_tunnel_expire(), clears sk_user_data before synchronize_net(), and
switches the per-tunnel GC work to disable_delayed_work_sync().
A few questions on the changelog and on the new drain loop below.
First, about the two justifications given in the changelog:
> 1. Quiesce RX: clear sk_user_data and call synchronize_net() to ensure
> no RCU readers are traversing tunnel_list. This makes list_del_init()
> safe (it is not RCU-reader-safe unlike list_del_rcu()).
[Severity: Medium]
Is this claim accurate for all readers of amt->tunnel_list?
amt_dev_xmit() traverses the same list in relay mode:
drivers/net/amt.c:amt_dev_xmit() {
...
list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
hash = amt_group_hash(tunnel, &group);
...
}
That reader is not reached through amt->sk / sk_user_data, so clearing
sk_user_data and calling synchronize_net() does not cover it.
> 4. Use disable_delayed_work_sync() instead of cancel_delayed_work_sync()
> to prevent amt_update_handler() from rearming the GC timer in a rare
> race where a packet arrives before the socket is fully released.
Can this window still exist after the change in point 1?
amt_update_handler() and amt_request_handler() are only called from
amt_rcv(), which resolves amt via rcu_dereference_sk_user_data(sk) inside
rcu_read_lock_bh(). Once sk_user_data is NULLed and synchronize_net() has
returned, amt_rcv() can no longer find amt, so the packet-driven
mod_delayed_work(amt_wq, &tunnel->gc_wq, ...) in amt_update_handler()
cannot run any more.
So point 1 says no RCU reader can be traversing tunnel_list, while point 4
describes a packet reaching amt_update_handler(), which can only reach that
mod_delayed_work() from inside list_for_each_entry_rcu() over
amt->tunnel_list. Could the changelog be made consistent about which
readers the quiesce actually covers, given that this is the stated reason
for dropping the RCU-safe removal primitive and the patch is tagged for
stable?
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index bddc24e1856d..a5db02d81291 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1348,6 +1348,11 @@ static void amt_tunnel_expire(struct work_struct *work)
> struct amt_dev *amt = tunnel->amt;
>
> spin_lock_bh(&amt->lock);
> + /* amt_dev_stop() marks tunnels it owns with list_del_init(). */
> + if (list_empty(&tunnel->list)) {
> + spin_unlock_bh(&amt->lock);
> + return;
> + }
> rcu_read_lock();
> list_del_rcu(&tunnel->list);
> amt->nr_tunnels--;
[Severity: High]
This isn't a bug introduced by this patch, but does the new list_empty()
guard cover the case where the GC work is rearmed while it is already
running?
amt_update_handler() rearms the work under rcu_read_lock_bh() only, without
amt->lock:
drivers/net/amt.c:amt_update_handler() {
list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
if (tunnel->ip4 == saddr) {
if ((nonce == tunnel->nonce &&
response_mac == tunnel->mac)) {
mod_delayed_work(amt_wq, &tunnel->gc_wq,
msecs_to_jiffies(amt_gmi(amt))
* 3);
...
}
Once process_one_work() has cleared the work's PENDING bit,
mod_delayed_work() arms the delayed timer again, and amt_tunnel_expire()
then proceeds to list_del_rcu() + kfree_rcu(tunnel, rcu) with
tunnel->gc_wq.timer still armed. The rearm delay is amt_gmi() * 3, which is
far longer than the grace period behind kfree_rcu(), so the timer_list
inside the freed tunnel stays linked in the timer wheel.
The drain loop in amt_dev_stop() does not help here, because the first
callback has already removed the tunnel from amt->tunnel_list, so
disable_delayed_work_sync() is never called for it.
And if the work does run again on still-mapped memory, the new guard cannot
detect it either: list_del_rcu() leaves entry->next unchanged, so
list_empty(&tunnel->list) is false and a second list_del_rcu() runs with
prev == LIST_POISON2, which is the same lib/list_debug.c BUG this patch is
fixing.
> @@ -3097,13 +3104,21 @@ static int amt_dev_open(struct net_device *dev)
> amt->req_cnt = 0;
> WRITE_ONCE(amt->remote_ip, 0);
>
> - list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
> - list_del_rcu(&tunnel->list);
> + spin_lock_bh(&amt->lock);
> + while (!list_empty(&amt->tunnel_list)) {
> + tunnel = list_first_entry(&amt->tunnel_list,
> + struct amt_tunnel_list, list);
> + list_del_init(&tunnel->list);
> amt->nr_tunnels--;
[Severity: High]
Can this wedge a concurrent amt_dev_xmit()?
list_del_init() points the node at itself:
include/linux/list.h:list_del_init() {
__list_del_entry(entry);
INIT_LIST_HEAD(entry);
}
amt->tunnel_list is published with list_add_tail_rcu() in
amt_request_handler() and read locklessly in amt_dev_xmit():
drivers/net/amt.c:amt_dev_xmit() {
...
list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
...
}
A reader parked on the node being unlinked gets itself back from
list_next_entry_rcu() and never reaches the list head, so it spins forever
with BH disabled inside an RCU read-side section. That also stalls the
grace period behind the kfree_rcu(tunnel, rcu) calls in the same loop.
Is this TX reader quiesced anywhere? amt_link_setup() sets:
drivers/net/amt.c:amt_link_setup() {
dev->priv_flags |= IFF_NO_QUEUE;
dev->lltx = true;
...
}
With the noqueue qdisc, noqueue_init() leaves qdisc->enqueue == NULL, so
dev_deactivate_queue() never sets *sync_needed and dev_deactivate_many()
skips its synchronize_net() for this device. And __dev_direct_xmit(), used
by AF_PACKET PACKET_QDISC_BYPASS and xsk generic xmit, tests liveness
before disabling BH:
net/core/dev.c:__dev_direct_xmit() {
if (unlikely(!netif_running(dev) ||
!netif_carrier_ok(dev)))
goto drop;
skb = validate_xmit_skb_list(skb, dev, &again);
...
local_bh_disable();
...
}
so a sender can pass the check, be preempted across __dev_close_many() and
amt_dev_stop()'s synchronize_net(), and only then enter amt_dev_xmit().
The previous list_del_rcu() poisoned only entry->prev and left entry->next
intact, so a racing reader still terminated. Would keeping list_del_rcu()
here (and using a different marker for the ownership test in
amt_tunnel_expire()) avoid this?
> - cancel_delayed_work_sync(&tunnel->gc_wq);
> + spin_unlock_bh(&amt->lock);
> +
> + disable_delayed_work_sync(&tunnel->gc_wq);
> amt_clear_groups(tunnel);
> kfree_rcu(tunnel, rcu);
[Severity: High]
This is a pre-existing issue, but the works nested under the tunnel are
still only cancelled with the non-waiting cancel_delayed_work() while the
tunnel-level work is now upgraded to disable_delayed_work_sync().
amt_clear_groups() -> amt_del_group() does:
drivers/net/amt.c:amt_del_group() {
if (cancel_delayed_work(&gnode->group_timer))
dev_put(amt->dev);
hlist_del_rcu(&gnode->node);
...
kfree_rcu(gnode, rcu);
}
and amt_destroy_source() does the same for the source timer:
drivers/net/amt.c:amt_destroy_source() {
cancel_delayed_work(&snode->source_timer);
hlist_del_init_rcu(&snode->node);
...
}
cancel_delayed_work() returns false once process_one_work() has cleared
PENDING, and it is never waited for. The callbacks dereference the objects
and take tunnel->lock before entering any RCU read-side section:
drivers/net/amt.c:amt_group_work() {
struct amt_tunnel_list *tunnel = gnode->tunnel_list;
struct amt_dev *amt = gnode->amt;
...
spin_lock_bh(&tunnel->lock);
...
}
drivers/net/amt.c:amt_source_work() {
struct amt_group_node *gnode = snode->gnode;
struct amt_dev *amt = gnode->amt;
...
tunnel = gnode->tunnel_list;
spin_lock_bh(&tunnel->lock);
rcu_read_lock();
...
}
A worker preempted between PENDING being cleared and spin_lock_bh() is in
plain preemptible process context, not an RCU reader, so the grace periods
behind kfree_rcu(gnode, rcu) and kfree_rcu(tunnel, rcu) can complete. When
it resumes it takes spin_lock_bh() on freed memory, reads gnode fields from
freed memory, and can call dev_put(amt->dev) derived from that state.
Would it make sense to use the sync cancel for the group and source timers
too, so the cancel-before-free ordering is complete for everything nested
under the tunnel?
> }
> + spin_unlock_bh(&amt->lock);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 2+ messages in thread