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

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

On Thu Sep 17, 2026 at 5:02 PM UTC,  wrote:

> > diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> > index 90cd5d8525698..84d70c09205a2 100644
> > --- a/net/ipv6/ndisc.c
> > +++ b/net/ipv6/ndisc.c
>
> [ ... ]
>
> > @@ -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
> > +     */
>         ^^^^
>
> [Severity: Low]
> This isn't a bug, but this comment block in accept_untracked_na() is
> indented with spaces while the switch statement right below it uses tabs,
> so checkpatch reports "ERROR: code indent should use tabs where possible".
>
> The comment also describes what the caller considers "untracked", based on
> the neighbour cache, but accept_untracked_na() only reads the sysctl value
> and never sees the neighbour entry. Would it fit better next to
>
> 	neigh_untracked = !neigh || neigh_failed;
>
> in ndisc_recv_na(), where the RFC 9131 decision table already documents the
> same concept?
This comment will be removed as per maintainer feedback.


> [Severity: Low]
> Is the new FAILED handling covered by an in-tree test at this point in the
> series? As of this commit, ndisc_unsolicited_na_test.sh and
> arp_ndisc_untracked_subnets.sh only seed absent entries, so the whole
> FAILED to STALE half of the knob, including the mode 2 same-subnet
> qualification, would still pass if it were broken.
>
> For the record, this looks resolved later in the same series: the follow-up
> patch "selftests: net: test untracked NA recovery of FAILED neighbors"
> (7be3b5fcfd3f9adb7489aff8013c6d4beacbf20c) seeds entries with
>
> 	ip neigh replace ... nud failed extern_learn
>
> and adds the failed combinations covering every row of the new table,
> including the in-prefix and out-of-prefix mode 2 cases. Mentioning it here
> only for completeness.
>
The tests are added in the next patch of this series

> [Severity: Medium]
> Why does the gateway address handed to rt6_clean_tohost() now depend on the
> previous NUD state of the entry? Before this patch the same "router to
> host" event always flushed routes keyed by saddr.
>
> fib6_clean_tohost() in net/ipv6/route.c matches the argument against the
> gateway of RA-installed routes and against cached exceptions:
>
> 	if (((rt->fib6_flags & RTF_RA_ROUTER) == RTF_RA_ROUTER) &&
> 	    nh->fib_nh_gw_family && ipv6_addr_equal(gateway, &nh->fib_nh_gw6))
> 		return -1;
>
> so it expects the address of the node that stopped being a router, which is
> unrelated to whether the cache entry happened to be in NUD_FAILED.
>
> When saddr differs from msg->target, and that is allowed since RFC 4861 4.4
> only requires the NA source to be an address of the sending interface (a
> router advertising a global target from its link-local source, or the proxy
> NA case handled a few lines above via pneigh_lookup()), the FAILED recovery
> path flushes routes keyed by msg->target while every other path flushes
> routes keyed by saddr. Can both be right for the identical event?
>
> If saddr was the wrong key all along, would it make sense to fix that
> separately for all cases, with a Fixes: tag, rather than changing it only
> for the FAILED path? The changelog describes only the FAILED to STALE
> transition and does not mention this change of key.
This change will be removed in the next version of the series as 
discussed in another thread: 
https://lore.kernel.org/all/DLHV5GF21AY4.3GAK241ZI2UR8@gmail.com/