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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwN9=A6YEU5QWf9cQjHwv6L_ssOH5K_rapE6hoZtF_A4gokuQ@mail.gmail.com>
Date:   Tue, 4 Sep 2018 11:31:23 -0700
From:   Ihar Hrachyshka <ihrachys@...hat.com>
To:     David Miller <davem@...emloft.net>
Cc:     Vasiliy Khoruzhick <vasilykh@...sta.com>,
        roopa@...ulusnetworks.com, adobriyan@...il.com,
        jwestfall@...realistic.net, stephen@...workplumber.org,
        anarsoul@...il.com, keescook@...omium.org, w.bumiller@...xmox.com,
        edumazet@...gle.com, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH] neighbour: confirm neigh entries when ARP packet is received

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