netdev - Re: [net-next] tcp: Avoid sending an erroneous RST due to RCU race

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <72f54aa95c3fad328b00b8196ca7f878c5d0a627.camel@redhat.com>
Date: Tue, 09 Jan 2024 08:24:48 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jon Maxwell <jmaxwell37@...il.com>, davem@...emloft.net
Cc: edumazet@...gle.com, kuba@...nel.org, dsahern@...nel.org, 
	netdev@...r.kernel.org, Davide Caratti <dcaratti@...hat.com>
Subject: Re: [net-next] tcp: Avoid sending an erroneous RST due to RCU race

On Tue, 2024-01-09 at 10:13 +1100, Jon Maxwell wrote:
> There are 2 cases where a socket lookup races due to RCU and finds a 
> LISTEN socket instead of an ESTABLISHED or a TIME-WAIT socket. As the ACK flag 
> is set this will generate an erroneous RST. 
> 
> There are 2 scenarios, one where 2 ACKs (one for the 3 way handshake and 
> another with the same sequence number carrying data) are sent with a very 
> small time interval between them. In this case the 2 ACKs can race while being
> processed on different CPUs and the latter may find the LISTEN socket instead 
> of the ESTABLISHED socket. That will make the one end of the TCP connection 
> out of sync with the other and cause a break in communications. The other 
> scenario is a "FIN ACK" racing with an ACK which may also find the LISTEN 
> socket instead of the TIME_WAIT socket. Instead of getting ignored that 
> generates an invalid RST. 
> 
> Instead of the next connection attempt succeeding. The client then gets an 
> ECONNREFUSED error on the next connection attempt when it finds a socket in 
> the FIN_WAIT_2 state as discussed here: 
> 
> https://lore.kernel.org/netdev/20230606064306.9192-1-duanmuquan@baidu.com/ 
> 
> Modeled on Erics idea, introduce __inet_lookup_skb_locked() and
> __inet6_lookup_skb_locked()  to fix this by doing a locked lookup only for 
> these rare cases to avoid finding the LISTEN socket. 

I think Eric's idea was to keep the bucket lock held after such lookup,
to avoid possibly re-acquiring it for time-wait sockets.

> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 93e9193df54461b25c61089bd5db4dd33c32dab6..ef9c0b5462bd0a85ebf350f53b4f3e6f6d394282 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -535,6 +535,55 @@ struct sock *__inet_lookup_established(struct net *net,
>  }
>  EXPORT_SYMBOL_GPL(__inet_lookup_established);
>  
> +struct sock *__inet_lookup_established_locked(struct net *net,
> +					      struct inet_hashinfo *hashinfo,
> +					      const __be32 saddr, const __be16 sport,
> +					      const __be32 daddr, const u16 hnum,
> +					      const int dif, const int sdif)
> +{
> +	INET_ADDR_COOKIE(acookie, saddr, daddr);
> +	const __portpair ports = INET_COMBINED_PORTS(sport, hnum);
> +	struct sock *sk;
> +	const struct hlist_nulls_node *node;
> +	/* Optimize here for direct hit, only listening connections can
> +	 * have wildcards anyways.
> +	 */
> +	unsigned int hash = inet_ehashfn(net, daddr, hnum, saddr, sport);
> +	unsigned int slot = hash & hashinfo->ehash_mask;
> +	struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
> +	spinlock_t *lock = inet_ehash_lockp(hashinfo, hash);
> +
> +	spin_lock(lock);
> +begin:
> +	sk_nulls_for_each(sk, node, &head->chain) {
> +		if (sk->sk_hash != hash)
> +			continue;
> +		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
> +			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +				goto out;
> +			if (unlikely(!inet_match(net, sk, acookie,
> +						 ports, dif, sdif))) {
> +				sock_gen_put(sk);
> +				goto begin;
> +			}
> +			goto found;
> +		}

Since the bucket is locked, I think refcount_inc() would suffice. The
double match and the additional looping should not be needed.


Cheers,

Paolo