Re: [PATCH net] amt: fix tunnel list corruption on device stop

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/[email protected]?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;
>  }