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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Claimed missing selftest coverage for the newly documented
  FAILED->STALE behaviour of accept_untracked_na: the in-tree tests…

--

ipv6: update NUD_FAILED neighbors from NA messages

This changes ndisc_recv_na() so that a neighbor entry in NUD_FAILED is
treated like an absent entry and moved to STALE when accept_untracked_na
is enabled and forwarding is on. The ip-sysctl documentation for
accept_untracked_na is updated to describe the new FAILED handling.

Link: https://lore.kernel.org/r/20260813233344.445265-1-lfqlee314@gmail.com

> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index f7af0286341c..685c84cf543d 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -3225,18 +3225,19 @@ drop_unsolicited_na - BOOLEAN
>  	Default: 0 (disabled).
>  
>  accept_untracked_na - INTEGER
> -	Define behavior for accepting neighbor advertisements from devices that
> -	are absent in the neighbor cache:
> +	Define behavior for accepting neighbor advertisements for IPv6 addresses
> +	that are absent from the neighbor cache or whose entries are in FAILED
> +	state:
>  
> -	- 0 - (default) Do not accept unsolicited and untracked neighbor
> -	  advertisements.
> +	- 0 - (default) Do not create new neighbor cache entries or update
> +	  FAILED entries from neighbor advertisements.
>  
> -	- 1 - Add a new neighbor cache entry in STALE state for routers on
> -	  receiving a neighbor advertisement (either solicited or unsolicited)
> -	  with target link-layer address option specified if no neighbor entry
> -	  is already present for the advertised IPv6 address. Without this knob,
> -	  NAs received for untracked addresses (absent in neighbor cache) are
> -	  silently ignored.
> +	- 1 - For routers, add a new neighbor cache entry or update an existing
> +	  FAILED entry to STALE upon receiving a neighbor advertisement (either
> +	  solicited or unsolicited) with the target link-layer address option
> +	  specified. Without this knob, NAs received for untracked addresses
> +	  (absent from the neighbor cache or in FAILED state) are silently
> +	  ignored.
>  
>  	  This is as per router-side behavior documented in RFC9131.
>  
> @@ -3251,9 +3252,10 @@ accept_untracked_na - INTEGER
>  	  used in conjunction with the ndisc_notify setting on the host to
>  	  satisfy this prerequisite.
>  
> -	- 2 - Extend option (1) to add a new neighbor cache entry only if the
> -	  source IP address is in the same subnet as an address configured on
> -	  the interface that received the neighbor advertisement.
> +	- 2 - Extend option (1) to add a new neighbor cache entry or update a
> +	  FAILED entry only if the source IP address is in the same subnet as
> +	  an address configured on the interface that received the neighbor
> +	  advertisement.

[ ... ]

> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 90cd5d852569..12d85d7f8234 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c

[ ... ]

> @@ -1067,34 +1067,39 @@ 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;
> +	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;
>  	}

[Severity: Low]
This isn't a bug, but a note on test coverage for the new FAILED branch in
ndisc_recv_na().

At the time this patch is applied on its own, the in-tree selftests only
walk the "absent entry" rows of the matrix: the matrix comment in
tools/testing/selftests/net/ndisc_unsolicited_na_test.sh and verify_ndisc()
start from an empty neighbor cache, and arp_ndisc_untracked_subnets.sh does
the same for the accept=2 subnet rule. That leaves the new

	if (!neigh || (READ_ONCE(neigh->nud_state) & NUD_FAILED)) {

path, and the FAILED rows now documented for accept_untracked_na, without a
test.

This is addressed later in the same series by "selftests: net: test
untracked NA recovery of FAILED neighbors", which extends the matrix
comment in ndisc_unsolicited_na_test.sh with the six "failed" rows, adds
initial_state/same_subnet parameters to verify_ndisc() and
test_unsolicited_na_common() that seed the entry with

	ip neigh replace ${HOST_ADDR} dev ${ROUTER_INTF} nud failed extern_learn

and then adds cases for accept=1, accept=0, forwarding=0, drop=1 and both
in-prefix and out-of-prefix accept=2, checking that the lladdr is learned
for the STALE rows and absent for the rows that must stay FAILED. So the
gap exists only for this commit in isolation, not for the final state of
the series.

Would it be worth mentioning the follow-up selftest patch in this commit
message so the coverage relationship is visible when this commit is read
alone?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789734885.git.lfqlee314%40gmail.com