arp: always override existing neigh entries with gratuitous ARP

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

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

---
 net/ipv4/arp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d54345a..053492a 100644
@@ -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

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

---
 net/ipv4/arp.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

index 053492a..ca6e1e6 100644
@@ -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

Hello,

	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

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

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

index ca6e1e6..c22103c 100644
@@ -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

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.

---
 net/ipv4/arp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index c22103c..ae96e6f 100644
@@ -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

From: Ihar Hrachyshka <[email protected]>
Date: Thu, 18 May 2017 12:41:17 -0700

Series applied, thanks.

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

Thanks.

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

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

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:

index ae96e6f3e0cb..39f26980a565 100644
@@ -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