netdev - Re: [PATCH RFC net 1/2] tcp: Add TIME

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: <99793af4ce55ba878bc201571c824a17a37ef81d.camel@redhat.com>
Date:   Fri, 23 Dec 2022 11:15:24 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>, joannelkoong@...il.com
Cc:     davem@...emloft.net, edumazet@...gle.com, jirislaby@...nel.org,
        kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net 1/2] tcp: Add TIME_WAIT sockets in bhash2.

On Fri, 2022-12-23 at 08:26 +0900, Kuniyuki Iwashima wrote:
> From:   Joanne Koong <joannelkoong@...il.com>
> Date:   Thu, 22 Dec 2022 13:46:57 -0800
> > On Thu, Dec 22, 2022 at 7:06 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > > 
> > > On Thu, 2022-12-22 at 00:12 +0900, Kuniyuki Iwashima wrote:
> > > > Jiri Slaby reported regression of bind() with a simple repro. [0]
> > > > 
> > > > The repro creates a TIME_WAIT socket and tries to bind() a new socket
> > > > with the same local address and port.  Before commit 28044fc1d495 ("net:
> > > > Add a bhash2 table hashed by port and address"), the bind() failed with
> > > > -EADDRINUSE, but now it succeeds.
> > > > 
> > > > The cited commit should have put TIME_WAIT sockets into bhash2; otherwise,
> > > > inet_bhash2_conflict() misses TIME_WAIT sockets when validating bind()
> > > > requests if the address is not a wildcard one.
> > 
> > (resending my reply because it wasn't in plaintext mode)
> > 
> > Thanks for adding this! I hadn't realized TIME_WAIT sockets also are
> > considered when checking against inet bind conflicts.
> > 
> > > 
> > > How does keeping the timewait sockets inside bhash2 affect the bind
> > > loopup performance? I fear that could defeat completely the goal of
> > > 28044fc1d495, on quite busy server we could have quite a bit of tw with
> > > the same address/port. If so, we could even consider reverting
> > > 28044fc1d495.
> 
> It will slow down along the number of twsk, but I think it's still faster
> than bhash if we listen() on multiple IP.  If we don't, bhash is always
> faster because of bhash2's additional locking.  However, this is the
> nature of bhash2 from the beginning.

I see. Before 28044fc1d495, todo a bind lookup, we had to traverse all
the tw in bhash. After 28044fc1d495, tw were ignored (in some cases).
My point is: if the number of tw sockets is >> the number of listeners
on multiple IPs, the bhash2 traversal time after this patch will be
comparable to the bhash traversal time before 28044fc1d495, with the
added cost of the double spin lock.

> > 
> > Can you clarify what you mean by bind loopup?
> 
> I think it means just bhash2 traversal.  (s/loopup/lookup/)

Indeed, sorry for the typo!
> 
> > 
> > > > [0]: https://lore.kernel.org/netdev/6b971a4e-c7d8-411e-1f92-fda29b5b2fb9@kernel.org/
> > > > 
> > > > Fixes: 28044fc1d495 ("net: Add a bhash2 table hashed by port and address")
> > > > Reported-by: Jiri Slaby <jirislaby@...nel.org>
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > > ---
> > > >  include/net/inet_timewait_sock.h |  2 ++
> > > >  include/net/sock.h               |  5 +++--
> > > >  net/ipv4/inet_hashtables.c       |  5 +++--
> > > >  net/ipv4/inet_timewait_sock.c    | 31 +++++++++++++++++++++++++++++--
> > > >  4 files changed, 37 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h
> > > > index 5b47545f22d3..c46ed239ad9a 100644
> > > > --- a/include/net/inet_timewait_sock.h
> > > > +++ b/include/net/inet_timewait_sock.h
> > > > @@ -44,6 +44,7 @@ struct inet_timewait_sock {
> > > >  #define tw_bound_dev_if              __tw_common.skc_bound_dev_if
> > > >  #define tw_node                      __tw_common.skc_nulls_node
> > > >  #define tw_bind_node         __tw_common.skc_bind_node
> > > > +#define tw_bind2_node                __tw_common.skc_bind2_node
> > > >  #define tw_refcnt            __tw_common.skc_refcnt
> > > >  #define tw_hash                      __tw_common.skc_hash
> > > >  #define tw_prot                      __tw_common.skc_prot
> > > > @@ -73,6 +74,7 @@ struct inet_timewait_sock {
> > > >       u32                     tw_priority;
> > > >       struct timer_list       tw_timer;
> > > >       struct inet_bind_bucket *tw_tb;
> > > > +     struct inet_bind2_bucket        *tw_tb2;
> > > >  };
> > > >  #define tw_tclass tw_tos
> > > > 
> > > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > > index dcd72e6285b2..aaec985c1b5b 100644
> > > > --- a/include/net/sock.h
> > > > +++ b/include/net/sock.h
> > > > @@ -156,6 +156,7 @@ typedef __u64 __bitwise __addrpair;
> > > >   *   @skc_tw_rcv_nxt: (aka tw_rcv_nxt) TCP window next expected seq number
> > > >   *           [union with @skc_incoming_cpu]
> > > >   *   @skc_refcnt: reference count
> > > > + *   @skc_bind2_node: bind node in the bhash2 table
> > > >   *
> > > >   *   This is the minimal network layer representation of sockets, the header
> > > >   *   for struct sock and struct inet_timewait_sock.
> > > > @@ -241,6 +242,7 @@ struct sock_common {
> > > >               u32             skc_window_clamp;
> > > >               u32             skc_tw_snd_nxt; /* struct tcp_timewait_sock */
> > > >       };
> > > > +     struct hlist_node       skc_bind2_node;
> > > 
> > > I *think* it would be better adding a tw_bind2_node field to the
> > > inet_timewait_sock struct, so that we leave unmodified the request
> > > socket and we don't change the struct sock binary layout. That could
> > > affect performances moving hot fields on different cachelines.
> > > 
> > +1. The rest of this patch LGTM.
> 
> Then we can't use sk_for_each_bound_bhash2(), or we have to guarantee this.
> 
>   BUILD_BUG_ON(offsetof(struct sock, sk_bind2_node),
>                offsetof(struct inet_timewait_sock, tw_bind2_node))
> 
> Considering the number of members in struct sock, at least we have
> to move sk_bind2_node forward.

You are right, I missed that point.


> Another option is to have another TIME_WAIT list in inet_bind2_bucket like
> tb2->deathrow or something.  sk_for_each_bound_bhash2() is used only in
> inet_bhash2_conflict(), so I think this is feasible.

This will add some more memory cost. I hope it's not a big deal, and
looks like the better option IMHO.

Cheers,

Paolo