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

[Ido Schimmel <idosch@nvidia.com>]

On Tue, Sep 15, 2026 at 05:01:31AM +0000, Lawrence Lee wrote:
> 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.

> 
> 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
> Assisted-by: LLM Sashiko sparse
> Signed-off-by: Lawrence Lee <lfqlee314@gmail.com>
> ---
>  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...

> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 208f46967ee5..4cc57a6be99b 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -3223,18 +3223,19 @@ drop_unsolicited_na - BOOLEAN
>  	Default: 0 (disabled).
>  
>  accept_untracked_na - INTEGER
> -	Define behavior for accepting neighbor advertisements from devices that
> -	are absent in the neighbor cache:
> +	Define behavior for accepting neighbor advertisements for IPv6 addresses
> +	that are absent from the neighbor cache or whose entries are in FAILED
> +	state:
>  
> -	- 0 - (default) Do not accept unsolicited and untracked neighbor
> -	  advertisements.
> +	- 0 - (default) Do not create new neighbor cache entries or update
> +	  FAILED entries from neighbor advertisements.
>  
> -	- 1 - Add a new neighbor cache entry in STALE state for routers on
> -	  receiving a neighbor advertisement (either solicited or unsolicited)
> -	  with target link-layer address option specified if no neighbor entry
> -	  is already present for the advertised IPv6 address. Without this knob,
> -	  NAs received for untracked addresses (absent in neighbor cache) are
> -	  silently ignored.
> +	- 1 - For routers, add a new neighbor cache entry or update an existing
> +	  FAILED entry to STALE upon receiving a neighbor advertisement (either
> +	  solicited or unsolicited) with the target link-layer address option
> +	  specified. Without this knob, NAs received for untracked addresses
> +	  (absent from the neighbor cache or in FAILED state) are silently
> +	  ignored.
>  
>  	  This is as per router-side behavior documented in RFC9131.
>  
> @@ -3249,9 +3250,10 @@ accept_untracked_na - INTEGER
>  	  used in conjunction with the ndisc_notify setting on the host to
>  	  satisfy this prerequisite.
>  
> -	- 2 - Extend option (1) to add a new neighbor cache entry only if the
> -	  source IP address is in the same subnet as an address configured on
> -	  the interface that received the neighbor advertisement.
> +	- 2 - Extend option (1) to add a new neighbor cache entry or update a
> +	  FAILED entry only if the source IP address is in the same subnet as
> +	  an address configured on the interface that received the neighbor
> +	  advertisement.
>  
>  enhanced_dad - BOOLEAN
>  	Include a nonce option in the IPv6 neighbor solicitation messages used for

The doc update looks OK.

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

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

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

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

>  	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

>  	u8 *lladdr = NULL;
>  	SKB_DR(reason);
>  	u8 new_state;
> @@ -1067,32 +1084,41 @@ static enum skb_drop_reason ndisc_recv_na(struct sk_buff *skb)
>  	neigh = neigh_lookup(tbl, &msg->target, dev);
>  
>  	/* RFC 9131 updates original Neighbour Discovery RFC 4861.
> -	 * NAs with Target LL Address option without a corresponding
> -	 * entry in the neighbour cache can now create a STALE neighbour
> -	 * cache entry on routers.
> +	 * NAs with Target LL Address option can now create a STALE neighbor
> +	 * cache entry on routers if the NA does not have a corresponding entry
> +	 * in the neighbour cache or has a corresponding FAILED entry.
>  	 *
> -	 *   entry accept  fwding  solicited        behaviour
> -	 * ------- ------  ------  ---------    ----------------------
> -	 * present      X       X         0     Set state to STALE
> -	 * present      X       X         1     Set state to REACHABLE
> -	 *  absent      0       X         X     Do nothing
> -	 *  absent      1       0         X     Do nothing
> -	 *  absent      1       1         X     Add a new STALE entry
> +	 *       entry accept  fwding  solicited        behaviour
> +	 * ----------- ------  ------  ---------    ----------------------
> +	 *  non-FAILED      X       X         0     Set state to STALE
> +	 *  non-FAILED      X       X         1     Set state to REACHABLE
> +	 *      FAILED      0       X         X     Do nothing
> +	 *      FAILED      1       0         X     Do nothing
> +	 *      FAILED      1       1         X     Set state to STALE
> +	 *      absent      0       X         X     Do nothing
> +	 *      absent      1       0         X     Do nothing
> +	 *      absent      1       1         X     Add a new STALE entry
>  	 *
>  	 * Note that we don't do a (daddr == all-routers-mcast) check.
>  	 */

