netdev - Re: [PATCH v4] net: neighbour: introduce EVICT

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] [day] [month] [year] [list]
Date:   Wed, 20 Oct 2021 09:59:59 -0700
From:   James Prestwood <prestwoj@...il.com>
To:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Ido Schimmel <idosch@...dia.com>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        Yajun Deng <yajun.deng@...ux.dev>,
        Tong Zhu <zhutong@...zon.com>,
        Johannes Berg <johannes@...solutions.net>,
        Jouni Malinen <jouni@...eaurora.org>
Subject: Re: [PATCH v4] net: neighbour: introduce EVICT_NOCARRIER table
 option

Hi David,

On Tue, 2021-10-19 at 20:15 -0600, David Ahern wrote:
> On 10/18/21 1:26 PM, James Prestwood wrote:
> > diff --git a/Documentation/networking/ip-sysctl.rst
> > b/Documentation/networking/ip-sysctl.rst
> > index 16b8bf72feaf..e2aced01905a 100644
> > --- a/Documentation/networking/ip-sysctl.rst
> > +++ b/Documentation/networking/ip-sysctl.rst
> > @@ -200,6 +200,15 @@ neigh/default/unres_qlen - INTEGER
> >  
> >         Default: 101
> >  
> > +neigh/default/evict_nocarrier - BOOLEAN
> > +       Clears the neighbor cache on NOCARRIER events. This option
> > is important
> > +       for wireless devices where the cache should not be cleared
> > when roaming
> > +       between access points on the same network. In most cases
> > this should
> > +       remain as the default (1).
> > +
> > +       - 1 - (default): Clear the neighbor cache on NOCARRIER
> > events
> > +       - 0 - Do not clear neighbor cache on NOCARRIER events
> > +
> >  mtu_expires - INTEGER
> >         Time, in seconds, that cached PMTU information is kept.
> >  
> > diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> > index e8e48be66755..71b28f83c3d3 100644
> > --- a/include/net/neighbour.h
> > +++ b/include/net/neighbour.h
> > @@ -54,7 +54,8 @@ enum {
> >         NEIGH_VAR_ANYCAST_DELAY,
> >         NEIGH_VAR_PROXY_DELAY,
> >         NEIGH_VAR_LOCKTIME,
> > -#define NEIGH_VAR_DATA_MAX (NEIGH_VAR_LOCKTIME + 1)
> > +       NEIGH_VAR_EVICT_NOCARRIER,
> > +#define NEIGH_VAR_DATA_MAX (NEIGH_VAR_EVICT_NOCARRIER + 1)
> >         /* Following are used as a second way to access one of the
> > above */
> >         NEIGH_VAR_QUEUE_LEN, /* same data as
> > NEIGH_VAR_QUEUE_LEN_BYTES */
> >         NEIGH_VAR_RETRANS_TIME_MS, /* same data as
> > NEIGH_VAR_RETRANS_TIME */
> > diff --git a/include/uapi/linux/neighbour.h
> > b/include/uapi/linux/neighbour.h
> > index db05fb55055e..1dc125dd4f50 100644
> > --- a/include/uapi/linux/neighbour.h
> > +++ b/include/uapi/linux/neighbour.h
> > @@ -152,6 +152,7 @@ enum {
> >         NDTPA_QUEUE_LENBYTES,           /* u32 */
> >         NDTPA_MCAST_REPROBES,           /* u32 */
> >         NDTPA_PAD,
> > +       NDTPA_EVICT_NOCARRIER,          /* u8 */
> 
> you are proposing a sysctl not a neighbor table attribute. ie., you
> don't need this.

Can't there be both, as there are with several of the other attributes?
I very well could have done it wrong, but my intent was to make this
settable via sysctl and RTNL.

But going the per netdev route as you describe below, yes this won't be
needed.

> 
> 
> >         __NDTPA_MAX
> >  };
> >  #define NDTPA_MAX (__NDTPA_MAX - 1)
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 47931c8be04b..953253f3e491 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -318,7 +318,7 @@ static void pneigh_queue_purge(struct
> > sk_buff_head *list)
> >  }
> >  
> >  static void neigh_flush_dev(struct neigh_table *tbl, struct
> > net_device *dev,
> > -                           bool skip_perm)
> > +                           bool nocarrier)
> 
> why are you dropping the skip_perm arg? These are orthogonal skip
> options.

I felt it was more appropriate to describe the reason/event (no
carrier) that triggered the flush after adding this option. Rather than
using something called 'skip_perm' to skip non-perminant entries.

> 
> 
> your change seems all over the board here.

If it wasn't obvious this subsystem is totally new to me :)

> 
> You should be adding a per netdevice sysctl to allow this setting to
> be
> controlled per device, not a global setting or a table setting.
> 
> In neigh_carrier_down, check the sysctl for this device (and check
> 'all'
> device too) and if eviction on carrier down is not wanted, then skip
> the
> call to __neigh_ifdown.

Sure, this seems like a good path forward. I was not a fan of needing
to go into the neighbor iteration loop since we want to skip everything
for a given netdev anyways.

Thanks for the feedback

- James