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

["Lawrence Lee" <lfqlee314@gmail.com>]

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

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

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.

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

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

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

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

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

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

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?

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