* Issue with gratuitous arps when new addr is different from cached addr
@ 2013-11-21 0:40 Salam Noureddine
2013-11-21 4:40 ` Hannes Frederic Sowa
0 siblings, 1 reply; 6+ messages in thread
From: Salam Noureddine @ 2013-11-21 0:40 UTC (permalink / raw)
To: David S. Miller, Daniel Borkmann, Willem de Bruijn, Phil Sutter,
Eric Dumazet, netdev
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.
Thanks,
Salam
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Issue with gratuitous arps when new addr is different from cached addr
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
2013-11-21 6:06 ` Salam Noureddine
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-21 4:40 UTC (permalink / raw)
To: Salam Noureddine
Cc: David S. Miller, Daniel Borkmann, Willem de Bruijn, Phil Sutter,
Eric Dumazet, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Issue with gratuitous arps when new addr is different from cached addr
2013-11-21 4:40 ` Hannes Frederic Sowa
@ 2013-11-21 6:06 ` Salam Noureddine
2013-11-21 6:23 ` Hannes Frederic Sowa
0 siblings, 1 reply; 6+ messages in thread
From: Salam Noureddine @ 2013-11-21 6:06 UTC (permalink / raw)
To: Salam Noureddine, David S. Miller, Daniel Borkmann,
Willem de Bruijn, Phil Sutter, Eric Dumazet, netdev
Isn't locktime useful in that case for limiting the rate of garp
changes to the arp cache?
Thanks,
Salam
On Wed, Nov 20, 2013 at 8:40 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Issue with gratuitous arps when new addr is different from cached addr
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
0 siblings, 2 replies; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-21 6:23 UTC (permalink / raw)
To: Salam Noureddine
Cc: David S. Miller, Daniel Borkmann, Willem de Bruijn, Phil Sutter,
Eric Dumazet, netdev
On Wed, Nov 20, 2013 at 10:06:34PM -0800, Salam Noureddine wrote:
> Isn't locktime useful in that case for limiting the rate of garp
> changes to the arp cache?
Yes, as I said, it would be nice to have rate-limiting for those, too.
GARP is used in cluster setups to make the switch-over as fast as possible and
I don't think they accept those lock-down delays, so I guess it would be nice
to forceful override the lladdr if (sip == tip) && ACCEPT_ARP(dev) is true in
every case.
I guess you are not testing with ACCEPT_ARP?
In that case I am not sure what to do.
override = (...timing...) || sip == tip; could work but does relax the
protection of the neigh cache.
In IPv6 we have a flag in the packet if we should overwrite the entry.
I'll have to think about that a bit more. Could well be the case that we need
your proposal, too. But then we would have to validate the change with IPv6,
too and neighbour cache states are really complex.
Greetings,
Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Issue with gratuitous arps when new addr is different from cached addr
2013-11-21 6:23 ` Hannes Frederic Sowa
@ 2013-11-21 6:26 ` Hannes Frederic Sowa
2013-11-21 6:33 ` Salam Noureddine
1 sibling, 0 replies; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-21 6:26 UTC (permalink / raw)
To: Salam Noureddine, David S. Miller, Daniel Borkmann,
Willem de Bruijn, Phil Sutter, Eric Dumazet, netdev
On Thu, Nov 21, 2013 at 07:23:49AM +0100, Hannes Frederic Sowa wrote:
> On Wed, Nov 20, 2013 at 10:06:34PM -0800, Salam Noureddine wrote:
> > Isn't locktime useful in that case for limiting the rate of garp
> > changes to the arp cache?
>
> Yes, as I said, it would be nice to have rate-limiting for those, too.
>
> GARP is used in cluster setups to make the switch-over as fast as possible and
> I don't think they accept those lock-down delays, so I guess it would be nice
> to forceful override the lladdr if (sip == tip) && ACCEPT_ARP(dev) is true in
> every case.
>
> I guess you are not testing with ACCEPT_ARP?
>
> In that case I am not sure what to do.
>
> override = (...timing...) || sip == tip; could work but does relax the
> protection of the neigh cache.
>
> In IPv6 we have a flag in the packet if we should overwrite the entry.
>
> I'll have to think about that a bit more. Could well be the case that we need
> your proposal, too. But then we would have to validate the change with IPv6,
> too and neighbour cache states are really complex.
Hmm, could it help to bring the neighbor to suspect state (== WEAK_OVERRIDE)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Issue with gratuitous arps when new addr is different from cached addr
2013-11-21 6:23 ` Hannes Frederic Sowa
2013-11-21 6:26 ` Hannes Frederic Sowa
@ 2013-11-21 6:33 ` Salam Noureddine
1 sibling, 0 replies; 6+ messages in thread
From: Salam Noureddine @ 2013-11-21 6:33 UTC (permalink / raw)
To: Salam Noureddine, David S. Miller, Daniel Borkmann,
Willem de Bruijn, Phil Sutter, Eric Dumazet, netdev
On Wed, Nov 20, 2013 at 10:23 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Wed, Nov 20, 2013 at 10:06:34PM -0800, Salam Noureddine wrote:
>> Isn't locktime useful in that case for limiting the rate of garp
>> changes to the arp cache?
>
> Yes, as I said, it would be nice to have rate-limiting for those, too.
>
> GARP is used in cluster setups to make the switch-over as fast as possible and
> I don't think they accept those lock-down delays, so I guess it would be nice
> to forceful override the lladdr if (sip == tip) && ACCEPT_ARP(dev) is true in
> every case.
I was in fact testing a switch-over scenario and saw that the arp
update was taking
much longer than the locktime period which pointed to this problem.
>
> I guess you are not testing with ACCEPT_ARP?
>
Yes, I am not setting ACCEPT_ARP.
> In that case I am not sure what to do.
>
> override = (...timing...) || sip == tip; could work but does relax the
> protection of the neigh cache.
>
> In IPv6 we have a flag in the packet if we should overwrite the entry.
>
> I'll have to think about that a bit more. Could well be the case that we need
> your proposal, too. But then we would have to validate the change with IPv6,
> too and neighbour cache states are really complex.
>
> Greetings,
>
> Hannes
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-21 6:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox