Re: [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na

[Bagas Sanjaya <bagasdotme@gmail.com>]

On 5/27/22 14:31, Arun Ajith S wrote:
> This change expands the current implementation to match the RFC. The
> sysctl knob is also renamed to accept_untracked_na to better reflect the
> implementation.
>

Say in imperative, "Expand and rename accept_unsolicited na to
accept_untracked_na" in both commit/patch subject and message.

> -accept_unsolicited_na - BOOLEAN
> +accept_untracked_na - BOOLEAN
>  	Add a new neighbour cache entry in STALE state for routers on receiving an
> -	unsolicited neighbour advertisement with target link-layer address option
> -	specified. This is as per router-side behavior documented in RFC9131.
> -	This has lower precedence than drop_unsolicited_na.
> +	neighbour advertisement with target link-layer address option specified
> +	if a corresponding entry is not already present.
> +	This is as per router-side behavior documented in RFC9131.
>  
> -	 ====   ======  ======  ==============================================
> -	 drop   accept  fwding                   behaviour
> -	 ----   ------  ------  ----------------------------------------------
> -	    1        X       X  Drop NA packet and don't pass up the stack
> -	    0        0       X  Pass NA packet up the stack, don't update NC
> -	    0        1       0  Pass NA packet up the stack, don't update NC
> -	    0        1       1  Pass NA packet up the stack, and add a STALE
> -	                        NC entry
> -	 ====   ======  ======  ==============================================
> +	This has lower precedence than drop_unsolicited_na.
>  

I think you should have made similar logical expansion of drop_unsolicited_na to
drop_untracked_na. Otherwise, ...

>  	/* RFC 9131 updates original Neighbour Discovery RFC 4861.
> -	 * An unsolicited NA can now create a neighbour cache entry
> -	 * on routers if it has Target LL Address option.
> +	 * NAs with Target LL Address option without a corresponding
> +	 * entry in the neighbour cache can now create a STALE neighbour
> +	 * cache entry on routers.
>  	 *
> -	 * drop   accept  fwding                   behaviour
> -	 * ----   ------  ------  ----------------------------------------------
> -	 *    1        X       X  Drop NA packet and don't pass up the stack
> -	 *    0        0       X  Pass NA packet up the stack, don't update NC
> -	 *    0        1       0  Pass NA packet up the stack, don't update NC
> -	 *    0        1       1  Pass NA packet up the stack, and add a STALE
> -	 *                          NC entry
> -	 * Note that we don't do a (daddr == all-routers-mcast) check.
> +	 *   entry accept  fwding  solicited        behaviour
> +	 * ------- ------  ------  ---------    ----------------------
> +	 * present      X       X         0     Set state to STALE
> +	 * present      X       X         1     Set state to REACHABLE
> +	 *  absent      0       X         X     Do nothing
> +	 *  absent      1       0         X     Do nothing
> +	 *  absent      1       1         X     Add a new STALE entry
>  	 */

The Documentation/ diff above drops behavior table but in the code comment
it is updated. Why didn't update in Documentation/ instead?

And my nitpick: for consistency, prefer en-US words over en-UK or mixed
varieties when writing (s/behaviour/behavior/gc, s/neighbour/neighbor/gc).

-- 
An old man doll... just what I always wanted! - Clara