From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: Salam Noureddine <noureddine@aristanetworks.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Daniel Borkmann <dborkman@redhat.com>,
Willem de Bruijn <willemb@google.com>, Phil Sutter <phil@nwl.cc>,
Eric Dumazet <edumazet@google.com>,
netdev@vger.kernel.org
Subject: Re: Issue with gratuitous arps when new addr is different from cached addr
Date: Thu, 21 Nov 2013 05:40:48 +0100 [thread overview]
Message-ID: <20131121044048.GB4347@order.stressinduktion.org> (raw)
In-Reply-To: <CAO7SqHAVz2D53u141Za01DvBFnLky2imkYNvBV-2UuALpC3w6w@mail.gmail.com>
On Wed, Nov 20, 2013 at 04:40:52PM -0800, Salam Noureddine wrote:
> Hi,
>
> It seems to me that neigh_update is not handling correctly the case
> when the new address is different from the cached one and
> NEIGH_UPDATE_F_OVERRIDE is not set. When we receive a gratuitous arp
> request we check jiffies against the neigh->updated + locktime in
> arp_process. If we're passed that time then the flag is set.
>
> In neigh_update, we set neigh->updated before checking for the case
> where we have a new address and the override flag is not set. This
> means, that we "extend the life of the old address". By setting
> locktime to 2 sec and sending an arp with a new address every 1 sec, I
> was able to perpetuate the old entry for as long as I wanted.
>
> To fix this, we can just move setting neigh->updated to after the
> check for new address and override flag not present,
>
> --- linux-3.4.orig/net/core/neighbour.c
> +++ linux-3.4/net/core/neighbour.c
> @@ -1206,10 +1206,6 @@ int neigh_update(struct neighbour *neigh
> 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.
> */
> @@ -1233,6 +1229,10 @@ int neigh_update(struct neighbour *neigh
> }
> }
>
> + if (new & NUD_CONNECTED)
> + neigh->confirmed = jiffies;
> + neigh->updated = jiffies;
> +
> if (new != old) {
> neigh_del_timer(neigh);
> if (new & NUD_IN_TIMER)
>
> If that seems like a an acceptable solution, I would post a patch shortly.
Yes, that would help. But wouldn't it be better if we detect garp and
overwrite the lladdr with F_OVERWRITE? It would be nice if these could also be
rate-limited.
Greetings,
Hannes
next prev parent reply other threads:[~2013-11-21 4:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-21 0:40 Issue with gratuitous arps when new addr is different from cached addr Salam Noureddine
2013-11-21 4:40 ` Hannes Frederic Sowa [this message]
2013-11-21 6:06 ` Salam Noureddine
2013-11-21 6:23 ` Hannes Frederic Sowa
2013-11-21 6:26 ` Hannes Frederic Sowa
2013-11-21 6:33 ` Salam Noureddine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131121044048.GB4347@order.stressinduktion.org \
--to=hannes@stressinduktion.org \
--cc=davem@davemloft.net \
--cc=dborkman@redhat.com \
--cc=edumazet@google.com \
--cc=netdev@vger.kernel.org \
--cc=noureddine@aristanetworks.com \
--cc=phil@nwl.cc \
--cc=willemb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox