net: neighbour: Avoid sending RTM_NEWNEIGH for a dead neighbor

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.

---

Notes:
v2:
- Drop the comment from the code.

 net/core/neighbour.c | 3 +++
 1 file changed, 3 insertions(+)

index d6e9c7c59a89..d013d9f4bf9d 100644
@@ -3593,6 +3593,9 @@ static void __neigh_notify(struct neighbour *n, int type, int flags,
 	int err = -ENOBUFS;
 	struct net *net;
 
+	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