Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 2 - [Medium] net/ipv6/ndisc.c:ndisc_recv_na() now passes two different gateway keys to rt6_clean_tohost() for the very same "router -> host"… - [Low] Reported for completeness only; assessed as resolved within this series. - [Low] The comment block added at net/ipv6/ndisc.c:985-988, at the top of accept_untracked_na(), is indented with four spaces instead of… -- ipv6: update NUD_FAILED neighbors from NA messages This teaches ndisc_recv_na() to treat a neighbour entry in NUD_FAILED the same way as a missing entry, so an NA with a target link-layer address option moves it to STALE on routers when accept_untracked_na is enabled. The ndisc_ops update callback gains a failed_recovery argument and the sysctl documentation is reworded accordingly. Link: https://lore.kernel.org/r/[email protected] A few questions below. > 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? > 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; [ ... ] > @@ -1067,32 +1084,41 @@ static enum skb_drop_reason ndisc_recv_na(struct > sk_buff *skb) [ ... ] > 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); [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. > > 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; > > /* > @@ -1108,19 +1134,23 @@ static enum skb_drop_reason ndisc_recv_na(struct > sk_buff *skb) [ ... ] > 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); > } [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. > reason = SKB_CONSUMED; > out: Thanks for looking at these. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789448374.git.lfqlee314%40gmail.com
