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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Oct 2021 20:15:14 -0600
From:   David Ahern <dsahern@...il.com>
To:     James Prestwood <prestwoj@...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

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.


>  	__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.


your change seems all over the board here.

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.