Message ID | CAO7SqHAVz2D53u141Za01DvBFnLky2imkYNvBV-2UuALpC3w6w@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
--- 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)