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

[Andrei Vagin <avagin@gmail.com>]

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?

>  		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.

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