Re: [PATCH] neighbour: confirm neigh entries when ARP packet is received

[Ihar Hrachyshka]

On Sat, Sep 1, 2018 at 4:51 PM, David Miller  wrote:
> From: Vasily Khoruzhick 
> Date: Tue, 28 Aug 2018 19:48:25 -0700
>
>> Update 'confirmed' timestamp when ARP packet is received. It shouldn't
>> affect locktime logic and anyway entry can be confirmed by any higher-layer
>> protocol. Thus it makes no sense not to confirm it when ARP packet is
>> received.
>>
>> Fixes: 77d7123342 ("neighbour: update neigh timestamps iff update is
>> effective")
>>
>> Signed-off-by: Vasily Khoruzhick 
>
> I'm not so sure.
>
> The comment above the code you are moving explains that the current
> behavior is intention, and it explains why too.
>
> Even if your change is correct, you're now making that comment
> inaccuratte, so you'd have to update it to match the new code.
>
> But I still think the current code is intentionally behaving that
> way, and for good reason.

Hi David,

(I am the one who put this comment there.)

I agree with the reasoning that Vasily provided for the change (we
should confirm the entry if e.g. ARP packet with identical
hwaddr/ipaddr pair arrives; just not mark it as updated). It was a
mistake of mine to put access to both updated and confirmed fields
under the "if" branch. Just leaving 'updated' there and moving
'confirmed' outside seems like the right thing to do.

The original intent was to not update 'updated' field when no update
happens (because of consequent ARP packets sent in short span of
time). The fix by Vasily should not negatively affect this scenario.

Of course, I also agree that the comment will need some adjustment to
reflect the fact that now a single timestamp is being updated. Perhaps
while at it, Vasily could also explicitly describe in a comment which
scenario the "if" branch check is supposed to cover. (I should have
done it myself, mea culpa.)

I hope it helps,
Ihar

[PATCH] arp: fixed -Wuninitialized compiler warning

[Ihar Hrachyshka]

Commit 7d472a59c0e5ec117220a05de6b370447fb6cb66 ("arp: always override
existing neigh entries with gratuitous ARP") introduced a compiler
warning:

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

While the code logic seems to be correct and doesn't allow the variable
to be used uninitialized, and the warning is not consistently
reproducible, it's still worth fixing it for other people not to waste
time looking at the warning in case it pops up in the build environment.
Yes, compiler is probably at fault, but we will need to accommodate.

Fixes: 7d472a59c0e5ec117220a05de6b370447fb6cb66
Signed-off-by: Ihar Hrachyshka <ihrac...@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 ae96e6f..e9f3386 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -863,8 +863,8 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
 
n = __neigh_lookup(_tbl, , dev, 0);
 
+   addr_type = -1;
if (n || IN_DEV_ARP_ACCEPT(in_dev)) {
-   addr_type = -1;
is_garp = arp_is_garp(net, dev, _type, arp->ar_op,
  sip, tip, sha, tha);
}
-- 
2.9.4

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

[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

[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 <ihrac...@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(_tbl, , 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, _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, _type, arp->ar_op,
- sip, tip, sha, tha);
-
if (!n &&
(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 <ihrac...@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=149506354216994

Suggested-by: Julian Anastasov <j...@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrac...@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(_tbl, , 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, _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(_tbl, , dev, 1);
}
 
-- 
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=149506354216994

Suggested-by: Julian Anastasov <j...@ssi.bg>
Signed-off-by: Ihar Hrachyshka <ihrac...@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 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 1/2] 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 <ihrac...@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 d54345a..3f06201 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;
+
+   /* Unsolicited 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;
-
-   /* Unsolicited 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 2/2] 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 <ihrac...@redhat.com>
---
 net/ipv4/arp.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

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

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

[Ihar Hrachyshka]

Good day.

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

The first patch in the series moves is_garp code into a separate function
preparing the code base for the 2nd patch that actually implements the needed
change.

Ihar Hrachyshka (2):
  arp: decompose is_garp logic into a separate function
  arp: always override existing neigh entries with gratuitous ARP

 net/ipv4/arp.c | 47 +--
 1 file changed, 29 insertions(+), 18 deletions(-)

-- 
2.9.3

Re: [PATCH] neighbour: update neigh timestamps iff update is effective

[Ihar Hrachyshka]

On Mon, May 15, 2017 at 2:40 PM, Ihar Hrachyshka <ihrac...@redhat.com> wrote:
> It's a common practice to send gratuitous ARPs after moving an
> IP address to another device to speed up healing of a service. To
> fulfill service availability constraints, the timing of network peers
> updating their caches to point to a new location of an IP address can be
> particularly important.
>
> Sometimes neigh_update calls won't touch neither lladdr nor state, for
> example if an update arrives in locktime interval. The neigh->updated
> value is tested by the protocol specific neigh code, which in turn
> will influence whether NEIGH_UPDATE_F_OVERRIDE gets set in the
> call to neigh_update() or not. As a result, we may effectively ignore
> the update request, bailing out of touching the neigh entry, except that
> we still bump its timestamps inside neigh_update.
>
>
> This may be a problem for updates arriving in quick succession. For
> example, consider the following scenario:
>
> A service is moved to another device with its IP address. The new device
> sends three gratuitous ARP requests into the network with ~1 seconds
> interval between them. Just before the first request arrives to one of
> network peer nodes, its neigh entry for the IP address transitions from
> STALE to DELAY.  This transition, among other things, updates
> neigh->updated. Once the kernel receives the first gratuitous ARP, it
> ignores it because its arrival time is inside the locktime interval. The
> kernel still bumps neigh->updated. Then the second gratuitous ARP
> request arrives, and it's also ignored because it's still in the (new)
> locktime interval. Same happens for the third request. The node
> eventually heals itself (after delay_first_probe_time seconds since the
> initial transition to DELAY state), but it just wasted some time and
> require a new ARP request/reply round trip. This unfortunate behaviour
> both puts more load on the network, as well as reduces service
> availability.
>
> This patch changes neigh_update so that it bumps neigh->updated (as well
> as neigh->confirmed) only once we are sure that either lladdr or entry
> state will change). In the scenario described above, it means that the
> second gratuitous ARP request will actually update the entry lladdr.
>
> Ideally, we would update the neigh entry on the very first gratuitous
> ARP request. The locktime mechanism is designed to ignore ARP updates in
> a short timeframe after a previous ARP update was honoured by the kernel
> layer. This would require tracking timestamps for state transitions
> separately from timestamps when actual updates are received. This would
> probably involve changes in neighbour struct. Therefore, the patch
> doesn't tackle the issue of the first gratuitous APR ignored, leaving
> it for a follow-up.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>

Please disregard this email, I forgot to update the patch version to
v2 and provide changelog. I posted (hopefully) correct version. Still
learning the process...

Ihar

Re: [PATCH v2] neighbour: update neigh timestamps iff update is effective

[Ihar Hrachyshka]

It's a common practice to send gratuitous ARPs after moving an
IP address to another device to speed up healing of a service. To
fulfill service availability constraints, the timing of network peers
updating their caches to point to a new location of an IP address can be
particularly important.

Sometimes neigh_update calls won't touch neither lladdr nor state, for
example if an update arrives in locktime interval. The neigh->updated
value is tested by the protocol specific neigh code, which in turn
will influence whether NEIGH_UPDATE_F_OVERRIDE gets set in the
call to neigh_update() or not. As a result, we may effectively ignore
the update request, bailing out of touching the neigh entry, except that
we still bump its timestamps inside neigh_update.

This may be a problem for updates arriving in quick succession. For
example, consider the following scenario:

A service is moved to another device with its IP address. The new device
sends three gratuitous ARP requests into the network with ~1 seconds
interval between them. Just before the first request arrives to one of
network peer nodes, its neigh entry for the IP address transitions from
STALE to DELAY.  This transition, among other things, updates
neigh->updated. Once the kernel receives the first gratuitous ARP, it
ignores it because its arrival time is inside the locktime interval. The
kernel still bumps neigh->updated. Then the second gratuitous ARP
request arrives, and it's also ignored because it's still in the (new)
locktime interval. Same happens for the third request. The node
eventually heals itself (after delay_first_probe_time seconds since the
initial transition to DELAY state), but it just wasted some time and
require a new ARP request/reply round trip. This unfortunate behaviour
both puts more load on the network, as well as reduces service
availability.

This patch changes neigh_update so that it bumps neigh->updated (as well
as neigh->confirmed) only once we are sure that either lladdr or entry
state will change). In the scenario described above, it means that the
second gratuitous ARP request will actually update the entry lladdr.

Ideally, we would update the neigh entry on the very first gratuitous
ARP request. The locktime mechanism is designed to ignore ARP updates in
a short timeframe after a previous ARP update was honoured by the kernel
layer. This would require tracking timestamps for state transitions
separately from timestamps when actual updates are received. This would
probably involve changes in neighbour struct. Therefore, the patch
doesn't tackle the issue of the first gratuitous APR ignored, leaving
it for a follow-up.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
v2: added more details to commit message to explain relation between arp
and neigh code.
---
 net/core/neighbour.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 58b0bcc..d274f81 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1132,10 +1132,6 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
lladdr = neigh->ha;
}
 
-   if (new & NUD_CONNECTED)
-   neigh->confirmed = jiffies;
-   neigh->updated = jiffies;
-
/* If entry was valid and address is not changed,
   do not change entry state, if new one is STALE.
 */
@@ -1157,6 +1153,16 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
}
}
 
+   /* Update timestamps only once we know we will make a change to the
+* neighbour entry. Otherwise we risk to move the locktime window with
+* noop updates and ignore relevant ARP updates.
+*/
+   if (new != old || lladdr != neigh->ha) {
+   if (new & NUD_CONNECTED)
+   neigh->confirmed = jiffies;
+   neigh->updated = jiffies;
+   }
+
if (new != old) {
neigh_del_timer(neigh);
if (new & NUD_PROBE)
-- 
2.9.3

Re: [PATCH] arp: honour gratuitous ARP _replies_

[Ihar Hrachyshka]

On Mon, May 15, 2017 at 2:16 PM, Ihar Hrachyshka <ihrac...@redhat.com> wrote:
> When arp_accept is 1, gratuitous ARPs are supposed to override matching
> entries irrespective of whether they arrive during locktime. This was
> implemented in commit 56022a8fdd87 ("ipv4: arp: update neighbour address
> when a gratuitous arp is received and arp_accept is set")
>
> There is a glitch in the patch though. RFC 2002, section 4.6, "ARP,
> Proxy ARP, and Gratuitous ARP", defines gratuitous ARPs so that they can
> be either of Request or Reply type. Those Reply gratuitous ARPs can be
> triggered with standard tooling, for example, arping -A option does just
> that.
>
> This patch fixes the glitch, making both Request and Reply flavours of
> gratuitous ARPs to behave identically.
>
> As per RFC, if gratuitous ARPs are of Reply type, their Target Hardware
> Address field should also be set to the link-layer address to which this
> cache entry should be updated. The field is present in ARP over Ethernet
> but not in IEEE 1394. In this patch, I don't consider any broadcasted
> ARP replies as gratuitous if the field is not present, to conform the
> standard. It's not clear whether there is such a thing for IEEE 1394 as
> a gratuitous ARP reply; until it's cleared up, we will ignore such
> broadcasts. Note that they will still update existing ARP cache entries,
> assuming they arrive out of locktime time interval.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>

Please disregard this email, I forgot to update the patch version to
v2 and provide changelog. I posted (hopefully) correct version. Still
learning the process...

Ihar

Re: [PATCH v2] arp: honour gratuitous ARP _replies_

[Ihar Hrachyshka]

When arp_accept is 1, gratuitous ARPs are supposed to override matching
entries irrespective of whether they arrive during locktime. This was
implemented in commit 56022a8fdd87 ("ipv4: arp: update neighbour address
when a gratuitous arp is received and arp_accept is set")

There is a glitch in the patch though. RFC 2002, section 4.6, "ARP,
Proxy ARP, and Gratuitous ARP", defines gratuitous ARPs so that they can
be either of Request or Reply type. Those Reply gratuitous ARPs can be
triggered with standard tooling, for example, arping -A option does just
that.

This patch fixes the glitch, making both Request and Reply flavours of
gratuitous ARPs to behave identically.

As per RFC, if gratuitous ARPs are of Reply type, their Target Hardware
Address field should also be set to the link-layer address to which this
cache entry should be updated. The field is present in ARP over Ethernet
but not in IEEE 1394. In this patch, I don't consider any broadcasted
ARP replies as gratuitous if the field is not present, to conform the
standard. It's not clear whether there is such a thing for IEEE 1394 as
a gratuitous ARP reply; until it's cleared up, we will ignore such
broadcasts. Note that they will still update existing ARP cache entries,
assuming they arrive out of locktime time interval.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
v2:
- removed #ifdef for CONFIG_FIREWIRE_NET
---
 net/ipv4/arp.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 0937b34..d54345a 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -653,6 +653,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
unsigned char *arp_ptr;
struct rtable *rt;
unsigned char *sha;
+   unsigned char *tha = NULL;
__be32 sip, tip;
u16 dev_type = dev->type;
int addr_type;
@@ -724,6 +725,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
break;
 #endif
default:
+   tha = arp_ptr;
arp_ptr += dev->addr_len;
}
memcpy(, arp_ptr, 4);
@@ -842,8 +844,18 @@ 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 = arp->ar_op == htons(ARPOP_REQUEST) && tip == sip &&
- addr_type == RTN_UNICAST;
+   is_garp = tip == sip && addr_type == RTN_UNICAST;
+
+   /* Unsolicited 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);
 
if (!n &&
((arp->ar_op == htons(ARPOP_REPLY)  &&
-- 
2.9.3

Re: [PATCH] neighbour: update neigh timestamps iff update is effective

[Ihar Hrachyshka]

It's a common practice to send gratuitous ARPs after moving an
IP address to another device to speed up healing of a service. To
fulfill service availability constraints, the timing of network peers
updating their caches to point to a new location of an IP address can be
particularly important.

Sometimes neigh_update calls won't touch neither lladdr nor state, for
example if an update arrives in locktime interval. The neigh->updated
value is tested by the protocol specific neigh code, which in turn
will influence whether NEIGH_UPDATE_F_OVERRIDE gets set in the
call to neigh_update() or not. As a result, we may effectively ignore
the update request, bailing out of touching the neigh entry, except that
we still bump its timestamps inside neigh_update.

This may be a problem for updates arriving in quick succession. For
example, consider the following scenario:

A service is moved to another device with its IP address. The new device
sends three gratuitous ARP requests into the network with ~1 seconds
interval between them. Just before the first request arrives to one of
network peer nodes, its neigh entry for the IP address transitions from
STALE to DELAY.  This transition, among other things, updates
neigh->updated. Once the kernel receives the first gratuitous ARP, it
ignores it because its arrival time is inside the locktime interval. The
kernel still bumps neigh->updated. Then the second gratuitous ARP
request arrives, and it's also ignored because it's still in the (new)
locktime interval. Same happens for the third request. The node
eventually heals itself (after delay_first_probe_time seconds since the
initial transition to DELAY state), but it just wasted some time and
require a new ARP request/reply round trip. This unfortunate behaviour
both puts more load on the network, as well as reduces service
availability.

This patch changes neigh_update so that it bumps neigh->updated (as well
as neigh->confirmed) only once we are sure that either lladdr or entry
state will change). In the scenario described above, it means that the
second gratuitous ARP request will actually update the entry lladdr.

Ideally, we would update the neigh entry on the very first gratuitous
ARP request. The locktime mechanism is designed to ignore ARP updates in
a short timeframe after a previous ARP update was honoured by the kernel
layer. This would require tracking timestamps for state transitions
separately from timestamps when actual updates are received. This would
probably involve changes in neighbour struct. Therefore, the patch
doesn't tackle the issue of the first gratuitous APR ignored, leaving
it for a follow-up.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
 net/core/neighbour.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 58b0bcc..d274f81 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1132,10 +1132,6 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
lladdr = neigh->ha;
}
 
-   if (new & NUD_CONNECTED)
-   neigh->confirmed = jiffies;
-   neigh->updated = jiffies;
-
/* If entry was valid and address is not changed,
   do not change entry state, if new one is STALE.
 */
@@ -1157,6 +1153,16 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
}
}
 
+   /* Update timestamps only once we know we will make a change to the
+* neighbour entry. Otherwise we risk to move the locktime window with
+* noop updates and ignore relevant ARP updates.
+*/
+   if (new != old || lladdr != neigh->ha) {
+   if (new & NUD_CONNECTED)
+   neigh->confirmed = jiffies;
+   neigh->updated = jiffies;
+   }
+
if (new != old) {
neigh_del_timer(neigh);
if (new & NUD_PROBE)
-- 
2.9.3

Re: [PATCH] neighbour: update neigh timestamps iff update is effective

[Ihar Hrachyshka]

On Mon, May 15, 2017 at 1:05 PM, Julian Anastasov  wrote:
>
> It seems arp_accept value currently has influence on
> the locktime for GARP requests. My understanding is that
> locktime is used to ignore replies from proxy_arp
> routers while the requested IP is present on the LAN
> and replies immediately. IMHO, GARP requests should not
> depend on locktime, even when arp_accept=0. For example:

Yes, I believe so.

I actually thought about introducing the patch that does just that:
forcing override on garp, but then I was thinking, maybe there is some
reason to still apply locktime rules to garps; f.e. if you have
multiple nodes carrying the ip address and located on the same
segment, maybe you want to pick the first that replies to you (in
theory, it may be the node that is less loaded, or closer to us; but
then, it's so fragile even if that was the intent...) Do you want me
to post the patch, or will you cover it?

Ihar

Re: [PATCH] arp: honour gratuitous ARP _replies_

[Ihar Hrachyshka]

When arp_accept is 1, gratuitous ARPs are supposed to override matching
entries irrespective of whether they arrive during locktime. This was
implemented in commit 56022a8fdd87 ("ipv4: arp: update neighbour address
when a gratuitous arp is received and arp_accept is set")

There is a glitch in the patch though. RFC 2002, section 4.6, "ARP,
Proxy ARP, and Gratuitous ARP", defines gratuitous ARPs so that they can
be either of Request or Reply type. Those Reply gratuitous ARPs can be
triggered with standard tooling, for example, arping -A option does just
that.

This patch fixes the glitch, making both Request and Reply flavours of
gratuitous ARPs to behave identically.

As per RFC, if gratuitous ARPs are of Reply type, their Target Hardware
Address field should also be set to the link-layer address to which this
cache entry should be updated. The field is present in ARP over Ethernet
but not in IEEE 1394. In this patch, I don't consider any broadcasted
ARP replies as gratuitous if the field is not present, to conform the
standard. It's not clear whether there is such a thing for IEEE 1394 as
a gratuitous ARP reply; until it's cleared up, we will ignore such
broadcasts. Note that they will still update existing ARP cache entries,
assuming they arrive out of locktime time interval.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
 net/ipv4/arp.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 0937b34..d54345a 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -653,6 +653,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
unsigned char *arp_ptr;
struct rtable *rt;
unsigned char *sha;
+   unsigned char *tha = NULL;
__be32 sip, tip;
u16 dev_type = dev->type;
int addr_type;
@@ -724,6 +725,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
break;
 #endif
default:
+   tha = arp_ptr;
arp_ptr += dev->addr_len;
}
memcpy(, arp_ptr, 4);
@@ -842,8 +844,18 @@ 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 = arp->ar_op == htons(ARPOP_REQUEST) && tip == sip &&
- addr_type == RTN_UNICAST;
+   is_garp = tip == sip && addr_type == RTN_UNICAST;
+
+   /* Unsolicited 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);
 
if (!n &&
((arp->ar_op == htons(ARPOP_REPLY)  &&
-- 
2.9.3

[PATCH] arp: honour gratuitous ARP _replies_

[Ihar Hrachyshka]

When arp_accept is 1, gratuitous ARPs are supposed to override matching
entries irrespective of whether they arrive during locktime. This was
implemented in commit 56022a8fdd87 ("ipv4: arp: update neighbour address
when a gratuitous arp is received and arp_accept is set")

There is a glitch in the patch though. RFC 2002, section 4.6, "ARP,
Proxy ARP, and Gratuitous ARP", defines gratuitous ARPs so that they can
be either of Request or Reply type. Those Reply gratuitous ARPs can be
triggered with standard tooling, for example, arping -A option does just
that.

This patch fixes the glitch, making both Request and Reply flavours of
gratuitous ARPs to behave identically.

As per RFC, if gratuitous ARPs are of Reply type, their Target Hardware
Address field should also be set to the link-layer address to which this
cache entry should be updated. The field is present in ARP over Ethernet
but not in IEEE 1394. In this patch, I don't consider any broadcasted
ARP replies as gratuitous if the field is not present, to conform the
standard. It's not clear whether there is such a thing for IEEE 1394 as
a gratuitous ARP reply; until it's cleared up, we will ignore such
broadcasts. Note that they will still update existing ARP cache entries,
assuming they arrive out of locktime time interval.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
 net/ipv4/arp.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 0937b34..fb97b9c 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -653,6 +653,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
unsigned char *arp_ptr;
struct rtable *rt;
unsigned char *sha;
+   unsigned char *tha = NULL;
__be32 sip, tip;
u16 dev_type = dev->type;
int addr_type;
@@ -724,6 +725,7 @@ static int arp_process(struct net *net, struct sock *sk, 
struct sk_buff *skb)
break;
 #endif
default:
+   tha = arp_ptr;
arp_ptr += dev->addr_len;
}
memcpy(, arp_ptr, 4);
@@ -842,8 +844,20 @@ 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 = arp->ar_op == htons(ARPOP_REQUEST) && tip == sip &&
- addr_type == RTN_UNICAST;
+   is_garp = tip == sip && addr_type == RTN_UNICAST;
+
+   /* Unsolicited ARP _replies_ also require target hwaddr to be
+* the same as source.
+*/
+   if (is_garp && arp->ar_op == htons(ARPOP_REPLY))
+   is_garp =
+#if IS_ENABLED(CONFIG_FIREWIRE_NET)
+   /* IPv4 over IEEE 1394 doesn't provide target
+* hardware address field in its ARP payload.
+*/
+   tha &&
+#endif
+   !memcmp(tha, sha, dev->addr_len);
 
if (!n &&
((arp->ar_op == htons(ARPOP_REPLY)  &&
-- 
2.9.3

[PATCH] neighbour: update neigh timestamps iff update is effective

[Ihar Hrachyshka]

It's a common practice to send gratuitous ARPs after moving an
IP address to another device to speed up healing of a service. To
fulfill service availability constraints, the timing of network peers
updating their caches to point to a new location of an IP address can be
particularly important.

Sometimes neigh_update calls won't touch neither lladdr nor state, for
example if an update arrives in locktime interval. Then we effectively
ignore the update request, bailing out of touching the neigh entry,
except that we still bump its timestamps.

This may be a problem for updates arriving in quick succession. For
example, consider the following scenario:

A service is moved to another device with its IP address. The new device
sends three gratuitous ARP requests into the network with ~1 seconds
interval between them. Just before the first request arrives to one of
network peer nodes, its neigh entry for the IP address transitions from
STALE to DELAY.  This transition, among other things, updates
neigh->updated. Once the kernel receives the first gratuitous ARP, it
ignores it because its arrival time is inside the locktime interval. The
kernel still bumps neigh->updated. Then the second gratuitous ARP
request arrives, and it's also ignored because it's still in the (new)
locktime interval. Same happens for the third request. The node
eventually heals itself (after delay_first_probe_time seconds since the
initial transition to DELAY state), but it just wasted some time and
require a new ARP request/reply round trip. This unfortunate behaviour
both puts more load on the network, as well as reduces service
availability.

This patch changes neigh_update so that it bumps neigh->updated (as well
as neigh->confirmed) only once we are sure that either lladdr or entry
state will change). In the scenario described above, it means that the
second gratuitous ARP request will actually update the entry lladdr.

Ideally, we would update the neigh entry on the very first gratuitous
ARP request. The locktime mechanism is designed to ignore ARP updates in
a short timeframe after a previous ARP update was honoured by the kernel
layer. This would require tracking timestamps for state transitions
separately from timestamps when actual updates are received. This would
probably involve changes in neighbour struct. Therefore, the patch
doesn't tackle the issue of the first gratuitous APR ignored, leaving
it for a follow-up.

Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
---
 net/core/neighbour.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 58b0bcc..d274f81 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1132,10 +1132,6 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
lladdr = neigh->ha;
}
 
-   if (new & NUD_CONNECTED)
-   neigh->confirmed = jiffies;
-   neigh->updated = jiffies;
-
/* If entry was valid and address is not changed,
   do not change entry state, if new one is STALE.
 */
@@ -1157,6 +1153,16 @@ int neigh_update(struct neighbour *neigh, const u8 
*lladdr, u8 new,
}
}
 
+   /* Update timestamps only once we know we will make a change to the
+* neighbour entry. Otherwise we risk to move the locktime window with
+* noop updates and ignore relevant ARP updates.
+*/
+   if (new != old || lladdr != neigh->ha) {
+   if (new & NUD_CONNECTED)
+   neigh->confirmed = jiffies;
+   neigh->updated = jiffies;
+   }
+
if (new != old) {
neigh_del_timer(neigh);
if (new & NUD_PROBE)
-- 
2.9.3