Re: [PATCH v2 net 3/6] tcp: Fix bind() regression for v4-mapped-v6 non-wildcard address.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Andrei Vagin <avagin@gmail.com>
Date: Tue, 12 Sep 2023 00:25:58 -0700
> On Mon, Sep 11, 2023 at 11:36:57AM -0700, Kuniyuki Iwashima wrote:
> > Since bhash2 was introduced, the example below does not work as expected.
> > These two bind() should conflict, but the 2nd bind() now succeeds.
> > 
> >   from socket import *
> > 
> >   s1 = socket(AF_INET6, SOCK_STREAM)
> >   s1.bind(('::ffff:127.0.0.1', 0))
> > 
> >   s2 = socket(AF_INET, SOCK_STREAM)
> >   s2.bind(('127.0.0.1', s1.getsockname()[1]))
> > 
> > During the 2nd bind() in inet_csk_get_port(), inet_bind2_bucket_find()
> > fails to find the 1st socket's tb2, so inet_bind2_bucket_create() allocates
> > a new tb2 for the 2nd socket.  Then, we call inet_csk_bind_conflict() that
> > checks conflicts in the new tb2 by inet_bhash2_conflict().  However, the
> > new tb2 does not include the 1st socket, thus the bind() finally succeeds.
> > 
> > In this case, inet_bind2_bucket_match() must check if AF_INET6 tb2 has
> > the conflicting v4-mapped-v6 address so that inet_bind2_bucket_find()
> > returns the 1st socket's tb2.
> > 
> > Note that if we bind two sockets to 127.0.0.1 and then ::FFFF:127.0.0.1,
> > the 2nd bind() fails properly for the same reason mentinoed in the previous
> > commit.
> > 
> > Fixes: 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  net/ipv4/inet_hashtables.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> > index a58b04052ca6..c32f5e28758b 100644
> > --- a/net/ipv4/inet_hashtables.c
> > +++ b/net/ipv4/inet_hashtables.c
> > @@ -820,8 +820,13 @@ static bool inet_bind2_bucket_match(const struct inet_bind2_bucket *tb,
> 
> Should we fix inet_bind2_bucket_addr_match too?

No, there's no real bug.

I have this patch in my local branch and will post it against
net-next after this series is merged.

---8<---
commit 06333d4b0d053e4c0d40090b29e5a8546b42bb50
Author: Kuniyuki Iwashima <kuniyu@amazon.com>
Date:   Sun Sep 10 19:01:23 2023 +0000

    tcp: Remove redundant sk_family check in inet_bind2_bucket_addr_match().

    Commit 5456262d2baa ("net: Fix incorrect address comparison when
    searching for a bind2 bucket") added the test for the KMSAN report.

    However, the condition never be true as tb2 is listener's
    inet_csk(sk)->icsk_bind2_hash and its sk_family always matches with
    child->sk_family.

    Link: https://lore.kernel.org/netdev/CAG_fn=Ud3zSW7AZWXc+asfMhZVL5ETnvuY44Pmyv4NPv-ijN-A@mail.gmail.com/
    Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index c32f5e28758b..dfb1c61c0c2b 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -149,9 +149,6 @@ static bool inet_bind2_bucket_addr_match(const struct inet_bind2_bucket *tb2,
 					 const struct sock *sk)
 {
 #if IS_ENABLED(CONFIG_IPV6)
-	if (sk->sk_family != tb2->family)
-		return false;
-
 	if (sk->sk_family == AF_INET6)
 		return ipv6_addr_equal(&tb2->v6_rcv_saddr,
 				       &sk->sk_v6_rcv_saddr);
---8<---


> 
> >  		return false;
> >  
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -	if (sk->sk_family != tb->family)
> > +	if (sk->sk_family != tb->family) {
> > +		if (sk->sk_family == AF_INET)
> > +			return ipv6_addr_v4mapped(&tb->v6_rcv_saddr) &&
> > +				tb->v6_rcv_saddr.s6_addr32[3] == sk->sk_rcv_saddr;
> 
> I was wondering why we don't check a case when sk is AF_INET6 and tb is
> AF_INET. I tried to run the next test:
> 
> import socket
> sk4 = socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0)
> sk6 = socket.socket(socket.AF_INET6, socket.SOCK_STREAM, 0)
> sk4.bind(("127.0.0.1", 32773))
> sk6.bind(("::ffff:127.0.0.1", 32773))
> 
> The second bind returned EADDRINUSE. It works as expected only because
> inet_use_bhash2_on_bind returns false for all v4mapped addresses. This
> doesn't look right, and I am not sure it was intentional. I think it can
> to be changed this way:
> 
> @@ -158,7 +158,7 @@ static bool inet_use_bhash2_on_bind(const struct sock *sk)
>                 int addr_type = ipv6_addr_type(&sk->sk_v6_rcv_saddr);
> 
>                 return addr_type != IPV6_ADDR_ANY &&
> -                       addr_type != IPV6_ADDR_MAPPED;
> +                       !ipv6_addr_v4mapped_any(&sk->sk_v6_rcv_saddr);
>         }
>  #endif
>         return sk->sk_rcv_saddr != htonl(INADDR_ANY);
> 
> As for this patch, I think it may be a good idea if bind2 buckets for
> v4-mapped addresses are created with the AF_INET family and matching
> ipv4 addresses.

Let's say we create tb2 with AF_INET for v4-mapped address.  If we bind
::ffff:127.0.0.1 and 127.0.0.1, in the second bind(), both tb->family and
sk->sk_family is AF_INET.  So, we can remove this AF_INET test.

  if (sk->sk_family != tb->family) {
      if (sk->sk_family == AF_INET)

But what about 127.0.0.1 and then ::ffff:127.0.0.1 ?  There tb->family is
AF_INET and sk->sk_family is AF_INET6.  We need to add another AF_INET6
test in the same place.

So, finally we need to check the special case in either way.

Also, as you may notice, we need to change inet_bind2_bucket_addr_match()
as well.  As mentioned in my patch above, sk->sk_family always match
tb2->family there, but v4-mapped AF_INET tb2 breaks the rule.

Using bhash2 for v4-mapped-v6 address could be done but churns code a lot.
So, I think we should not include such changes as fix at least.


> 
> > +
> >  		return false;
> > +	}
> >  
> >  	if (sk->sk_family == AF_INET6)
> >  		return ipv6_addr_equal(&tb->v6_rcv_saddr, &sk->sk_v6_rcv_saddr);