[PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[James Prestwood]

This change introduces a new sysctl parameter, arp_evict_nocarrier.
When set (default) the ARP cache will be cleared on a NOCARRIER event.
This new option has been defaulted to '1' which maintains existing
behavior.

Clearing the ARP cache on NOCARRIER is relatively new, introduced by:

commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
Author: David Ahern <dsahern@gmail.com>
Date:   Thu Oct 11 20:33:49 2018 -0700

    net: Evict neighbor entries on carrier down

The reason for this changes is to prevent the ARP cache from being
cleared when a wireless device roams. Specifically for wireless roams
the ARP cache should not be cleared because the underlying network has not
changed. Clearing the ARP cache in this case can introduce significant
delays sending out packets after a roam.

A user reported such a situation here:

https://lore.kernel.org/linux-wireless/CACsRnHWa47zpx3D1oDq9JYnZWniS8yBwW1h0WAVZ6vrbwL_S0w@mail.gmail.com/

After some investigation it was found that the kernel was holding onto
packets until ARP finished which resulted in this 1 second delay. It
was also found that the first ARP who-has was never responded to,
which is actually what caues the delay. This change is more or less
working around this behavior, but again, there is no reason to clear
the cache on a roam anyways.

As for the unanswered who-has, we know the packet made it OTA since
it was seen while monitoring. Why it never received a response is
unknown.

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 include/linux/inetdevice.h  | 1 +
 include/uapi/linux/ip.h     | 1 +
 include/uapi/linux/sysctl.h | 1 +
 net/ipv4/arp.c              | 4 +++-
 net/ipv4/devinet.c          | 4 ++++
 5 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 53aa0343bf69..63180170fdbd 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 #define IN_DEV_ARP_ANNOUNCE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_ANNOUNCE)
 #define IN_DEV_ARP_IGNORE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_IGNORE)
 #define IN_DEV_ARP_NOTIFY(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_NOTIFY)
+#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev) IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)
 
 struct in_ifaddr {
 	struct hlist_node	hash;
diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
index e42d13b55cf3..e00bbb9c47bb 100644
--- a/include/uapi/linux/ip.h
+++ b/include/uapi/linux/ip.h
@@ -169,6 +169,7 @@ enum
 	IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST,
 	IPV4_DEVCONF_DROP_GRATUITOUS_ARP,
 	IPV4_DEVCONF_BC_FORWARDING,
+	IPV4_DEVCONF_ARP_EVICT_NOCARRIER,
 	__IPV4_DEVCONF_MAX
 };
 
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 1e05d3caa712..6a3b194c50fe 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -482,6 +482,7 @@ enum
 	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
 	NET_IPV4_CONF_ARP_ACCEPT=21,
 	NET_IPV4_CONF_ARP_NOTIFY=22,
+	NET_IPV4_CONF_ARP_EVICT_NOCARRIER=23,
 };
 
 /* /proc/sys/net/ipv4/netfilter */
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 922dd73e5740..50cfe4f37089 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -1247,6 +1247,7 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct netdev_notifier_change_info *change_info;
+	struct in_device *in_dev = __in_dev_get_rcu(dev);
 
 	switch (event) {
 	case NETDEV_CHANGEADDR:
@@ -1257,7 +1258,8 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
 		change_info = ptr;
 		if (change_info->flags_changed & IFF_NOARP)
 			neigh_changeaddr(&arp_tbl, dev);
-		if (!netif_carrier_ok(dev))
+		if (IN_DEV_ARP_EVICT_NOCARRIER(in_dev) &&
+		    !netif_carrier_ok(dev))
 			neigh_carrier_down(&arp_tbl, dev);
 		break;
 	default:
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 1c6429c353a9..4ff4403749e0 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -75,6 +75,7 @@ static struct ipv4_devconf ipv4_devconf = {
 		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
 		[IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL - 1] = 10000 /*ms*/,
 		[IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL - 1] =  1000 /*ms*/,
+		[IPV4_DEVCONF_ARP_EVICT_NOCARRIER - 1] = 1,
 	},
 };
 
@@ -87,6 +88,7 @@ static struct ipv4_devconf ipv4_devconf_dflt = {
 		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE - 1] = 1,
 		[IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL - 1] = 10000 /*ms*/,
 		[IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL - 1] =  1000 /*ms*/,
+		[IPV4_DEVCONF_ARP_EVICT_NOCARRIER - 1] = 1,
 	},
 };
 
@@ -2527,6 +2529,8 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_RW_ENTRY(ARP_IGNORE, "arp_ignore"),
 		DEVINET_SYSCTL_RW_ENTRY(ARP_ACCEPT, "arp_accept"),
 		DEVINET_SYSCTL_RW_ENTRY(ARP_NOTIFY, "arp_notify"),
