Re: [PATCH v1 net 1/5] tcp: Fix bind() regression for v4-mapped-v6 wildcard address.

[Andrei Vagin <avagin@google.com>]

On Mon, Sep 11, 2023 at 9:52 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Andrei Vagin reported bind() regression with strace logs.
>
> If we bind() a TCPv6 socket to ::FFFF:0.0.0.0 and then bind() a TCPv4
> socket to 127.0.0.1, the 2nd bind() should fail but now succeeds.
>
>   from socket import *
>
>   s1 = socket(AF_INET6, SOCK_STREAM)
>   s1.bind(('::ffff:0.0.0.0', 0))
>
>   s2 = socket(AF_INET, SOCK_STREAM)
>   s2.bind(('127.0.0.1', s1.getsockname()[1]))
>
> During the 2nd bind(), if tb->family is AF_INET6 and sk->sk_family is
> AF_INET in inet_bind2_bucket_match_addr_any(), we still need to check
> if tb has the v4-mapped-v6 wildcard address.
>
> The example above does not work after commit 5456262d2baa ("net: Fix
> incorrect address comparison when searching for a bind2 bucket"), but
> the blamed change is not the commit.
>
> Before the commit, the leading zeros of ::FFFF:0.0.0.0 were treated
> as 0.0.0.0, and the sequence above worked by chance.  Technically, this
> case has been broken since bhash2 was introduced.
>
> Note that if we bind() two sockets to 127.0.0.1 and then ::FFFF:0.0.0.0,
> the 2nd bind() fails properly because we fall back to using bhash to
> detect conflicts for the v4-mapped-v6 address.

I think we have one more issue here:

socket(AF_INET6, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_IP) = 3
socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_IP) = 4
bind(3, {sa_family=AF_INET6, sin6_port=htons(9999),
sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:127.0.0.1",
&sin6_addr), sin6_scope_id=0}, 28) = 0
bind(4, {sa_family=AF_INET, sin_port=htons(9999),
sin_addr=inet_addr("127.0.0.1")}, 16) = 0

I think the second bind has to return EADDRINUSE, doesn't it?

I don't deep dive to this code, but after a quick look, I think we can
do something like this:

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 7876b7d703cb..f7a700d392d0 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -110,8 +110,13 @@ static void inet_bind2_bucket_init(struct
inet_bind2_bucket *tb,
        tb->l3mdev    = l3mdev;
        tb->port      = port;
 #if IS_ENABLED(CONFIG_IPV6)
-       tb->family    = sk->sk_family;
-       if (sk->sk_family == AF_INET6)
+
+       if (sk->sk_family == AF_INET6 &&
+           ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr))
+               tb->family = AF_INET;
+       else
+               tb->family = sk->sk_family;
+       if (tb->family == AF_INET6)
                tb->v6_rcv_saddr = sk->sk_v6_rcv_saddr;
        else
 #endif
@@ -149,10 +154,15 @@ 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)
+       unsigned short family = sk->sk_family;
+
+       if (ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr))
+               family = AF_INET;
+
+       if (family != tb2->family)
                return false;

-       if (sk->sk_family == AF_INET6)
+       if (family == AF_INET6)
                return ipv6_addr_equal(&tb2->v6_rcv_saddr,
                                       &sk->sk_v6_rcv_saddr);
 #endif
@@ -816,10 +826,15 @@ static bool inet_bind2_bucket_match(const struct
inet_bind2_bucket *tb,
                                    int l3mdev, const struct sock *sk)
 {
 #if IS_ENABLED(CONFIG_IPV6)
-       if (sk->sk_family != tb->family)
+       unsigned short family = sk->sk_family;
+
+       if (ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr))
+               family = AF_INET;
+
+       if (family != tb->family)
                return false;

-       if (sk->sk_family == AF_INET6)
+       if (family == AF_INET6)
                return net_eq(ib2_net(tb), net) && tb->port == port &&
                        tb->l3mdev == l3mdev &&
                        ipv6_addr_equal(&tb->v6_rcv_saddr,
&sk->sk_v6_rcv_saddr);



>
> Fixes: 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")
> Reported-by: Andrei Vagin <avagin@google.com>
> Closes: https://lore.kernel.org/netdev/ZPuYBOFC8zsK6r9T@google.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  include/net/ipv6.h         | 5 +++++
>  net/ipv4/inet_hashtables.c | 3 ++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 0675be0f3fa0..56d8217ea6cf 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -784,6 +784,11 @@ static inline bool ipv6_addr_v4mapped(const struct in6_addr *a)
>                                         cpu_to_be32(0x0000ffff))) == 0UL;
>  }
>
> +static inline bool ipv6_addr_v4mapped_any(const struct in6_addr *a)
> +{
> +       return ipv6_addr_v4mapped(a) && ipv4_is_zeronet(a->s6_addr32[3]);
> +}
> +
>  static inline bool ipv6_addr_v4mapped_loopback(const struct in6_addr *a)
>  {
>         return ipv6_addr_v4mapped(a) && ipv4_is_loopback(a->s6_addr32[3]);
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 7876b7d703cb..0a9b20eb81c4 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -837,7 +837,8 @@ bool inet_bind2_bucket_match_addr_any(const struct inet_bind2_bucket *tb, const
>                 if (sk->sk_family == AF_INET)
>                         return net_eq(ib2_net(tb), net) && tb->port == port &&
>                                 tb->l3mdev == l3mdev &&
> -                               ipv6_addr_any(&tb->v6_rcv_saddr);
> +                               (ipv6_addr_any(&tb->v6_rcv_saddr) ||
> +                                ipv6_addr_v4mapped_any(&tb->v6_rcv_saddr));
>
>                 return false;
>         }
> --
> 2.30.2
>