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

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

[Vasily Khoruzhick]

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 <vasilykh@arista.com>
---
 net/core/neighbour.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index aa19d86937af..901418ef70ea 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1180,6 +1180,9 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		lladdr = neigh->ha;
 	}
 
+	if (new & NUD_CONNECTED)
+		neigh->confirmed = jiffies;
+
 	/* If entry was valid and address is not changed,
 	   do not change entry state, if new one is STALE.
 	 */
@@ -1205,11 +1208,8 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 	 * 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;
+	if (new != old || lladdr != neigh->ha)
 		neigh->updated = jiffies;
-	}
 
 	if (new != old) {
 		neigh_del_timer(neigh);
-- 
2.18.0

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

[David Miller]

From: Vasily Khoruzhick <vasilykh@arista.com>
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 <vasilykh@arista.com>

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.

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

[Ihar Hrachyshka]

On Sat, Sep 1, 2018 at 4:51 PM, David Miller <davem@davemloft.net> wrote:
> From: Vasily Khoruzhick <vasilykh@arista.com>
> 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 <vasilykh@arista.com>
>
> 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

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

[David Miller]

From: Ihar Hrachyshka <ihrachys@redhat.com>
Date: Tue, 4 Sep 2018 11:31:23 -0700

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

Yes, that would help a lot.