+		DEVINET_SYSCTL_RW_ENTRY(ARP_EVICT_NOCARRIER,
+					"arp_evict_nocarrier"),
 		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP_PVLAN, "proxy_arp_pvlan"),
 		DEVINET_SYSCTL_RW_ENTRY(FORCE_IGMP_VERSION,
 					"force_igmp_version"),
-- 
2.31.1

[PATCH 2/4] net: ndisc: introduce ndisc_evict_nocarrier sysctl parameter

[James Prestwood]

In most situations the neighbor discovery cache should be cleared on a
NOCARRIER event which is currently done unconditionally. But for wireless
roams the neighbor discovery cache can and should remain intact since
the underlying network has not changed.

This patch introduces a sysctl option ndisc_evict_nocarrier which can
be disabled by a wireless supplicant during a roam. This allows packets
to be sent after a roam immediately without having to wait for
neighbor discovery.

A user reported roughly a 1 second delay after a roam before packets
could be sent out (note, on IPv4). This delay was due to the ARP
cache being cleared. During testing of this same scenario using IPv6
no delay was noticed, but regardless there is no reason to clear
the ndisc cache for wireless roams.

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 include/linux/ipv6.h      |  1 +
 include/uapi/linux/ipv6.h |  1 +
 net/ipv6/addrconf.c       | 10 ++++++++++
 net/ipv6/ndisc.c          |  5 ++++-
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 70b2ad3b9884..72297d759a23 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -76,6 +76,7 @@ struct ipv6_devconf {
 	__s32		disable_policy;
 	__s32           ndisc_tclass;
 	__s32		rpl_seg_enabled;
+	__u32		ndisc_evict_nocarrier;
 
 	struct ctl_table_header *sysctl_header;
 };
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index 70603775fe91..999b290d79c3 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -190,6 +190,7 @@ enum {
 	DEVCONF_NDISC_TCLASS,
 	DEVCONF_RPL_SEG_ENABLED,
 	DEVCONF_RA_DEFRTR_METRIC,
+	DEVCONF_NDISC_EVICT_NOCARRIER,
 	DEVCONF_MAX
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 701eb82acd1c..b538054f8d0d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -237,6 +237,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.addr_gen_mode		= IN6_ADDR_GEN_MODE_EUI64,
 	.disable_policy		= 0,
 	.rpl_seg_enabled	= 0,
+	.ndisc_evict_nocarrier	= 1,
 };
 
 static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
@@ -293,6 +294,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.addr_gen_mode		= IN6_ADDR_GEN_MODE_EUI64,
 	.disable_policy		= 0,
 	.rpl_seg_enabled	= 0,
+	.ndisc_evict_nocarrier	= 1,
 };
 
 /* Check if link is ready: is it up and is a valid qdisc available */
@@ -5526,6 +5528,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_DISABLE_POLICY] = cnf->disable_policy;
 	array[DEVCONF_NDISC_TCLASS] = cnf->ndisc_tclass;
 	array[DEVCONF_RPL_SEG_ENABLED] = cnf->rpl_seg_enabled;
+	array[DEVCONF_NDISC_EVICT_NOCARRIER] = cnf->ndisc_evict_nocarrier;
 }
 
 static inline size_t inet6_ifla6_size(void)
