Message-ID: <CANaxB-zPEvvVPmNeTPtkk0oG1uT_DXK5aBXye1gQ75GTMxYTdg@mail.gmail.com>
Date: Tue, 12 Sep 2023 07:29:27 -0700
From: Andrei Vagin <avagin@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, 
	joannelkoong@...il.com, kuba@...nel.org, kuni1840@...il.com, 
	netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH v2 net 3/6] tcp: Fix bind() regression for v4-mapped-v6
 non-wildcard address.

On Tue, Sep 12, 2023 at 12:59 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Andrei Vagin <avagin@...il.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@...zon.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@...zon.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@...zon.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.

Thanks for the explanation. It looks quite reasonable.

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

My mistake was to suppose that it was done unintentionally.  I didn't find any
explanations in commit messages and in the code. Sorry if I missed something.
I think a comment in the code could help to avoid such questions.

The patch looks good to me.
Acked-by: Andrei Vagin <avagin@...il.com>

Thanks,
Andrei

Powered by blists - more mailing lists