netdev - Re: [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ef24dae-bad0-9641-7eb9-7d8207d198be@gmail.com>
Date:   Fri, 27 May 2022 16:30:11 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Arun Ajith S <aajith@...sta.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, dsahern@...nel.org,
        yoshfuji@...ux-ipv6.org, kuba@...nel.org, pabeni@...hat.com,
        gilligan@...sta.com, noureddine@...sta.com, gk@...sta.com
Subject: Re: [PATCH net] net/ipv6: Change accept_unsolicited_na to
 accept_untracked_na

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