Re: [PATCH net] net: neighbour: Serialize proxy timer teardown

[Kuniyuki Iwashima <kuniyu@google.com>]

From: Runyu Xiao <runyu.xiao@seu.edu.cn>
Date: Mon, 14 Sep 2026 15:56:04 +0800
> __neigh_ifdown() purges device-specific entries and then checks
> whether the shared proxy queue is empty before deleting proxy_timer. A
> concurrent ARP or NDISC request can enqueue a delayed proxy response
> between the check and timer_delete_sync(). The timer deletion can then
> leave the new skb queued without a timer to process it.
> 
> Serialize proxy queue insertion and timer rearming with the purge, empty
> check, and timer deletion. The timer callback does not take the new lock,
> so timer_delete_sync() can still wait for an in-flight callback. Keep
> timer_delete_sync() rather than timer_shutdown_sync(), since the timer is
> reused after per-device and carrier teardown.
> 
> A controlled QEMU test with explicit 5 ms schedule injection reproduced

Could you provide the repro ?


> the orphaned queue state on the unfixed kernel and did not reproduce it
> with this change. Natural-load runs reached both paths but did not
> trigger the narrow race.
> 
> Fixes: 66ba215cb513 ("neigh: fix possible DoS due to net iface start/stop loop")
> Assisted-by: LLM Codex
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  include/net/neighbour.h | 2 ++
>  net/core/neighbour.c    | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 8860cc2175fc1..15107c00b4df4 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -239,6 +239,8 @@ struct neigh_table {
>  	struct list_head	gc_list;
>  	struct list_head	managed_list;
>  	spinlock_t		lock;
> +	/* Serializes proxy timer rearming with teardown. */
> +	spinlock_t		proxy_timer_lock;
>  	unsigned long		last_rand;
>  	struct neigh_statistics	__percpu *stats;
>  	struct neigh_hash_table __rcu *nht;
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 1349c0eedb642..1ea794a541efb 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -471,10 +471,13 @@ static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
>  	spin_unlock_bh(&tbl->lock);
>  
>  	pneigh_ifdown(tbl, dev, skip_perm);
> +	/* The callback does not take this lock, so sync deletion can wait for it. */
> +	spin_lock_bh(&tbl->proxy_timer_lock);
>  	pneigh_queue_purge(&tbl->proxy_queue, dev ? dev_net(dev) : NULL,
>  			   tbl->family);
>  	if (skb_queue_empty_lockless(&tbl->proxy_queue))
>  		timer_delete_sync(&tbl->proxy_timer);
> +	spin_unlock_bh(&tbl->proxy_timer_lock);
>  	return 0;
>  }
>  
> @@ -1729,6 +1732,7 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
>  	NEIGH_CB(skb)->sched_next = sched_next;
>  	NEIGH_CB(skb)->flags |= LOCALLY_ENQUEUED;
>  
> +	spin_lock_bh(&tbl->proxy_timer_lock);
>  	spin_lock(&tbl->proxy_queue.lock);

Isn't is resolved by simply checking if dev is down here ?



>  	if (timer_delete(&tbl->proxy_timer)) {
>  		if (time_before(tbl->proxy_timer.expires, sched_next))
> @@ -1740,6 +1744,7 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
>  	p->qlen++;
>  	mod_timer(&tbl->proxy_timer, sched_next);
>  	spin_unlock(&tbl->proxy_queue.lock);
> +	spin_unlock_bh(&tbl->proxy_timer_lock);
>  }
>  EXPORT_SYMBOL(pneigh_enqueue);
>  
> @@ -1858,6 +1863,7 @@ void neigh_table_init(int index, struct neigh_table *tbl)
>  		WARN_ON(tbl->entry_size % NEIGH_PRIV_ALIGN);
>  
>  	spin_lock_init(&tbl->lock);
> +	spin_lock_init(&tbl->proxy_timer_lock);
>  	mutex_init(&tbl->phash_lock);
>  
>  	INIT_DEFERRABLE_WORK(&tbl->gc_work, neigh_periodic_work);
> -- 
> 2.34.1