__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 check of netif_running() in pneigh_enqueue() alone would not serialize
the insertion with the purge, empty check, and timer deletion. An enqueue
can pass that check while the device is still up and then be delayed until
after __neigh_ifdown() has observed an empty queue. Also, __neigh_ifdown()
is used by neigh_carrier_down(), where netif_running() can remain true.

Tested in an x86_64 QEMU guest with proxy ARP, three network namespaces,
and veth pairs. An AF_PACKET helper sent valid broadcast ARP requests while
the proxy-ARP ingress interface was repeatedly brought down and up. A
temporary test-only build flag added a 5 ms delay before pneigh_enqueue()
and after the empty-queue check before timer_delete_sync() to amplify the
narrow scheduling window.

The unfixed kernel reached both paths and observed a non-empty proxy_queue
with proxy_timer not pending 772 times. The fixed kernel reached both paths
under the same workload without observing this state. Natural-load runs
reached both paths but did not hit the narrow race without the temporary
delay.

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>
---
v2:
- Add the QEMU reproduction and buggy/fixed results.
- Explain why a device-state check does not serialize carrier teardown.

 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 8860cc2175fc1c9eb6b67f2c5ef149188c9feb16..15107c00b4df473e53e6cf2e757fb2ef3c4fd3f7 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 1349c0eedb642539b28391390fd82f543e004353..1ea794a541efb6852700062da2e25efac6c060e8 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

On Mon, Sep 21, 2026 at 12:01 AM Runyu Xiao <runyu.xiao@seu.edu.cn> wrote:
>
> __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 check of netif_running() in pneigh_enqueue() alone would not serialize
> the insertion with the purge, empty check, and timer deletion. An enqueue
> can pass that check while the device is still up and then be delayed until
> after __neigh_ifdown() has observed an empty queue. Also, __neigh_ifdown()
> is used by neigh_carrier_down(), where netif_running() can remain true.

By "dev is down" I meant whatever that creates ordering between
the teardown thread and insertion, but I missed it's still racy by multiple
devs.

But still, I think adding a spinlock is not worth it, see below.


>
> Tested in an x86_64 QEMU guest with proxy ARP, three network namespaces,
> and veth pairs. An AF_PACKET helper sent valid broadcast ARP requests while
> the proxy-ARP ingress interface was repeatedly brought down and up. A
> temporary test-only build flag added a 5 ms delay before pneigh_enqueue()
> and after the empty-queue check before timer_delete_sync() to amplify the
> narrow scheduling window.
>
> The unfixed kernel reached both paths and observed a non-empty proxy_queue
> with proxy_timer not pending 772 times. The fixed kernel reached both paths
> under the same workload without observing this state. Natural-load runs
> reached both paths but did not hit the narrow race without the temporary
> delay.
>
> 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>
> ---
> v2:
> - Add the QEMU reproduction and buggy/fixed results.
> - Explain why a device-state check does not serialize carrier teardown.
>
>  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 8860cc2175fc1c9eb6b67f2c5ef149188c9feb16..15107c00b4df473e53e6cf2e757fb2ef3c4fd3f7 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 1349c0eedb642539b28391390fd82f543e004353..1ea794a541efb6852700062da2e25efac6c060e8 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);

I'd simply remove these two lines.

In net.git, arp_tbl/nd_tbl is still global, and we do not need to worry
about timer touching the table after being freed.  Even if skb was
put into the queue after pneigh_queue_purge(), it's harmless
since skb holds dev refcnt and neigh_proxy_process() will just free it.

In net-next.git, we call timer_shutdown_sync() in neigh_table_clear(),
where prior synchronize_rcu() guarantees there are no inflight arp/ND,
and then neigh_ifdown() is called and pneigh_queue_purge() purges
the queue.



> +       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