Re: [PATCH net-next v2 1/2] ipv6: update NUD_FAILED neighbors from NA messages

On Wed Sep 16, 2026 at 12:02 PM UTC, Ido Schimmel wrote:

On Tue, Sep 15, 2026 at 05:01:31AM +0000, Lawrence Lee wrote:

quoted

Transition a FAILED neighbor entry to STALE upon receipt of an NA
message on routers when accept_untracked_na is enabled. This extends the
RFC 9131 accept_untracked_na behavior so that FAILED entries are treated
the same as non-existent entries. In the context of RFC 4861 which
introduced NDP, both non-existent and FAILED entries are considered
untracked since they do not have a valid neighbor cache entry.

RFC 4861 didn't introduce NDP (RFC 1970 did), so please omit this bit.
But if you're going to mention RFC 4861, then cite 7.3.3 which says that
"If address resolution fails, the entry SHOULD be deleted". The fact
that Linux keeps it as FAILED is an implementation detail and treating
it as untracked is correct from RFC perspective.

That's my mistake, I'll update this to mention that 4861 is just the 
most recent NDP standard and will cite 7.3.3.

quoted

Trying to resolve FAILED neighbors via periodic probing (e.g. using
NTF_EXT_MANAGED) is more work compared to this approach which uses
information in NAs that the kernel may already be receiving. Note that
because this behavior in IPv6 is dependent on the accept_untracked_na
sysctl setting, this approach is more conservative than IPv4 which
transitions FAILED neighbors to STALE by default upon receiving GARPs.

Link: https://lore.kernel.org/r/20260813233344.445265-1-lfqlee314@gmail.com (local)
Assisted-by: LLM Sashiko sparse
Signed-off-by: Lawrence Lee <redacted>
---
 Documentation/networking/ip-sysctl.rst | 28 ++++----
 include/net/ndisc.h                    | 15 ++--
 net/6lowpan/ndisc.c                    | 15 ++--
 net/ipv6/ndisc.c                       | 94 +++++++++++++++++---------
 4 files changed, 96 insertions(+), 56 deletions(-)

The RFC was:

1 file changed, 11 insertions(+), 2 deletions(-)

I'm not sure how this ballooned to this size...

Some of the additional size can be attributed to doc/comment changes, 
but a lot of it comes from changes I implemented to address issues found 
by the local Sashiko review I ran. It was definitely an oversight on my 
part to not mention these in either the commit message or comments, 
sorry about that.

quoted

diff --git a/include/net/ndisc.h b/include/net/ndisc.h
index 96e3bb6e83af..7fb3f10eca6c 100644
--- a/include/net/ndisc.h
+++ b/include/net/ndisc.h

@@ -154,11 +154,13 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, const void *data,
  *     option parser will take care about that option.
  *
  * void (*update)(const struct net_device *dev, struct neighbour *n,
- *		  u32 flags, u8 icmp6_type,
+ *		  u32 flags, bool failed_recovery, u8 icmp6_type,
  *		  const struct ndisc_options *ndopts):
  *     This function is called when IPv6 ndisc updates the neighbour cache
  *     entry. Additional options which can be updated may be previously
  *     parsed by parse_opts callback and accessible over ndopts parameter.
