netdev - Re: [PATCH] neighbour: update neigh timestamps iff update is effective

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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>