Re: [PATCH RFC net 1/2] datagram: Rehash sockets only if local address changed for their family

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 15 Nov 2024 19:10:24 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> [Updated Mike Manning's address in Cc:]
> 
> On Fri, 15 Nov 2024 12:48:29 -0500
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> > Stefano Brivio wrote:  
> > > It makes no sense to rehash an IPv4 socket when we change
> > > sk_v6_rcv_saddr, or to rehash an IPv6 socket as inet_rcv_saddr is set:
> > > the secondary hash (including the local address) won't change, because
> > > ipv4_portaddr_hash() and ipv6_portaddr_hash() only take the address
> > > matching the socket family.    
> > 
> > Even if this is correct, it sounds like an optimization.  
> 
> It is, see the cover letter.
> 
> > If so, it belongs in net-next.  
> 
> Well, it makes the fix smaller.
> 
> > Avoid making a fix (to net and eventually stable kernels) conditional
> > on optimizations that are not suitable for stable cherry-picks.  
> 
> Given that the fix is for an issue that existed for 15 years, I don't
> think it's stable material.
> 
> Whether it's 'net' material is also debatable, if it looks too big to
> you it probably isn't, let's go for net-next even if it's a fix.
> 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  net/ipv4/datagram.c | 2 +-
> > >  net/ipv6/datagram.c | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
> > > index cc6d0bd7b0a9..d52333e921f3 100644
> > > --- a/net/ipv4/datagram.c
> > > +++ b/net/ipv4/datagram.c
> > > @@ -65,7 +65,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
> > >  		inet->inet_saddr = fl4->saddr;	/* Update source address */
> > >  	if (!inet->inet_rcv_saddr) {
> > >  		inet->inet_rcv_saddr = fl4->saddr;
> > > -		if (sk->sk_prot->rehash)
> > > +		if (sk->sk_prot->rehash && sk->sk_family == AF_INET)
> > >  			sk->sk_prot->rehash(sk);    
> > 
> > When is sk_family != AF_INET in __ip4_datagram_connect?  
> 
> This happens with dual-stack sockets, that is, IPv6 sockets that don't
> have IPV6_V6ONLY set, on which you connect() using an IPv4 address.
> 
> I haven't checked whether this makes sense in the bigger picture,
> because trying to avoid this case is definitely beyond the scope of this
> patch, but you can make it happen quite easily by simply starting a
> recent Debian or Fedora with OpenSSH listening on both families
> (default settings).

Ah, sorry, it's the other way around: the v4 rehash is called on a
AF_INET6 socket in that case.

I can have a look at what I can reproduce with several combinations,
even though I wonder if it isn't just more robust this way (given 2/2).

-- 
Stefano