On Fri, May 27, 2022 at 3:00 PM Bagas Sanjaya <bagasdotme@xxxxxxxxx> wrote:
>
> 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.
>
Okay, I will fix this.
> > -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, ...
>
drop_unsolicited_na is a separate feature which is pre-existing in linux-5.18.
> > /* 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?
>
In the documentation, I skipped the table and mentioned the behavior in words.
1. drop_unsolicited_na takes precedence over accept_untracked_na.
2. The feature applies only for routers (fwding=1) and when target
link-layer address
option is specified.
Some of the behavior mentioned in the table is existing behavior and
didn't want to
document under this feature. I thought it best to skip the table in
Documentation as
it is more for code-readability.
> 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).
>
Sure, I tried to use UK-en since the code uses that, but I might have mixed
it up unintentionally. I'll make it uniform in the next version of the patch.
> --
> An old man doll... just what I always wanted! - Clara
I accidentally replied in HTML earlier. Fixing it to be plaintext.
