[PATCH net-next] net: neighbour: Avoid sending RTM_NEWNEIGH for a dead neighbor

[PATCH net-next] net: neighbour: Avoid sending RTM_NEWNEIGH for a dead neighbor

[Petr Machata]

During the processing of neighbor timer, in neigh_timer_handler(),
when the number of probes for an unresolved neighbor is exceeded, a
non-ext_validated neighbor is marked as failed. To send the ICMP /
ICMPv6 destination unreachable diagnostic, neigh_invalidate() unlocks,
and gives opportunity for a concurrent update to mark the neighbor as
dead and send an RTM_DELNEIGH notification. After the lock is
regained, neigh_timer_handler() proceeds to send its own RTM_NEWNEIGH
notification, this time on a dead neighbor.

Sending an RTM_NEWNEIGH after the corresponding RTM_DELNEIGH has the
obvious semantics of recreating the neighbor, which is not what we want to
convey here.

Userspace can in theory recognize these faulty RTM_NEWNEIGH reports by
the state, which will be NUD_NONE, but there is no reason why it
should.

The following diagram, which illustrates the issue, comes from Andy
Roulin:

timer handler (softirq)                neigh_flush_dev() (linkwatch)
write_lock(n->lock)
nud_state = NUD_FAILED
neigh_invalidate():
  write_unlock(n->lock)
  error_report()
                                       write_lock(n->lock)
                                       unlink, n->dead = 1
                                       refcnt != 1 -> nud_state = NUD_NONE
                                       write_unlock(n->lock)
                                       RTM_DELNEIGH
  write_lock(n->lock)
write_unlock(n->lock)
RTM_NEWNEIGH: state 0, no lladdr, ndm_refcnt 0   <- stale
neigh_release() -> neigh_destroy()

Fix the problem by refusing to send any notification other than
RTM_DELNEIGH when the neighbor is dead.

The issue comes from a user report and was obseved in the wild. It was
reproduced on a vanilla kernel by extending the race window by inserting an
mdelay for neighbors on a device with a hardcoded name.

It is tricky to track down where the issue was actually introduced, and
practically speaking, it may have been present "always". At least as far
back as 5.10, which is currently the oldest LTS-supported kernel release,
the code still looks substantially the same: neighbor timer causes neighbor
invalidation, which unlocks to send ICMP, which a flush can then race. Even
2.6 has elements that look like they could combine into the same race. I am
therefore not citing any particular Fixes tag.

