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/[email protected] > > Assisted-by: LLM Sashiko sparse > > Signed-off-by: Lawrence Lee <[email protected]> > > --- > > 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.
