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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAOvjArSHVV81nhzK=4PVps47v5==kXOZ8OxTS+kxh93fwJYH1w@mail.gmail.com>
Date:   Fri, 27 May 2022 17:00:48 +0530
From:   Arun Ajith S <aajith@...sta.com>
To:     Bagas Sanjaya <bagasdotme@...il.com>
Cc:     netdev@...r.kernel.org, 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 Fri, May 27, 2022 at 3:00 PM Bagas Sanjaya <bagasdotme@...il.com> 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.