[PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[Ihar Hrachyshka]

This patchset is spurred by discussion started at
https://patchwork.ozlabs.org/patch/760372/ where we figured that there is no
real reason for enforcing override by gratuitous ARP packets only when
arp_accept is 1. Same should happen when it's 0 (the default value).

changelog v2: handled review comments by Julian Anastasov
- fixed a mistake in a comment;
- postponed addr_type calculation to as late as possible.

Ihar Hrachyshka (4):
  arp: fixed error in a comment
  arp: decompose is_garp logic into a separate function
  arp: postpone addr_type calculation to as late as possible
  arp: always override existing neigh entries with gratuitous ARP

 net/ipv4/arp.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 17 deletions(-)

-- 
2.9.3

[PATCH v2 1/4] arp: fixed error in a comment

[Ihar Hrachyshka]

the is_garp code deals just with gratuitous ARP packets, not every
unsolicited packet.

This patch is a result of a discussion in netdev:
http://marc.info/?l=linux-netdev&m=149506354216994

Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
 net/ipv4/arp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index d54345a..053492a 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -846,7 +846,7 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 		 */
 		is_garp = tip == sip && addr_type == RTN_UNICAST;
 
-		/* Unsolicited ARP _replies_ also require target hwaddr to be
+		/* Gratuitous ARP _replies_ also require target hwaddr to be
 		 * the same as source.
 		 */
 		if (is_garp && arp->ar_op == htons(ARPOP_REPLY))
-- 
2.9.3

[PATCH v2 2/4] arp: decompose is_garp logic into a separate function

[Ihar Hrachyshka]

The code is quite involving already to earn a separate function for
itself. If anything, it helps arp_process readability.

Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
 net/ipv4/arp.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 053492a..ca6e1e6 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -641,6 +641,27 @@ void arp_xmit(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(arp_xmit);
 
+static bool arp_is_garp(struct net_device *dev, int addr_type,
+			__be16 ar_op,
+			__be32 sip, __be32 tip,
+			unsigned char *sha, unsigned char *tha)
+{
+	bool is_garp = tip == sip && addr_type == RTN_UNICAST;
+
+	/* Gratuitous ARP _replies_ also require target hwaddr to be
+	 * the same as source.
+	 */
+	if (is_garp && ar_op == htons(ARPOP_REPLY))
+		is_garp =
+			/* IPv4 over IEEE 1394 doesn't provide target
+			 * hardware address field in its ARP payload.
+			 */
+			tha &&
+			!memcmp(tha, sha, dev->addr_len);
+
+	return is_garp;
+}
+
 /*
  *	Process an arp request.
  */
@@ -844,18 +865,8 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 		   It is possible, that this option should be enabled for some
 		   devices (strip is candidate)
 		 */
-		is_garp = tip == sip && addr_type == RTN_UNICAST;
-
-		/* Gratuitous ARP _replies_ also require target hwaddr to be
-		 * the same as source.
-		 */
-		if (is_garp && arp->ar_op == htons(ARPOP_REPLY))
-			is_garp =
-				/* IPv4 over IEEE 1394 doesn't provide target
-				 * hardware address field in its ARP payload.
-				 */
-				tha &&
-				!memcmp(tha, sha, dev->addr_len);
+		is_garp = arp_is_garp(dev, addr_type, arp->ar_op,
+				      sip, tip, sha, tha);
 
 		if (!n &&
 		    ((arp->ar_op == htons(ARPOP_REPLY)  &&
-- 
2.9.3

[PATCH v2 3/4] arp: postpone addr_type calculation to as late as possible

[Ihar Hrachyshka]

The addr_type retrieval can be costly, so it's worth trying to avoid its
calculation as much as possible. This patch makes it calculated only
for gratuitous ARP packets. This is especially important since later we
may want to move is_garp calculation outside of arp_accept block, at
which point the costly operation will be executed for all setups.

The patch is the result of a discussion in net-dev:
http://marc.info/?l=linux-netdev&m=149506354216994

Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
 net/ipv4/arp.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ca6e1e6..c22103c 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -641,12 +641,12 @@ void arp_xmit(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(arp_xmit);
 
-static bool arp_is_garp(struct net_device *dev, int addr_type,
-			__be16 ar_op,
+static bool arp_is_garp(struct net *net, struct net_device *dev,
+			int *addr_type, __be16 ar_op,
 			__be32 sip, __be32 tip,
 			unsigned char *sha, unsigned char *tha)
 {
-	bool is_garp = tip == sip && addr_type == RTN_UNICAST;
+	bool is_garp = tip == sip;
 
 	/* Gratuitous ARP _replies_ also require target hwaddr to be
 	 * the same as source.
@@ -659,6 +659,11 @@ static bool arp_is_garp(struct net_device *dev, int addr_type,
 			tha &&
 			!memcmp(tha, sha, dev->addr_len);
 
+	if (is_garp) {
+		*addr_type = inet_addr_type_dev_table(net, dev, sip);
+		if (*addr_type != RTN_UNICAST)
+			is_garp = false;
+	}
 	return is_garp;
 }
 
@@ -859,18 +864,23 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 	n = __neigh_lookup(&arp_tbl, &sip, dev, 0);
 
 	if (IN_DEV_ARP_ACCEPT(in_dev)) {
-		unsigned int addr_type = inet_addr_type_dev_table(net, dev, sip);
+		addr_type = -1;
 
 		/* Unsolicited ARP is not accepted by default.
 		   It is possible, that this option should be enabled for some
 		   devices (strip is candidate)
 		 */
-		is_garp = arp_is_garp(dev, addr_type, arp->ar_op,
+		is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
 				      sip, tip, sha, tha);
 
 		if (!n &&
-		    ((arp->ar_op == htons(ARPOP_REPLY)  &&
-				addr_type == RTN_UNICAST) || is_garp))
+		    (is_garp ||
+		     (arp->ar_op == htons(ARPOP_REPLY) &&
+		      (addr_type == RTN_UNICAST ||
+		       (addr_type < 0 &&
+			/* postpone calculation to as late as possible */
+			inet_addr_type_dev_table(net, dev, sip) ==
+				RTN_UNICAST)))))
 			n = __neigh_lookup(&arp_tbl, &sip, dev, 1);
 	}
 
-- 
2.9.3

[PATCH v2 4/4] arp: always override existing neigh entries with gratuitous ARP

[Ihar Hrachyshka]

Currently, when arp_accept is 1, we always override existing neigh
entries with incoming gratuitous ARP replies. Otherwise, we override
them only if new replies satisfy _locktime_ conditional (packets arrive
not earlier than _locktime_ seconds since the last update to the neigh
entry).

The idea behind locktime is to pick the very first (=> close) reply
received in a unicast burst when ARP proxies are used. This helps to
avoid ARP thrashing where Linux would switch back and forth from one
proxy to another.

This logic has nothing to do with gratuitous ARP replies that are
generally not aligned in time when multiple IP address carriers send
them into network.

This patch enforces overriding of existing neigh entries by all incoming
gratuitous ARP packets, irrespective of their time of arrival. This will
make the kernel honour all incoming gratuitous ARP packets.

Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
---
 net/ipv4/arp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index c22103c..ae96e6f 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -863,16 +863,17 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	n = __neigh_lookup(&arp_tbl, &sip, dev, 0);
 
-	if (IN_DEV_ARP_ACCEPT(in_dev)) {
+	if (n || IN_DEV_ARP_ACCEPT(in_dev)) {
 		addr_type = -1;
+		is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
+				      sip, tip, sha, tha);
+	}
 
+	if (IN_DEV_ARP_ACCEPT(in_dev)) {
 		/* Unsolicited ARP is not accepted by default.
 		   It is possible, that this option should be enabled for some
 		   devices (strip is candidate)
 		 */
-		is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
-				      sip, tip, sha, tha);
-
 		if (!n &&
 		    (is_garp ||
 		     (arp->ar_op == htons(ARPOP_REPLY) &&
-- 
2.9.3

Re: [PATCH v2 2/4] arp: decompose is_garp logic into a separate function

[Julian Anastasov]

	Hello,

On Thu, 18 May 2017, Ihar Hrachyshka wrote:

> The code is quite involving already to earn a separate function for
> itself. If anything, it helps arp_process readability.
> 
> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
> ---
>  net/ipv4/arp.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 053492a..ca6e1e6 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -641,6 +641,27 @@ void arp_xmit(struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(arp_xmit);
>  
> +static bool arp_is_garp(struct net_device *dev, int addr_type,
> +			__be16 ar_op,
> +			__be32 sip, __be32 tip,
> +			unsigned char *sha, unsigned char *tha)
> +{
> +	bool is_garp = tip == sip && addr_type == RTN_UNICAST;
> +
> +	/* Gratuitous ARP _replies_ also require target hwaddr to be
> +	 * the same as source.
> +	 */
> +	if (is_garp && ar_op == htons(ARPOP_REPLY))
> +		is_garp =
> +			/* IPv4 over IEEE 1394 doesn't provide target
> +			 * hardware address field in its ARP payload.
> +			 */
> +			tha &&

	All 4 patches look ok to me with only a small problem
which comes from patch already included in kernel. I see
that GARP replies can not work for 1394, is_garp will be
cleared. May be 'tha' check should be moved in if expression,
for example:

	if (is_garp && ar_op == htons(ARPOP_REPLY) && tha)
		is_garp = !memcmp(tha, sha, dev->addr_len);

> +			!memcmp(tha, sha, dev->addr_len);
> +
> +	return is_garp;
> +}

Regards

Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[David Miller]

From: Ihar Hrachyshka <ihrachys@redhat.com>
Date: Thu, 18 May 2017 12:41:17 -0700

> This patchset is spurred by discussion started at
> https://patchwork.ozlabs.org/patch/760372/ where we figured that there is no
> real reason for enforcing override by gratuitous ARP packets only when
> arp_accept is 1. Same should happen when it's 0 (the default value).
> 
> changelog v2: handled review comments by Julian Anastasov
> - fixed a mistake in a comment;
> - postponed addr_type calculation to as late as possible.

Series applied, thanks.

Please address the feedback from Julian about the ieee1394 change
you put into ARP earlier.

Thanks.

Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[Arnd Bergmann]

On Thu, May 18, 2017 at 9:41 PM, Ihar Hrachyshka <ihrachys@redhat.com> wrote:
> This patchset is spurred by discussion started at
> https://patchwork.ozlabs.org/patch/760372/ where we figured that there is no
> real reason for enforcing override by gratuitous ARP packets only when
> arp_accept is 1. Same should happen when it's 0 (the default value).
>
> changelog v2: handled review comments by Julian Anastasov
> - fixed a mistake in a comment;
> - postponed addr_type calculation to as late as possible.

This seems to have caused a build warning:

net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
in this function [-Wmaybe-uninitialized]

       Arnd

Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[Ihar Hrachyshka]

On 05/23/2017 01:56 PM, Arnd Bergmann wrote:
> This seems to have caused a build warning:
>
> net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>
Not sure. How do you reproduce it? I just did 'make net' in the latest 
tree that includes the patch, and it doesn't trigger the failure. Also 
the code logic prevents it to be uninitialized (it's always set to -1 or 
the actual type value).

Please advise,
Ihar

Re: [PATCH v2 2/4] arp: decompose is_garp logic into a separate function

[Ihar Hrachyshka]

On 05/18/2017 01:49 PM, Julian Anastasov wrote:

> 	All 4 patches look ok to me with only a small problem
> which comes from patch already included in kernel. I see
> that GARP replies can not work for 1394, is_garp will be
> cleared. May be 'tha' check should be moved in if expression,
> for example:
>
> 	if (is_garp && ar_op == htons(ARPOP_REPLY) && tha)
> 		is_garp = !memcmp(tha, sha, dev->addr_len);

I can easily miss something substantial, so please correct me, but...

If it's of REPLY type, the RFC 2002 requires that target hardware 
address field equals to source address field for a packet to be 
considered gratuitous. Since IEEE 1394 ARP standard defines its payload 
without target field, it seems to me that there is no such thing as a 
gratuitous ARP reply for IEEE 1394. That's why I think resetting is_garp 
to 0 for those packets is justified.

Ihar

Re: [PATCH v2 0/4] arp: always override existing neigh entries with gratuitous ARP

[Arnd Bergmann]

On Wed, May 24, 2017 at 11:32 PM, Ihar Hrachyshka <ihrachys@redhat.com> wrote:
> On 05/23/2017 01:56 PM, Arnd Bergmann wrote:
>>
>> This seems to have caused a build warning:
>>
>> net/ipv4/arp.c:880:35: warning: 'addr_type' may be used uninitialized
>> in this function [-Wmaybe-uninitialized]
>>
> Not sure. How do you reproduce it? I just did 'make net' in the latest tree
> that includes the patch, and it doesn't trigger the failure. Also the code
> logic prevents it to be uninitialized (it's always set to -1 or the actual
> type value).

I saw it reported by the kernelci build bot:

https://kernelci.org/build/id/5925b81859b514fb106b9590/logs/

It happened on arm64 with 'make allmodconfig', but none of the other
architectures or the other configurations on that architecture.

This kind of warning can be a real pain to shut up when the compiler
gets it wrong. I haven't tested the latest kernel with your patch on my
own build box yet, so I also haven't been able to reproduce it.
Most likely, gcc gets confused when it needs to track the state of
too many variables here (I certainly get confused when I read it too).

My first attempt to resolve it would be:

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ae96e6f3e0cb..39f26980a565 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -663,6 +663,8 @@ static bool arp_is_garp(struct net *net, struct
net_device *dev,
                *addr_type = inet_addr_type_dev_table(net, dev, sip);
                if (*addr_type != RTN_UNICAST)
                        is_garp = false;
+       } else {
+               *addr_type = -1;
        }
        return is_garp;
 }
@@ -864,7 +866,6 @@ static int arp_process(struct net *net, struct
sock *sk, struct sk_buff *skb)
        n = __neigh_lookup(&arp_tbl, &sip, dev, 0);

        if (n || IN_DEV_ARP_ACCEPT(in_dev)) {
-               addr_type = -1;
                is_garp = arp_is_garp(net, dev, &addr_type, arp->ar_op,
                                      sip, tip, sha, tha);
        }

but this clearly needs a lot of build testing to see if it's sufficient. Moving
the initialization of addr_type=1 to the start of the function would
certainly avoid the warning, but I suspect this would be incorrect here.

        Arnd