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

[Kuniyuki Iwashima]

On Mon, Sep 21, 2026 at 12:01 AM Runyu Xiao <runyu.xiao@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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