[RFC net-next] ipv6: update NUD_FAILED neighbors from NA messages

[RFC net-next] ipv6: update NUD_FAILED neighbors from NA messages

[Lawrence Lee]

I noticed an inconsistency between IPv4 and IPv6 in how the kernel handles
neighbor advertisements/ARP replies for NUD_FAILED neighbor entries
and would like some guidance/input from maintainers.

For an existing IPv4 neighbor that is in the NUD_FAILED state, receiving an
ARP reply for the neighbor IP will update the entry in the kernel to
either NUD_STALE or NUD_REACHABLE depending on if the reply is unicast or
broadcast.

For an existing IPv6 neighbor that is in the NUD_FAILED state, receiving a
neighbor advertisement (NA) for the neighbor IP does nothing as
ndisc_recv_na() explicitly ignores NUD_FAILED neighbor entries:

        if (READ_ONCE(neigh->nud_state) & NUD_FAILED)
                goto out;

This check was added by commit titled "[IPV6] Don't update FAILED
entries on receipt of NAs." (Hideaki Yoshifuji, 2005-01-16; pre-git, in
mainline since v2.6.12-rc2) with the justification "As NAs do not create
new entries (RFC2461 7.2.5), NA should not change state of FAILED entries."

However, RFC9131 introduced a method for NAs to create new neighbor entries
(implemented as `accept_unsolicited_na` and later renamed to
`accept_untracked_na`), which means the original justification for ignoring
NAs for NUD_FAILED neighbor entries is no longer 100% correct. I think to
remain logically consistent, it makes sense to allow NAs to update
NUD_FAILED entries anytime we allow creating new entries with
`accept_untracked_na`.

I realize that RFC9131 section 4.2 states the following:

    ... routers create a new Neighbor Cache entry upon
    receiving an unsolicited Neighbor Advertisement for an address that
    does not already have a Neighbor Cache entry. These changes do not
    modify the router behavior specified in [RFC4861] for the scenario
    when the corresponding Neighbor Cache entry already exists.

However, I would argue that since NUD_FAILED is purely a kernel construct
and has no equivalent state defined in RFC4861 section 7.3.2, a neighbor in
state NUD_FAILED does not actually have a valid Neighbor Cache entry as
defined by RFC4861 and should be treated as if the neighbor entry doesn't
exist; therefore NUD_FAILED neighbors does fall within the scope of
RFC9131.

The motivation for this question comes from my work on SONiC, a network OS
which is built on top of Debian and runs on switching hardware. We have
encountered an issue where the switch receives traffic for an IPv6 neighbor
before that neighbor is resolvable, which leads to the kernel neighbor
being set to NUD_FAILED. When the IPv6 neighbor becomes ready to receive
traffic, it sends an unsolicited NA to the switch which gets ignored
because the kernel neighbor is NUD_FAILED. Subsequent traffic destined to
this neighbor stays entirely within the switch ASIC and isn't visible to
the kernel, so there's no stimulus for the kernel to send neighbor
solicitations; as a result, the neighbor entry stays unresolved and traffic
to the neighbor is dropped.

The main questions I'd like to pose:

1. When RFC9131 was implemented (`accept_untracked_na`), was an intentional
   choice made to not update the handling of NUD_FAILED neighbors? I
   searched through the discussions for all three commits relevant to this
   feature but did not find any mention of NUD_FAILED handling:
       commit f9a2fb73318e ("net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131")
       commit 3e0b8f529c10 ("net/ipv6: Expand and rename accept_unsolicited_na to accept_untracked_na")
       commit aaa5f515b16b ("net: ipv6: new accept_untracked_na option to accept na only if in-network")

2. Should unsolicited NAs be allowed to update NUD_FAILED neighbors when
   `accept_untracked_na` is enabled (this would align with existing
   IPv4/ARP behavior that allows ARP replies to update NUD_FAILED
   neighbors).

I've included a proof-of-concept diff below for how this might be
implemented (please note that this is just meant to help illustrate my
point, the diff is untested). I'd be happy to follow up with a more
thorough patch if the community decides that this is worth pursuing.

References:
  - commit titled "[IPV6] Don't update FAILED entries on receipt of NAs."
    (Hideaki Yoshifuji, 2005-01-16; pre-git, in mainline since v2.6.12-rc2)
  - net/ipv6/ndisc.c :: ndisc_recv_na(), accept_untracked_na()
  - RFC 4861 sec 7.2.5, 7.3.2 ; RFC 9131 sec 3, 4.2

Signed-off-by: Lawrence Lee <lfqlee314@gmail.com>
---
 net/ipv6/ndisc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index fe36b3f51285..2a6fa1252a04 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1086,8 +1086,17 @@ static enum skb_drop_reason ndisc_recv_na(struct sk_buff *skb)
 		u8 old_flags = neigh->flags;
 		struct net *net = dev_net(dev);
 
-		if (READ_ONCE(neigh->nud_state) & NUD_FAILED)
-			goto out;
+		if (READ_ONCE(neigh->nud_state) & NUD_FAILED) {
+			/* Update a FAILED entry when accept_untracked_na is
+			 * enabled, under the same conditions used to create a
+			 * new STALE entry (cf. IPv4 arp_process()).
+			 */
+			if (lladdr && idev && READ_ONCE(idev->cnf.forwarding) &&
+			    accept_untracked_na(idev, saddr))
+				new_state = NUD_STALE;
+			else
+				goto out;
+		}
 
 		/*
 		 * Don't update the neighbor cache entry on a proxy NA from
-- 
2.43.0