+ *     failed_recovery indicates that ndisc accepted the packet to recover
+ *     an entry observed in NUD_FAILED.
  *
  * int (*opt_addr_space)(const struct net_device *dev, u8 icmp6_type,
  *			 struct neighbour *neigh, u8 *ha_buf,

@@ -197,7 +199,7 @@ struct ndisc_ops {
 				 struct nd_opt_hdr *nd_opt,
 				 struct ndisc_options *ndopts);
 	void	(*update)(const struct net_device *dev, struct neighbour *n,
-			  u32 flags, u8 icmp6_type,
+			  u32 flags, bool failed_recovery, u8 icmp6_type,
 			  const struct ndisc_options *ndopts);
 	int	(*opt_addr_space)(const struct net_device *dev, u8 icmp6_type,
 				  struct neighbour *neigh, u8 *ha_buf,

@@ -227,12 +229,13 @@ static inline int ndisc_ops_parse_options(const struct net_device *dev,
 }
 
 static inline void ndisc_ops_update(const struct net_device *dev,
-					  struct neighbour *n, u32 flags,
-					  u8 icmp6_type,
-					  const struct ndisc_options *ndopts)
+				    struct neighbour *n, u32 flags,
+				    bool failed_recovery, u8 icmp6_type,
+				    const struct ndisc_options *ndopts)
 {
 	if (dev->ndisc_ops && dev->ndisc_ops->update)
-		dev->ndisc_ops->update(dev, n, flags, icmp6_type, ndopts);
+		dev->ndisc_ops->update(dev, n, flags, failed_recovery,
+				       icmp6_type, ndopts);
 }

All the changes in this file can be dropped. See below.

This is tied to the lowpan changes below.

quoted

 
 static inline int ndisc_ops_opt_addr_space(const struct net_device *dev,

diff --git a/net/6lowpan/ndisc.c b/net/6lowpan/ndisc.c
index 868d28583c0a..8fedfef93740 100644
--- a/net/6lowpan/ndisc.c
+++ b/net/6lowpan/ndisc.c

@@ -47,7 +47,8 @@ static int lowpan_ndisc_parse_options(const struct net_device *dev,
 	}
 }
 
-static void lowpan_ndisc_802154_update(struct neighbour *n, u32 flags,
+static void lowpan_ndisc_802154_update(struct neighbour *n,
+				       bool failed_recovery,
 				       u8 icmp6_type,
 				       const struct ndisc_options *ndopts)
 {

@@ -87,20 +88,24 @@ static void lowpan_ndisc_802154_update(struct neighbour *n, u32 flags,
 		ieee802154_be16_to_le16(&neigh->short_addr, lladdr_short);
 		if (!lowpan_802154_is_valid_src_short_addr(neigh->short_addr))
 			neigh->short_addr = cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC);
+	} else if (failed_recovery) {
+		neigh->short_addr = cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC);
 	}
 	write_unlock_bh(&n->lock);
 }
 
 static void lowpan_ndisc_update(const struct net_device *dev,
-				struct neighbour *n, u32 flags, u8 icmp6_type,
+				struct neighbour *n, u32 flags,
+				bool failed_recovery, u8 icmp6_type,
 				const struct ndisc_options *ndopts)
 {
 	if (!lowpan_is_ll(dev, LOWPAN_LLTYPE_IEEE802154))
 		return;
 
-	/* react on overrides only. TODO check if this is really right. */
-	if (flags & NEIGH_UPDATE_F_OVERRIDE)
-		lowpan_ndisc_802154_update(n, flags, icmp6_type, ndopts);
+	/* React to overrides or accepted FAILED-entry recovery. */
+	if ((flags & NEIGH_UPDATE_F_OVERRIDE) || failed_recovery)
+		lowpan_ndisc_802154_update(n, failed_recovery, icmp6_type,
+					   ndopts);
 }
 
 static int lowpan_ndisc_opt_addr_space(const struct net_device *dev,

I'm not sure why you added these lowpan changes to the patch. They are
not described in the commit message. Given that lowpan_ndisc_update()
already has a TODO comment about only handling overrides, I suggest to
ignore it. If needed, it can be modified in the future by someone who
can explain the use case and test the change.

I added the lowpan changes after my local Sashiko review run identified 
an issue where a FAILED neighbor can retain an outdated private short 
address if it's moved to STALE by a non-override NA. Happy to drop all 
lowpan-related changes or update comments/commit message to reflect the 
changes, please let me know your preference.

quoted

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 90cd5d852569..84d70c09205a 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c

@@ -778,13 +778,23 @@ static int pndisc_is_router(const void *pkey,
 	return ret;
 }
 
+static void __ndisc_update(const struct net_device *dev,
+			   struct neighbour *neigh, const u8 *lladdr, u8 new,
+			   u32 flags, bool failed_recovery, u8 icmp6_type,
+			   struct ndisc_options *ndopts)
+{
+	neigh_update(neigh, lladdr, new, flags, 0);
+	/* report ndisc ops about neighbour update */
+	ndisc_ops_update(dev, neigh, flags, failed_recovery, icmp6_type,
+			 ndopts);
+}
+
 void ndisc_update(const struct net_device *dev, struct neighbour *neigh,
 		  const u8 *lladdr, u8 new, u32 flags, u8 icmp6_type,
 		  struct ndisc_options *ndopts)
 {
-	neigh_update(neigh, lladdr, new, flags, 0);
-	/* report ndisc ops about neighbour update */
-	ndisc_ops_update(dev, neigh, flags, icmp6_type, ndopts);
+	__ndisc_update(dev, neigh, lladdr, new, flags, false, icmp6_type,
+		       ndopts);
 }

This hunk can be dropped.

Tied to the lowpan changes above.

quoted

 
 static enum skb_drop_reason ndisc_recv_ns(struct sk_buff *skb)

@@ -972,14 +982,18 @@ static enum skb_drop_reason ndisc_recv_ns(struct sk_buff *skb)
 
 static int accept_untracked_na(struct inet6_dev *idev, struct in6_addr *saddr)
 {
+    /* For any given neighbor IP address, consider it an untracked neighbor if
+     * it is absent from the neighbor cache or if it has a NUD_FAILED entry in
+     * the neighbor cache
+     */

Redundant given the comments in the caller and the sysctl documentation.
Simply modify the existing comments below to mention FAILED case.

Will update.

quoted

 	switch (READ_ONCE(idev->cnf.accept_untracked_na)) {
-	case 0: /* Don't accept untracked na (absent in neighbor cache) */
+	case 0: /* Reject NAs for untracked neighbours */
 		return 0;
-	case 1: /* Create new entries from na if currently untracked */
+	case 1: /* Accept NAs for untracked neighbours */
 		return 1;
-	case 2: /* Create new entries from untracked na only if saddr is in the
+	case 2: /* Accept NAs for untracked neighbours only if saddr is in the
 		 * same subnet as an address configured on the interface that
-		 * received the na
+		 * received the NA
 		 */
 		return !!ipv6_chk_prefix(saddr, idev->dev);
 	default:

@@ -1001,6 +1015,9 @@ static enum skb_drop_reason ndisc_recv_na(struct sk_buff *skb)
 	struct neigh_table *tbl;
 	struct neighbour *neigh;
 	struct inet6_dev *idev;
+	bool neigh_failed = false;
+	bool neigh_untracked = false;
+	bool accept_untracked = false;

Try to maintain reverse xmas tree:

https://docs.kernel.org/next/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs

Will fix.

Was originally unsure if I should modify the existing logical structure.  
Thanks for the suggestion, will implement this. Is it appropriate to 
credit you with a commit tag?

quoted

 
 		if ((old_flags & ~neigh->flags) & NTF_ROUTER) {
 			/*
 			 * Change: router to host
 			 */
-			rt6_clean_tohost(dev_net(dev),  saddr);
+			rt6_clean_tohost(net,
+					 neigh_failed ? &msg->target : saddr);
 		}

What is the reason for this change? It's also not explained in the
commit message and I suspect it's not needed.

This was added in response to another Sashiko local review finding.  
Let's say we have some FAILED neighbor T with NTF_ROUTER set. If we get 
an NA from source address S with target address T and with the Router 
bit clear, existing kernel code will cleanup routes with gateway S, but 
IMO we should clean routes with gateway T instead since that is the 
neighbor which was updated by the NA. I can either update 
comments/commit message to reflect this or remove the change entirely, 
please let me know your preference.