Assisted-by: LLM
Co-developed-by: Andy Roulin <aroulin@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 net/core/neighbour.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d6e9c7c59a89..6fa490f7652a 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -3593,6 +3593,13 @@ static void __neigh_notify(struct neighbour *n, int type, int flags,
 	int err = -ENOBUFS;
 	struct net *net;
 
+	/* An attempt to send anything but RTM_DELNEIGH on a dead neighbor
+	 * indicates that the neighbor was marked dead while another update
+	 * gave the lock up for a bit. Do not send such notifications.
+	 */
+	if (n->dead && type != RTM_DELNEIGH)
+		return;
+
 	rcu_read_lock();
 	net = dev_net_rcu(n->dev);
 	skb = nlmsg_new(neigh_nlmsg_size(), GFP_ATOMIC);
-- 
2.56.0

Re: [PATCH net-next] net: neighbour: Avoid sending RTM_NEWNEIGH for a dead neighbor

[Kuniyuki Iwashima]

On Wed, 04 Nov 2026 14:56:56 +0100, Petr Machata wrote:
> During the processing of neighbor timer, in neigh_timer_handler(),
> when the number of probes for an unresolved neighbor is exceeded, a
> non-ext_validated neighbor is marked as failed. To send the ICMP /
> ICMPv6 destination unreachable diagnostic, neigh_invalidate() unlocks,
> and gives opportunity for a concurrent update to mark the neighbor as
> dead and send an RTM_DELNEIGH notification. After the lock is
> regained, neigh_timer_handler() proceeds to send its own RTM_NEWNEIGH
> notification, this time on a dead neighbor.
> 
> Sending an RTM_NEWNEIGH after the corresponding RTM_DELNEIGH has the
> obvious semantics of recreating the neighbor, which is not what we want to
> convey here.
> 
> Userspace can in theory recognize these faulty RTM_NEWNEIGH reports by
> the state, which will be NUD_NONE, but there is no reason why it
> should.
> 
> The following diagram, which illustrates the issue, comes from Andy
> Roulin:
> 
> timer handler (softirq)                neigh_flush_dev() (linkwatch)
> write_lock(n->lock)
> nud_state = NUD_FAILED
> neigh_invalidate():
>   write_unlock(n->lock)
>   error_report()
>                                        write_lock(n->lock)
>                                        unlink, n->dead = 1
>                                        refcnt != 1 -> nud_state = NUD_NONE
>                                        write_unlock(n->lock)
>                                        RTM_DELNEIGH
>   write_lock(n->lock)
> write_unlock(n->lock)
> RTM_NEWNEIGH: state 0, no lladdr, ndm_refcnt 0   <- stale
> neigh_release() -> neigh_destroy()
> 
> Fix the problem by refusing to send any notification other than
> RTM_DELNEIGH when the neighbor is dead.
> 
> The issue comes from a user report and was obseved in the wild. It was
> reproduced on a vanilla kernel by extending the race window by inserting an
> mdelay for neighbors on a device with a hardcoded name.
> 
> It is tricky to track down where the issue was actually introduced, and
> practically speaking, it may have been present "always". At least as far
> back as 5.10, which is currently the oldest LTS-supported kernel release,
> the code still looks substantially the same: neighbor timer causes neighbor
> invalidation, which unlocks to send ICMP, which a flush can then race. Even
> 2.6 has elements that look like they could combine into the same race. I am
> therefore not citing any particular Fixes tag.
> 
> Assisted-by: LLM
> Co-developed-by: Andy Roulin <aroulin@nvidia.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Andy Roulin <aroulin@nvidia.com>
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>  net/core/neighbour.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d6e9c7c59a89..6fa490f7652a 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -3593,6 +3593,13 @@ static void __neigh_notify(struct neighbour *n, int type, int flags,
>  	int err = -ENOBUFS;
>  	struct net *net;
>  
> +	/* An attempt to send anything but RTM_DELNEIGH on a dead neighbor
> +	 * indicates that the neighbor was marked dead while another update
> +	 * gave the lock up for a bit. Do not send such notifications.
> +	 */

I'd remove the comment, it's apparent from ->dead that there
was a race and the rest is just repeating the condition bleow.

Otherwise looks good.

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>



> +	if (n->dead && type != RTM_DELNEIGH)
> +		return;
> +
>  	rcu_read_lock();
>  	net = dev_net_rcu(n->dev);
>  	skb = nlmsg_new(neigh_nlmsg_size(), GFP_ATOMIC);
> -- 
> 2.56.0
> 

Re: [PATCH net-next] net: neighbour: Avoid sending RTM_NEWNEIGH for a dead neighbor

[Petr Machata]

Kuniyuki Iwashima <kuniyu@google.com> writes:

> On Wed, 04 Nov 2026 14:56:56 +0100, Petr Machata wrote:
>
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -3593,6 +3593,13 @@ static void __neigh_notify(struct neighbour *n, int type, int flags,
>>  	int err = -ENOBUFS;
>>  	struct net *net;
>>  
>> +	/* An attempt to send anything but RTM_DELNEIGH on a dead neighbor
>> +	 * indicates that the neighbor was marked dead while another update
>> +	 * gave the lock up for a bit. Do not send such notifications.
>> +	 */
>
> I'd remove the comment, it's apparent from ->dead that there
> was a race and the rest is just repeating the condition bleow.

I figured it might be useful to clarify how it can come up, but yeah.
I'll respin without the comment.

> Otherwise looks good.
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thanks!