@@ -6932,6 +6935,13 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "ndisc_evict_nocarrier",
+		.data		= &ipv6_devconf.ndisc_evict_nocarrier,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
 	{
 		/* sentinel */
 	}
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index c467c6419893..d80346a2a789 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1805,10 +1805,13 @@ static int ndisc_netdev_event(struct notifier_block *this, unsigned long event,
 		in6_dev_put(idev);
 		break;
 	case NETDEV_CHANGE:
+		idev = in6_dev_get(dev);
+		if (!idev)
+			break;
 		change_info = ptr;
 		if (change_info->flags_changed & IFF_NOARP)
 			neigh_changeaddr(&nd_tbl, dev);
-		if (!netif_carrier_ok(dev))
+		if (!netif_carrier_ok(dev) && idev->cnf.ndisc_evict_nocarrier)
 			neigh_carrier_down(&nd_tbl, dev);
 		break;
 	case NETDEV_DOWN:
-- 
2.31.1

[PATCH 3/4] doc: networking: document arp_evict_nocarrier

[James Prestwood]

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index c2ecc9894fd0..174d9d3ee5c2 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1565,6 +1565,15 @@ arp_accept - BOOLEAN
 	gratuitous arp frame, the arp table will be updated regardless
 	if this setting is on or off.
 
+arp_evict_nocarrier - BOOLEAN
+	Clears the ARP cache on NOCARRIER events. This option is important for
+	wireless devices where the ARP 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 ARP cache on NOCARRIER events
+	- 0 - Do not clear ARP cache on NOCARRIER events
+
 mcast_solicit - INTEGER
 	The maximum number of multicast probes in INCOMPLETE state,
 	when the associated hardware address is unknown.  Defaults
-- 
2.31.1

Re: [PATCH 3/4] doc: networking: document arp_evict_nocarrier

[David Ahern]

On 10/13/21 4:27 PM, James Prestwood wrote:
> Signed-off-by: James Prestwood <prestwoj@gmail.com>
> ---
>  Documentation/networking/ip-sysctl.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index c2ecc9894fd0..174d9d3ee5c2 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -1565,6 +1565,15 @@ arp_accept - BOOLEAN
>  	gratuitous arp frame, the arp table will be updated regardless
>  	if this setting is on or off.
>  
> +arp_evict_nocarrier - BOOLEAN
> +	Clears the ARP cache on NOCARRIER events. This option is important for
> +	wireless devices where the ARP 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 ARP cache on NOCARRIER events
> +	- 0 - Do not clear ARP cache on NOCARRIER events
> +
>  mcast_solicit - INTEGER
>  	The maximum number of multicast probes in INCOMPLETE state,
>  	when the associated hardware address is unknown.  Defaults
> 

documentation can be added to the patch that adds the new sysctl

[PATCH 4/4] doc: networking: document ndisc_evict_nocarrier

[James Prestwood]

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 174d9d3ee5c2..7bac67a2f6d7 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2258,6 +2258,15 @@ ndisc_tclass - INTEGER
 
 	* 0 - (default)
 
+ndisc_evict_nocarrier - BOOLEAN
+	Clears the neighbor discovery table on NOCARRIER events. This option is
+	important for wireless devices where the neighbor discovery 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 neighbor discover cache on NOCARRIER events.
+	- 0 - Do not clear neighbor discovery cache on NOCARRIER events.
+
 mldv1_unsolicited_report_interval - INTEGER
 	The interval in milliseconds in which the next unsolicited
 	MLDv1 report retransmit will take place.
-- 
2.31.1

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[Jakub Kicinski]

On Wed, 13 Oct 2021 15:27:07 -0700 James Prestwood wrote:
> This change introduces a new sysctl parameter, arp_evict_nocarrier.
> When set (default) the ARP cache will be cleared on a NOCARRIER event.
> This new option has been defaulted to '1' which maintains existing
> behavior.
> 
> Clearing the ARP cache on NOCARRIER is relatively new, introduced by:
> 
> commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> Author: David Ahern <dsahern@gmail.com>
> Date:   Thu Oct 11 20:33:49 2018 -0700
> 
>     net: Evict neighbor entries on carrier down
> 
> The reason for this changes is to prevent the ARP cache from being
> cleared when a wireless device roams. Specifically for wireless roams
> the ARP cache should not be cleared because the underlying network has not
> changed. Clearing the ARP cache in this case can introduce significant
> delays sending out packets after a roam.
> 
> A user reported such a situation here:
> 
> https://lore.kernel.org/linux-wireless/CACsRnHWa47zpx3D1oDq9JYnZWniS8yBwW1h0WAVZ6vrbwL_S0w@mail.gmail.com/
> 
> After some investigation it was found that the kernel was holding onto
> packets until ARP finished which resulted in this 1 second delay. It
> was also found that the first ARP who-has was never responded to,
> which is actually what caues the delay. This change is more or less
> working around this behavior, but again, there is no reason to clear
> the cache on a roam anyways.
> 
> As for the unanswered who-has, we know the packet made it OTA since
> it was seen while monitoring. Why it never received a response is
> unknown.
> 
> Signed-off-by: James Prestwood <prestwoj@gmail.com>

Seems sensible at a glance, some quick feedback.

Please make sure you run ./scripts/get_maintainers.pl on the patch
and add appropriate folks to CC.

Please rebase the code on top of this tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/

> diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
> index 53aa0343bf69..63180170fdbd 100644
> --- a/include/linux/inetdevice.h
> +++ b/include/linux/inetdevice.h
> @@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
>  #define IN_DEV_ARP_ANNOUNCE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_ANNOUNCE)
>  #define IN_DEV_ARP_IGNORE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_IGNORE)
>  #define IN_DEV_ARP_NOTIFY(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_NOTIFY)
> +#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev) IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)

IN_DEV_ANDCONF() makes most sense, I'd think.

>  struct in_ifaddr {
>  	struct hlist_node	hash;

> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 922dd73e5740..50cfe4f37089 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1247,6 +1247,7 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
>  {
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	struct netdev_notifier_change_info *change_info;
> +	struct in_device *in_dev = __in_dev_get_rcu(dev);

Don't we need to hold the RCU lock to call this?

>  	switch (event) {
>  	case NETDEV_CHANGEADDR:
> @@ -1257,7 +1258,8 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
>  		change_info = ptr;
>  		if (change_info->flags_changed & IFF_NOARP)
>  			neigh_changeaddr(&arp_tbl, dev);
> -		if (!netif_carrier_ok(dev))
> +		if (IN_DEV_ARP_EVICT_NOCARRIER(in_dev) &&
> +		    !netif_carrier_ok(dev))
>  			neigh_carrier_down(&arp_tbl, dev);
>  		break;
>  	default:

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[Jakub Kicinski]

On Wed, 13 Oct 2021 16:37:14 -0700 Jakub Kicinski wrote:
> Please make sure you run ./scripts/get_maintainers.pl on the patch
> and add appropriate folks to CC.

Ah, and beyond that please make sure to CC wireless folks.
The linux-wireless mailing list, Johannes, Jouni, etc.
The question of the "real root cause" is slightly under-explained.
Sounds like something restores carrier before the device is actually up.

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[James Prestwood]

On Wed, 2021-10-13 at 16:40 -0700, Jakub Kicinski wrote:
> On Wed, 13 Oct 2021 16:37:14 -0700 Jakub Kicinski wrote:
> > Please make sure you run ./scripts/get_maintainers.pl on the patch
> > and add appropriate folks to CC.
> 
> Ah, and beyond that please make sure to CC wireless folks.
> The linux-wireless mailing list, Johannes, Jouni, etc.
> The question of the "real root cause" is slightly under-explained.
> Sounds like something restores carrier before the device is actually
> up.

Sure. As for the real root cause, are you talking about the AP side? As
far as the station is concerened the initial packets are making it out
after a roam. So possibly the AP is restoring carrier before it should,
and packets are getting dropped.

In v2 I'll include a cover letter with more details about the test
setup, and what was observed.

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[Jakub Kicinski]

On Thu, 14 Oct 2021 09:32:31 -0700 James Prestwood wrote:
> On Wed, 2021-10-13 at 16:40 -0700, Jakub Kicinski wrote:
> > On Wed, 13 Oct 2021 16:37:14 -0700 Jakub Kicinski wrote:  
> > > Please make sure you run ./scripts/get_maintainers.pl on the patch
> > > and add appropriate folks to CC.  
> > 
> > Ah, and beyond that please make sure to CC wireless folks.
> > The linux-wireless mailing list, Johannes, Jouni, etc.
> > The question of the "real root cause" is slightly under-explained.
> > Sounds like something restores carrier before the device is actually
> > up.  
> 
> Sure. As for the real root cause, are you talking about the AP side? As
> far as the station is concerened the initial packets are making it out
> after a roam. So possibly the AP is restoring carrier before it should,
> and packets are getting dropped.

Yeah, that's what I meant. If you're saying that station sends the
packet out correctly then indeed nothing we can do but the workaround.

> In v2 I'll include a cover letter with more details about the test
> setup, and what was observed.

Thanks!

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[James Prestwood]

Hi Jakub,

On Wed, 2021-10-13 at 16:37 -0700, Jakub Kicinski wrote:
> On Wed, 13 Oct 2021 15:27:07 -0700 James Prestwood wrote:
> > This change introduces a new sysctl parameter, arp_evict_nocarrier.
> > When set (default) the ARP cache will be cleared on a NOCARRIER
> > event.
> > This new option has been defaulted to '1' which maintains existing
> > behavior.
> > 
> > Clearing the ARP cache on NOCARRIER is relatively new, introduced
> > by:
> > 
> > commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> > Author: David Ahern <dsahern@gmail.com>
> > Date:   Thu Oct 11 20:33:49 2018 -0700
> > 
> >     net: Evict neighbor entries on carrier down
> > 
> > The reason for this changes is to prevent the ARP cache from being
> > cleared when a wireless device roams. Specifically for wireless
> > roams
> > the ARP cache should not be cleared because the underlying network
> > has not
> > changed. Clearing the ARP cache in this case can introduce
> > significant
> > delays sending out packets after a roam.
> > 
> > A user reported such a situation here:
> > 
> > https://lore.kernel.org/linux-wireless/CACsRnHWa47zpx3D1oDq9JYnZWniS8yBwW1h0WAVZ6vrbwL_S0w@mail.gmail.com/
> > 
> > After some investigation it was found that the kernel was holding
> > onto
> > packets until ARP finished which resulted in this 1 second delay.
> > It
> > was also found that the first ARP who-has was never responded to,
> > which is actually what caues the delay. This change is more or less
> > working around this behavior, but again, there is no reason to
> > clear
> > the cache on a roam anyways.
> > 
> > As for the unanswered who-has, we know the packet made it OTA since
> > it was seen while monitoring. Why it never received a response is
> > unknown.
> > 
> > Signed-off-by: James Prestwood <prestwoj@gmail.com>
> 
> Seems sensible at a glance, some quick feedback.
> 
> Please make sure you run ./scripts/get_maintainers.pl on the patch
> and add appropriate folks to CC.
> 
> Please rebase the code on top of this tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
> 
> > diff --git a/include/linux/inetdevice.h
> > b/include/linux/inetdevice.h
> > index 53aa0343bf69..63180170fdbd 100644
> > --- a/include/linux/inetdevice.h
> > +++ b/include/linux/inetdevice.h
> > @@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct
> > in_device *in_dev)
> >  #define IN_DEV_ARP_ANNOUNCE(in_dev)    IN_DEV_MAXCONF((in_dev),
> > ARP_ANNOUNCE)
> >  #define IN_DEV_ARP_IGNORE(in_dev)      IN_DEV_MAXCONF((in_dev),
> > ARP_IGNORE)
> >  #define IN_DEV_ARP_NOTIFY(in_dev)      IN_DEV_MAXCONF((in_dev),
> > ARP_NOTIFY)
> > +#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev)
> > IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)
> 
> IN_DEV_ANDCONF() makes most sense, I'd think.

