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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 15 Nov 2024 11:55:21 -0800
Kuniyuki Iwashima <kuniyu@amazon.com> wrote:

> From: Stefano Brivio <sbrivio@redhat.com>
> Date: Thu, 14 Nov 2024 22:54:14 +0100
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index da50df485090..fcd2e2b89876 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -643,8 +643,17 @@ static int sock_bindtoindex_locked(struct sock *sk, int ifindex)
> >  	/* Paired with all READ_ONCE() done locklessly. */
> >  	WRITE_ONCE(sk->sk_bound_dev_if, ifindex);
> >  
> > -	if (sk->sk_prot->rehash)
> > -		sk->sk_prot->rehash(sk);
> > +	/* Force rehash if protocol needs it */
> > +	if (sk->sk_prot->set_rcv_saddr) {
> > +		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > +			sk->sk_prot->set_rcv_saddr(sk, &sk->sk_v6_rcv_saddr);  
> 
> sk_v6_rcv_saddr is not defined without CONFIG_IPV6 so I think the
> compiler will complain ?  see net/ipv4/inet_connection_sock.c

You're right, it breaks the build for CONFIG_IPV6=n, I should have
checked. Fixed in v1.

I'll post it for net-next when it reopens.

> > +		} else if (sk->sk_family == AF_INET) {
> > +			struct inet_sock *inet = inet_sk(sk);
> > +
> > +			sk->sk_prot->set_rcv_saddr(sk, &inet->inet_rcv_saddr);  
> 
> simply use &sk->sk_rcv_saddr.

Changed.

> > +		}
> > +	}
> > +
> >  	sk_dst_reset(sk);
> >  
> >  	ret = 0;  
> [...]
> > @@ -2034,20 +2052,32 @@ void udp_lib_rehash(struct sock *sk, u16 newhash)
> >  				nhslot2->count++;
> >  				spin_unlock(&nhslot2->lock);
> >  			}
> > -
> > -			spin_unlock_bh(&hslot->lock);
> >  		}
> >  	}
> > +
> > +	if (sk->sk_family == AF_INET)
> > +		sk->sk_rcv_saddr = *(__be32 *)addr;
> > +	else if (sk->sk_family == AF_INET6)
> > +		sk->sk_v6_rcv_saddr = *(struct in6_addr *)addr;  
> 
> inet_update_saddr() can be reused ?  at least we should
> use sk_rcv_saddr_set().

Ah, thanks, that's beautiful, I didn't know about inet_update_saddr().
Moved to headers and reused here.

It also updates sk_v6_rcv_saddr to the v4-mapped address if sk_family
is AF_INET, but that's correct anyway.

> Same for other places.

I think that makes sense but I guess it's beyond the scope of this
series, because it has the side effect of setting inet_saddr and
enabling further clean-ups. For example, in __ip4_datagram_connect(),
we could simplify all this:

	if (!inet->inet_saddr)
		inet->inet_saddr = fl4->saddr;	/* Update source address */
	if (!inet->inet_rcv_saddr) {
		inet->inet_rcv_saddr = fl4->saddr;
		if (sk->sk_prot->rehash)
			sk->sk_prot->rehash(sk);
	}

...let me do that as a follow-up patch. This series already looks big
enough.

-- 
Stefano