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

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

[Runyu Xiao]

__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
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);
 	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

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

[Kuniyuki Iwashima]

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

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

[Runyu Xiao]

Hi,

Thanks for reviewing this.

On Mon, 14 Sep 2026, Runyu Xiao wrote:

> A controlled QEMU test with explicit 5 ms schedule injection reproduced
> 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.

Could you provide the repro ?

The reproducer uses an x86_64 QEMU guest with three network namespaces and
veth pairs. Proxy ARP and IP forwarding are enabled on the guest, and an
AF_PACKET helper sends valid broadcast ARP requests from a source namespace
through the proxy-ARP ingress interface. The same interface is repeatedly
brought down and up while the requests are being sent.

For the deterministic run, I built the baseline and fixed kernels from the
same source and temporarily enabled C0109_POC instrumentation. It adds a
5 ms delay at both of these points:

  - immediately before arp_process() calls pneigh_enqueue();
  - after __neigh_ifdown() observes an empty proxy queue and immediately
    before timer_delete_sync().

The guest was run with two vCPUs and 2 GiB of memory. The trigger performed
300 down/up cycles while sending the ARP requests. The test-only oracle
reported a failure when __neigh_ifdown() completed with proxy_queue non-empty
and proxy_timer not pending.

With the unfixed kernel, the run reached both paths and reported the oracle
772 times. With the fixed kernel, the same workload reached both paths but
reported the oracle zero times. The natural-load runs also reached both
paths, but did not hit the narrow interleaving without the temporary delay.
The delay is only schedule amplification for this race; it is not a claim
that a third-party workload triggers it frequently in normal operation.

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

A check of the device state in pneigh_enqueue() alone does not serialize the
queue insertion with the purge, empty check, and timer deletion. An enqueue
can pass that check while the device is still up, then be delayed until
after __neigh_ifdown() has observed an empty queue and before it deletes the
timer. The newly inserted skb would still be left without a timer.

There is also a second teardown path to consider. __neigh_ifdown() is used
by neigh_carrier_down(), where the device can remain administratively up and
netif_running() can still be true. The proxy_timer_lock serializes both the
per-device and carrier-down teardown paths with proxy queue insertion and
timer rearming. The timer callback does not take this lock, so
timer_delete_sync() can still wait for an in-flight callback without a lock
dependency.

I will include a shorter version of these reproduction details in v2's
commit message and can provide the complete QEMU harness if needed.

Regards,
Runyu