So given we want '1' as the default as well as the ability to toggle
this option per-netdev I thought this was more appropriate. One caviat
is this would not work for setting ALL netdev's, but I don't think this
is a valid use case; or at least I can't imagine why you'd want to ever
do this.

This is a whole new area to me though, so if I'm understanding these
macros wrong please educate me :)

> 
> >  struct in_ifaddr {
> >         struct hlist_node       hash;
> 
> > diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> > index 922dd73e5740..50cfe4f37089 100644
> > --- a/net/ipv4/arp.c
> > +++ b/net/ipv4/arp.c
> > @@ -1247,6 +1247,7 @@ static int arp_netdev_event(struct
> > notifier_block *this, unsigned long event,
> >  {
> >         struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> >         struct netdev_notifier_change_info *change_info;
> > +       struct in_device *in_dev = __in_dev_get_rcu(dev);
> 
> Don't we need to hold the RCU lock to call this?

I was wondering that as well. It seems to be used in some places and
not others. Maybe the caller locked prior in places where there was no
lock, I'll look further into it.

> 
> >         switch (event) {
> >         case NETDEV_CHANGEADDR:
> > @@ -1257,7 +1258,8 @@ static int arp_netdev_event(struct
> > notifier_block *this, unsigned long event,
> >                 change_info = ptr;
> >                 if (change_info->flags_changed & IFF_NOARP)
> >                         neigh_changeaddr(&arp_tbl, dev);
> > -               if (!netif_carrier_ok(dev))
> > +               if (IN_DEV_ARP_EVICT_NOCARRIER(in_dev) &&
> > +                   !netif_carrier_ok(dev))
> >                         neigh_carrier_down(&arp_tbl, dev);
> >                 break;
> >         default:

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[Jakub Kicinski]

On Thu, 14 Oct 2021 09:29:05 -0700 James Prestwood wrote:
> > > diff --git a/include/linux/inetdevice.h
> > > b/include/linux/inetdevice.h
> > > index 53aa0343bf69..63180170fdbd 100644
> > > --- a/include/linux/inetdevice.h
> > > +++ b/include/linux/inetdevice.h
> > > @@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct
> > > in_device *in_dev)
> > >  #define IN_DEV_ARP_ANNOUNCE(in_dev)    IN_DEV_MAXCONF((in_dev),
> > > ARP_ANNOUNCE)
> > >  #define IN_DEV_ARP_IGNORE(in_dev)      IN_DEV_MAXCONF((in_dev),
> > > ARP_IGNORE)
> > >  #define IN_DEV_ARP_NOTIFY(in_dev)      IN_DEV_MAXCONF((in_dev),
> > > ARP_NOTIFY)
> > > +#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev)
> > > IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)  
> > 
> > IN_DEV_ANDCONF() makes most sense, I'd think.  
> 
> So given we want '1' as the default as well as the ability to toggle
> this option per-netdev I thought this was more appropriate. One caviat
> is this would not work for setting ALL netdev's, but I don't think this
> is a valid use case; or at least I can't imagine why you'd want to ever
> do this.
> 
> This is a whole new area to me though, so if I'm understanding these
> macros wrong please educate me :)

