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

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: <20241115191024.5bc07d74@elisabeth>
Date: Fri, 15 Nov 2024 19:10:24 +0100
From: Stefano Brivio <sbrivio@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org, David Gibson
 <david@...son.dropbear.id.au>, Ed Santiago <santiago@...hat.com>, Paul
 Holzinger <pholzing@...hat.com>, Mike Manning <mvrmanning@...il.com>
Subject: Re: [PATCH RFC net 1/2] datagram: Rehash sockets only if local
 address changed for their family

[Updated Mike Manning's address in Cc:]

On Fri, 15 Nov 2024 12:48:29 -0500
Willem de Bruijn <willemdebruijn.kernel@...il.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@...hat.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).

> 
> >  	}
> >  	inet->inet_daddr = fl4->daddr;
> > diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> > index fff78496803d..5c28a11128c7 100644
> > --- a/net/ipv6/datagram.c
> > +++ b/net/ipv6/datagram.c
> > @@ -211,7 +211,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
> >  		    ipv6_mapped_addr_any(&sk->sk_v6_rcv_saddr)) {
> >  			ipv6_addr_set_v4mapped(inet->inet_rcv_saddr,
> >  					       &sk->sk_v6_rcv_saddr);
> > -			if (sk->sk_prot->rehash)
> > +			if (sk->sk_prot->rehash && sk->sk_family == AF_INET6)
> >  				sk->sk_prot->rehash(sk);  
> 
> Even if this is a v4mappedv6 address, sk_family will be AF_INET6.

I think we could have a case that's symmetric to the one above, even
though I haven't reproduced this one (but I didn't try hard).

-- 
Stefano