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
| ||
|
Message-ID: <alpine.LFD.2.20.1705152234490.4002@ja.home.ssi.bg> Date: Mon, 15 May 2017 23:05:24 +0300 (EEST) From: Julian Anastasov <ja@....bg> To: Ihar Hrachyshka <ihrachys@...hat.com> cc: "David S. Miller" <davem@...emloft.net>, He Chunhui <hchunhui@...l.ustc.edu.cn>, netdev@...r.kernel.org Subject: Re: [PATCH] neighbour: update neigh timestamps iff update is effective Hello, On Tue, 9 May 2017, Ihar Hrachyshka wrote: > It's a common practice to send gratuitous ARPs after moving an > IP address to another device to speed up healing of a service. To > fulfill service availability constraints, the timing of network peers > updating their caches to point to a new location of an IP address can be > particularly important. > > Sometimes neigh_update calls won't touch neither lladdr nor state, for > example if an update arrives in locktime interval. Then we effectively > ignore the update request, bailing out of touching the neigh entry, > except that we still bump its timestamps. > > This may be a problem for updates arriving in quick succession. For > example, consider the following scenario: > > A service is moved to another device with its IP address. The new device > sends three gratuitous ARP requests into the network with ~1 seconds > interval between them. Just before the first request arrives to one of > network peer nodes, its neigh entry for the IP address transitions from > STALE to DELAY. This transition, among other things, updates > neigh->updated. Once the kernel receives the first gratuitous ARP, it > ignores it because its arrival time is inside the locktime interval. The > kernel still bumps neigh->updated. Then the second gratuitous ARP > request arrives, and it's also ignored because it's still in the (new) > locktime interval. Same happens for the third request. The node > eventually heals itself (after delay_first_probe_time seconds since the > initial transition to DELAY state), but it just wasted some time and > require a new ARP request/reply round trip. This unfortunate behaviour > both puts more load on the network, as well as reduces service > availability. > > This patch changes neigh_update so that it bumps neigh->updated (as well > as neigh->confirmed) only once we are sure that either lladdr or entry > state will change). In the scenario described above, it means that the > second gratuitous ARP request will actually update the entry lladdr. > > Ideally, we would update the neigh entry on the very first gratuitous > ARP request. The locktime mechanism is designed to ignore ARP updates in > a short timeframe after a previous ARP update was honoured by the kernel > layer. This would require tracking timestamps for state transitions > separately from timestamps when actual updates are received. This would > probably involve changes in neighbour struct. Therefore, the patch > doesn't tackle the issue of the first gratuitous APR ignored, leaving > it for a follow-up. > > Signed-off-by: Ihar Hrachyshka <ihrachys@...hat.com> Looks ok to me, Acked-by: Julian Anastasov <ja@....bg> It seems arp_accept value currently has influence on the locktime for GARP requests. My understanding is that locktime is used to ignore replies from proxy_arp routers while the requested IP is present on the LAN and replies immediately. IMHO, GARP requests should not depend on locktime, even when arp_accept=0. For example: if (IN_DEV_ARP_ACCEPT(in_dev)) { ... + } else if (n && tip == sip && arp->ar_op == htons(ARPOP_REQUEST)) { + unsigned int addr_type = inet_addr_type_dev_table(net, dev, sip); + + is_garp = (addr_type == RTN_UNICAST); } > --- > net/core/neighbour.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 58b0bcc..d274f81 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -1132,10 +1132,6 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new, > 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. > */ > @@ -1157,6 +1153,16 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new, > } > } > > + /* Update timestamps only once we know we will make a change to the > + * neighbour entry. Otherwise we risk to move the locktime window with > + * noop updates and ignore relevant ARP updates. > + */ > + if (new != old || lladdr != neigh->ha) { > + if (new & NUD_CONNECTED) > + neigh->confirmed = jiffies; > + neigh->updated = jiffies; > + } > + > if (new != old) { > neigh_del_timer(neigh); > if (new & NUD_PROBE) > -- > 2.9.3 Regards -- Julian Anastasov <ja@....bg>
Powered by blists - more mailing lists