Yeah, TBH I'm not sure what the best practice is here. I know we
weren't very consistent in the past. Having a knob for "all" which 
is 100% ignored seems like the worst option. Let me CC Dave Ahern,
please make sure you CC him on v2 as well.

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[David Ahern]

On 10/14/21 10:59 AM, Jakub Kicinski wrote:
> On Thu, 14 Oct 2021 09:29:05 -0700 James Prestwood wrote:
>>>> diff --git a/include/linux/inetdevice.h
>>>> b/include/linux/inetdevice.h
>>>> index 53aa0343bf69..63180170fdbd 100644
>>>> --- a/include/linux/inetdevice.h
>>>> +++ b/include/linux/inetdevice.h
>>>> @@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct
>>>> in_device *in_dev)
>>>>  #define IN_DEV_ARP_ANNOUNCE(in_dev)    IN_DEV_MAXCONF((in_dev),
>>>> ARP_ANNOUNCE)
>>>>  #define IN_DEV_ARP_IGNORE(in_dev)      IN_DEV_MAXCONF((in_dev),
>>>> ARP_IGNORE)
>>>>  #define IN_DEV_ARP_NOTIFY(in_dev)      IN_DEV_MAXCONF((in_dev),
>>>> ARP_NOTIFY)
>>>> +#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev)
>>>> IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)  
>>>
>>> IN_DEV_ANDCONF() makes most sense, I'd think.  
>>
>> So given we want '1' as the default as well as the ability to toggle
>> this option per-netdev I thought this was more appropriate. One caviat
>> is this would not work for setting ALL netdev's, but I don't think this
>> is a valid use case; or at least I can't imagine why you'd want to ever
>> do this.
>>
>> This is a whole new area to me though, so if I'm understanding these
>> macros wrong please educate me :)
> 
> Yeah, TBH I'm not sure what the best practice is here. I know we
> weren't very consistent in the past. Having a knob for "all" which 
> is 100% ignored seems like the worst option. Let me CC Dave Ahern,
> please make sure you CC him on v2 as well.
> 