The comment looks OK.

>  	new_state = msg->icmph.icmp6_solicited ? NUD_REACHABLE : NUD_STALE;
> -	if (!neigh && lladdr && idev && READ_ONCE(idev->cnf.forwarding)) {
> -		if (accept_untracked_na(idev, saddr)) {
> -			neigh = neigh_create(tbl, &msg->target, dev);
> -			new_state = NUD_STALE;
> -		}
> -	}
> +	neigh_failed = neigh &&
> +		       (READ_ONCE(neigh->nud_state) & NUD_FAILED);
> +	neigh_untracked = !neigh || neigh_failed;
> +	if (neigh_untracked) {
> +		accept_untracked = lladdr && idev &&
> +				   READ_ONCE(idev->cnf.forwarding) &&
> +				   accept_untracked_na(idev, saddr);
> +		new_state = NUD_STALE;
> +	}
> +	if (!neigh && accept_untracked)
> +		neigh = neigh_create(tbl, &msg->target, dev);
>  
>  	if (neigh && !IS_ERR(neigh)) {
> +		u32 update_flags;
>  		u8 old_flags = neigh->flags;
>  
> -		if (READ_ONCE(neigh->nud_state) & NUD_FAILED)
> +		if (neigh_untracked && !accept_untracked)
>  			goto out;
>  
>  		/*

This can be simplified to:

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 90cd5d852569..7d114bdb263e 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1082,19 +1082,21 @@ static enum skb_drop_reason ndisc_recv_na(struct sk_buff *skb)
 	 * Note that we don't do a (daddr == all-routers-mcast) check.
 	 */
 	new_state = msg->icmph.icmp6_solicited ? NUD_REACHABLE : NUD_STALE;
-	if (!neigh && lladdr && idev && READ_ONCE(idev->cnf.forwarding)) {
-		if (accept_untracked_na(idev, saddr)) {
-			neigh = neigh_create(tbl, &msg->target, dev);
-			new_state = NUD_STALE;
+	if (!neigh || (READ_ONCE(neigh->nud_state) & NUD_FAILED)) {
+		if (!lladdr || !idev || !READ_ONCE(idev->cnf.forwarding) ||
+		    !accept_untracked_na(idev, saddr)) {
+			if (neigh)
+				neigh_release(neigh);
+			return reason;
 		}
+		if (!neigh)
+			neigh = neigh_create(tbl, &msg->target, dev);
+		new_state = NUD_STALE;
 	}
 
 	if (neigh && !IS_ERR(neigh)) {
 		u8 old_flags = neigh->flags;
 
-		if (READ_ONCE(neigh->nud_state) & NUD_FAILED)
-			goto out;
-
 		/*
 		 * Don't update the neighbor cache entry on a proxy NA from
 		 * ourselves because either the proxied node is off link or it

> @@ -1108,19 +1134,23 @@ static enum skb_drop_reason ndisc_recv_na(struct sk_buff *skb)
>  			goto out;
>  		}
>  
> -		ndisc_update(dev, neigh, lladdr,
> -			     new_state,
> -			     NEIGH_UPDATE_F_WEAK_OVERRIDE|
> -			     (msg->icmph.icmp6_override ? NEIGH_UPDATE_F_OVERRIDE : 0)|
> -			     NEIGH_UPDATE_F_OVERRIDE_ISROUTER|
> -			     (msg->icmph.icmp6_router ? NEIGH_UPDATE_F_ISROUTER : 0),
> -			     NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);
> +		update_flags = NEIGH_UPDATE_F_WEAK_OVERRIDE |
> +			       (msg->icmph.icmp6_override ?
> +				NEIGH_UPDATE_F_OVERRIDE : 0) |
> +			       NEIGH_UPDATE_F_OVERRIDE_ISROUTER |
> +			       (msg->icmph.icmp6_router ?
> +				NEIGH_UPDATE_F_ISROUTER : 0);
> +
> +		__ndisc_update(dev, neigh, lladdr,
> +			       new_state, update_flags, neigh_failed,
> +			       NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);

Please drop this hunk. It's only needed because of the unnecessary
lowpan change.

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

>  		reason = SKB_CONSUMED;
>  out:
> -- 
> 2.43.0