Issue with gratuitous arps when new addr is different from cached addr

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,


If that seems like a an acceptable solution, I would post a patch shortly.

Thanks,

Salam
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html