agreed. It's a matter of consistency in the use of the 'all' variant. I
would say support it even if it you do not believe it will be changed.

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[Daniel Borkmann]

[ Adding few more to Cc ]

On 10/14/21 12:27 AM, James Prestwood wrote:
> This change introduces a new sysctl parameter, arp_evict_nocarrier.
> When set (default) the ARP cache will be cleared on a NOCARRIER event.
> This new option has been defaulted to '1' which maintains existing
> behavior.
> 
> Clearing the ARP cache on NOCARRIER is relatively new, introduced by:
> 
> commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> Author: David Ahern <dsahern@gmail.com>
> Date:   Thu Oct 11 20:33:49 2018 -0700
> 
>      net: Evict neighbor entries on carrier down
> 
> The reason for this changes is to prevent the ARP cache from being
> cleared when a wireless device roams. Specifically for wireless roams
> the ARP cache should not be cleared because the underlying network has not
> changed. Clearing the ARP cache in this case can introduce significant
> delays sending out packets after a roam.
> 
> A user reported such a situation here:
> 
> https://lore.kernel.org/linux-wireless/CACsRnHWa47zpx3D1oDq9JYnZWniS8yBwW1h0WAVZ6vrbwL_S0w@mail.gmail.com/
> 
> After some investigation it was found that the kernel was holding onto
> packets until ARP finished which resulted in this 1 second delay. It
> was also found that the first ARP who-has was never responded to,
> which is actually what caues the delay. This change is more or less
> working around this behavior, but again, there is no reason to clear
> the cache on a roam anyways.
> 
> As for the unanswered who-has, we know the packet made it OTA since
> it was seen while monitoring. Why it never received a response is
> unknown.

Wouldn't it make more sense to extend neigh_flush_dev() where we skip eviction
of NUD_PERMANENT (see the skip_perm condition)? Either as a per table setting
(tbl->parms) or as a NTF_EXT_* flag for specific neighbors?

> Signed-off-by: James Prestwood <prestwoj@gmail.com>
> ---
>   include/linux/inetdevice.h  | 1 +
>   include/uapi/linux/ip.h     | 1 +
>   include/uapi/linux/sysctl.h | 1 +
>   net/ipv4/arp.c              | 4 +++-
>   net/ipv4/devinet.c          | 4 ++++
>   5 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
> index 53aa0343bf69..63180170fdbd 100644
> --- a/include/linux/inetdevice.h
> +++ b/include/linux/inetdevice.h
> @@ -133,6 +133,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
>   #define IN_DEV_ARP_ANNOUNCE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_ANNOUNCE)
>   #define IN_DEV_ARP_IGNORE(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_IGNORE)
>   #define IN_DEV_ARP_NOTIFY(in_dev)	IN_DEV_MAXCONF((in_dev), ARP_NOTIFY)
> +#define IN_DEV_ARP_EVICT_NOCARRIER(in_dev) IN_DEV_CONF_GET((in_dev), ARP_EVICT_NOCARRIER)
>   
>   struct in_ifaddr {
>   	struct hlist_node	hash;
> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> index e42d13b55cf3..e00bbb9c47bb 100644
> --- a/include/uapi/linux/ip.h
> +++ b/include/uapi/linux/ip.h
> @@ -169,6 +169,7 @@ enum
>   	IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST,
>   	IPV4_DEVCONF_DROP_GRATUITOUS_ARP,
>   	IPV4_DEVCONF_BC_FORWARDING,
> +	IPV4_DEVCONF_ARP_EVICT_NOCARRIER,
>   	__IPV4_DEVCONF_MAX
>   };
>   
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 1e05d3caa712..6a3b194c50fe 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -482,6 +482,7 @@ enum
>   	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
>   	NET_IPV4_CONF_ARP_ACCEPT=21,
>   	NET_IPV4_CONF_ARP_NOTIFY=22,
> +	NET_IPV4_CONF_ARP_EVICT_NOCARRIER=23,
>   };
>   
>   /* /proc/sys/net/ipv4/netfilter */
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 922dd73e5740..50cfe4f37089 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -1247,6 +1247,7 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
>   {
>   	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>   	struct netdev_notifier_change_info *change_info;
> +	struct in_device *in_dev = __in_dev_get_rcu(dev);
>   
>   	switch (event) {
>   	case NETDEV_CHANGEADDR:
> @@ -1257,7 +1258,8 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event,
>   		change_info = ptr;
>   		if (change_info->flags_changed & IFF_NOARP)
>   			neigh_changeaddr(&arp_tbl, dev);
> -		if (!netif_carrier_ok(dev))
> +		if (IN_DEV_ARP_EVICT_NOCARRIER(in_dev) &&
> +		    !netif_carrier_ok(dev))
>   			neigh_carrier_down(&arp_tbl, dev);
>   		break;
>   	default:
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 1c6429c353a9..4ff4403749e0 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -75,6 +75,7 @@ static struct ipv4_devconf ipv4_devconf = {
>   		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
>   		[IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL - 1] = 10000 /*ms*/,
>   		[IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL - 1] =  1000 /*ms*/,
> +		[IPV4_DEVCONF_ARP_EVICT_NOCARRIER - 1] = 1,
>   	},
>   };
>   
> @@ -87,6 +88,7 @@ static struct ipv4_devconf ipv4_devconf_dflt = {
>   		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE - 1] = 1,
>   		[IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL - 1] = 10000 /*ms*/,
>   		[IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL - 1] =  1000 /*ms*/,
> +		[IPV4_DEVCONF_ARP_EVICT_NOCARRIER - 1] = 1,
>   	},
>   };
>   
> @@ -2527,6 +2529,8 @@ static struct devinet_sysctl_table {
>   		DEVINET_SYSCTL_RW_ENTRY(ARP_IGNORE, "arp_ignore"),
>   		DEVINET_SYSCTL_RW_ENTRY(ARP_ACCEPT, "arp_accept"),
>   		DEVINET_SYSCTL_RW_ENTRY(ARP_NOTIFY, "arp_notify"),
> +		DEVINET_SYSCTL_RW_ENTRY(ARP_EVICT_NOCARRIER,
> +					"arp_evict_nocarrier"),
>   		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP_PVLAN, "proxy_arp_pvlan"),
>   		DEVINET_SYSCTL_RW_ENTRY(FORCE_IGMP_VERSION,
>   					"force_igmp_version"),
> 

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[James Prestwood]

Hi Daniel,

On Thu, 2021-10-14 at 10:30 +0200, Daniel Borkmann wrote:
> [ Adding few more to Cc ]
> 
> On 10/14/21 12:27 AM, James Prestwood wrote:
> > This change introduces a new sysctl parameter, arp_evict_nocarrier.
> > When set (default) the ARP cache will be cleared on a NOCARRIER
> > event.
> > This new option has been defaulted to '1' which maintains existing
> > behavior.
> > 
> > Clearing the ARP cache on NOCARRIER is relatively new, introduced
> > by:
> > 
> > commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> > Author: David Ahern <dsahern@gmail.com>
> > Date:   Thu Oct 11 20:33:49 2018 -0700
> > 
> >      net: Evict neighbor entries on carrier down
> > 
> > The reason for this changes is to prevent the ARP cache from being
> > cleared when a wireless device roams. Specifically for wireless
> > roams
> > the ARP cache should not be cleared because the underlying network
> > has not
> > changed. Clearing the ARP cache in this case can introduce
> > significant
> > delays sending out packets after a roam.
> > 
> > A user reported such a situation here:
> > 
> > https://lore.kernel.org/linux-wireless/CACsRnHWa47zpx3D1oDq9JYnZWniS8yBwW1h0WAVZ6vrbwL_S0w@mail.gmail.com/
> > 
> > After some investigation it was found that the kernel was holding
> > onto
> > packets until ARP finished which resulted in this 1 second delay.
> > It
> > was also found that the first ARP who-has was never responded to,
> > which is actually what caues the delay. This change is more or less
> > working around this behavior, but again, there is no reason to
> > clear
> > the cache on a roam anyways.
> > 
> > As for the unanswered who-has, we know the packet made it OTA since
> > it was seen while monitoring. Why it never received a response is
> > unknown.
> 
> Wouldn't it make more sense to extend neigh_flush_dev() where we skip
> eviction
> of NUD_PERMANENT (see the skip_perm condition)? Either as a per table
> setting
> (tbl->parms) or as a NTF_EXT_* flag for specific neighbors?
> 

This is all new code to me, but from what I can tell NUD_PERMANENT is a
per-neighbor thing, which a wireless supplicant shouldn't be expected
to manage. It also seems like a pain to mark all neighbors as permanent
prior to a roam, then undo it all after a roam.

As for table settings I'll need to look into those, and if/how you
expose them to userspace? I modeled both these (arp/ndisc) options
after the other arp_*/ndisc_* sysctl parameters, as they seemed to fit
in there.

Thanks,
James

> 

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[David Ahern]

On 10/13/21 4:27 PM, James Prestwood wrote:
> This change introduces a new sysctl parameter, arp_evict_nocarrier.
> When set (default) the ARP cache will be cleared on a NOCARRIER event.
> This new option has been defaulted to '1' which maintains existing
> behavior.
> 
> Clearing the ARP cache on NOCARRIER is relatively new, introduced by:
> 
> commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> Author: David Ahern <dsahern@gmail.com>
> Date:   Thu Oct 11 20:33:49 2018 -0700
> 
>     net: Evict neighbor entries on carrier down
> 
> The reason for this changes is to prevent the ARP cache from being
> cleared when a wireless device roams. Specifically for wireless roams
> the ARP cache should not be cleared because the underlying network has not
> changed. Clearing the ARP cache in this case can introduce significant
> delays sending out packets after a roam.

how do you know if the existing ARP / ND entries are valid when roaming?

Re: [PATCH 1/4] net: arp: introduce arp_evict_nocarrier sysctl parameter

[James Prestwood]

Hi David,

On Thu, 2021-10-14 at 12:34 -0600, David Ahern wrote:
> On 10/13/21 4:27 PM, James Prestwood wrote:
> > This change introduces a new sysctl parameter, arp_evict_nocarrier.
> > When set (default) the ARP cache will be cleared on a NOCARRIER
> > event.
> > This new option has been defaulted to '1' which maintains existing
> > behavior.
> > 
> > Clearing the ARP cache on NOCARRIER is relatively new, introduced
> > by:
> > 
> > commit 859bd2ef1fc1110a8031b967ee656c53a6260a76
> > Author: David Ahern <dsahern@gmail.com>
> > Date:   Thu Oct 11 20:33:49 2018 -0700
> > 
> >     net: Evict neighbor entries on carrier down
> > 
> > The reason for this changes is to prevent the ARP cache from being
> > cleared when a wireless device roams. Specifically for wireless
> > roams
> > the ARP cache should not be cleared because the underlying network
> > has not
> > changed. Clearing the ARP cache in this case can introduce
> > significant
> > delays sending out packets after a roam.
> 
> how do you know if the existing ARP / ND entries are valid when
> roaming?
> 

I guess there is no way of really knowing, but since your roaming in
the same network I think we can assume the entries are just as valid
prior to the roam as after it right? If there are invalid entries, they
were likely invalid prior to the roam too.

I obviously cant speak to all network configurations, but I think if
your network infrastructure is set up to roam you into a different
subnet you're doing something wrong. Or I can't think of a reason to do
this, it would defeat the purpose of quickly roaming between BSS's
since you would then have to do DHCP aftewards.

Thanks,
James