Re: [PATCH net-next 2/2] datagram, udp: Set local address and rehash socket atomically against lookup

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 6 Dec 2024 13:36:47 +0100
Paolo Abeni <pabeni@redhat.com> wrote:

> On 12/6/24 11:50, Stefano Brivio wrote:
> > On Thu, 5 Dec 2024 17:53:33 +0100 Paolo Abeni <pabeni@redhat.com> wrote:  
> >> I'm wondering if the issue could be solved (almost) entirely in the
> >> rehash callback?!? if the rehash happens on connect and the the socket
> >> does not have hash4 yet (it's not a reconnect) do the l4 hashing before
> >> everything else.  
> > 
> > So, yes, that's actually the first thing I tried: do the hashing (any
> > hash) before setting the address (I guess that's what you mean by
> > "everything else").
> > 
> > If you take this series, and drop the changes in __udp4_lib_lookup(), I
> > guess that would match what you suggest.  
> 
> I mean something slightly different. Just to explain the idea something
> alike the following (completely untested):
> 
> ---
> diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
> index cc6d0bd7b0a9..e9cc6edbcdc6 100644
> --- a/net/ipv4/datagram.c
> +++ b/net/ipv4/datagram.c
> @@ -61,6 +61,10 @@ int __ip4_datagram_connect(struct sock *sk, struct
> sockaddr *uaddr, int addr_len
>  		err = -EACCES;
>  		goto out;
>  	}
> +
> +	sk->sk_state = TCP_ESTABLISHED;
> +	inet->inet_daddr = fl4->daddr;
> +	inet->inet_dport = usin->sin_port;
>  	if (!inet->inet_saddr)
>  		inet->inet_saddr = fl4->saddr;	/* Update source address */
>  	if (!inet->inet_rcv_saddr) {
> @@ -68,10 +72,7 @@ int __ip4_datagram_connect(struct sock *sk, struct
> sockaddr *uaddr, int addr_len
>  		if (sk->sk_prot->rehash)
>  			sk->sk_prot->rehash(sk);
>  	}
> -	inet->inet_daddr = fl4->daddr;
> -	inet->inet_dport = usin->sin_port;
>  	reuseport_has_conns_set(sk);
> -	sk->sk_state = TCP_ESTABLISHED;
>  	sk_set_txhash(sk);
>  	atomic_set(&inet->inet_id, get_random_u16());
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 6a01905d379f..c6c58b0a6b7b 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2194,6 +2194,21 @@ void udp_lib_rehash(struct sock *sk, u16 newhash,
> u16 newhash4)
>  			if (rcu_access_pointer(sk->sk_reuseport_cb))
>  				reuseport_detach_sock(sk);
> 
> +			if (sk->sk_state == TCP_ESTABLISHED && !udp_hashed4(sk)) {
> +				struct udp_hslot * hslot4 = udp_hashslot4(udptable, newhash4);
> +
> +				udp_sk(sk)->udp_lrpa_hash = newhash4;
> +				spin_lock(&hslot4->lock);
> +				hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node,
> +							 &hslot4->nulls_head);
> +				hslot4->count++;
> +				spin_unlock(&hslot4->lock);
> +
> +				spin_lock(&hslot2->lock);
> +				udp_hash4_inc(hslot2);
> +				spin_unlock(&hslot2->lock);
> +			}
> +
>  			if (hslot2 != nhslot2) {
>  				spin_lock(&hslot2->lock);
>  				hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node);
> ---
> 
> Basically the idea is to leverage the hash4 - which should be not yet
> initialized when rehash is invoked due to connect().

That assumption seems to be correct from my tests.

> In such a case, before touching hash{,2}, do hash4.

Brilliant, thanks. I'll give that a try.

-- 
Stefano