netdev - Re: [PATCH v3 net-next] inet: Remove an unnecessary argument of syn_ack

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] [day] [month] [year] [list]

Message-ID: <20200710154701.84658-1-kuniyu@amazon.co.jp>
Date:   Sat, 11 Jul 2020 00:47:01 +0900
From:   Kuniyuki Iwashima <kuniyu@...zon.co.jp>
To:     <edumazet@...gle.com>
CC:     <benh@...zon.com>, <davem@...emloft.net>, <ja@....bg>,
        <kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.co.jp>,
        <kuznet@....inr.ac.ru>, <netdev@...r.kernel.org>,
        <osa-contribution-log@...zon.com>, <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH v3 net-next] inet: Remove an unnecessary argument of syn_ack_recalc().

From:   Eric Dumazet <edumazet@...gle.com>
Date:   Fri, 10 Jul 2020 08:20:03 -0700
> On Fri, Jul 10, 2020 at 7:11 AM Kuniyuki Iwashima <kuniyu@...zon.co.jp> wrote:
> >
> > Commit 0c3d79bce48034018e840468ac5a642894a521a3 ("tcp: reduce SYN-ACK
> > retrans for TCP_DEFER_ACCEPT") introduces syn_ack_recalc() which decides
> > if a minisock is held and a SYN+ACK is retransmitted or not.
> >
> > If rskq_defer_accept is not zero in syn_ack_recalc(), max_retries always
> > has the same value because max_retries is overwritten by rskq_defer_accept
> > in reqsk_timer_handler().
> >
> > This commit adds three changes:
> > - remove redundant non-zero check for rskq_defer_accept in
> >    reqsk_timer_handler().
> > - remove max_retries from the arguments of syn_ack_recalc() and use
> >    rskq_defer_accept instead.
> > - rename thresh to max_syn_ack_retries for readability.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.co.jp>
> > Reviewed-by: Benjamin Herrenschmidt <benh@...zon.com>
> > CC: Julian Anastasov <ja@....bg>
> > ---
> >  net/ipv4/inet_connection_sock.c | 33 +++++++++++++++------------------
> >  1 file changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index afaf582a5aa9..21bc80a3c7cf 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -648,20 +648,23 @@ struct dst_entry *inet_csk_route_child_sock(const struct sock *sk,
> >  EXPORT_SYMBOL_GPL(inet_csk_route_child_sock);
> >
> >  /* Decide when to expire the request and when to resend SYN-ACK */
> > -static inline void syn_ack_recalc(struct request_sock *req, const int thresh,
> > -                                 const int max_retries,
> > +static inline void syn_ack_recalc(struct request_sock *req,
> 
> While we are at it, please remove the inline keyword.

I will remove 'inline' in next spin.


> > +                                 const int max_syn_ack_retries,
> >                                   const u8 rskq_defer_accept,
> >                                   int *expire, int *resend)
> >  {
> >         if (!rskq_defer_accept) {
> > -               *expire = req->num_timeout >= thresh;
> > +               *expire = req->num_timeout >= max_syn_ack_retries;
> >                 *resend = 1;
> >                 return;
> >         }
> > -       *expire = req->num_timeout >= thresh &&
> > -                 (!inet_rsk(req)->acked || req->num_timeout >= max_retries);
> > -       /*
> > -        * Do not resend while waiting for data after ACK,
> > +       /* If a bare ACK has already been dropped, the client is alive, so
> > +        * do not free the request_sock to drop a bare ACK at most
> > +        * rskq_defer_accept times and wait for data.
> > +        */
> 
> I honestly do not believe this comment is needed.
> The bare ack has not been 'dropped' since it had the effect of
> validating the 3WHS.
> I find it confusing, and not describing the order of the conditions
> expressed in the C code.

Exactly, thank you for correcting my misunderstanding.
I will remove the comment.

Best Regards,
Kuniyuki