netdev - Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

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: <CANn89iLiDnDfoeuEE-AsbG_bsU5Ojt9VQcZ53FmEOStT9_fj6A@mail.gmail.com>
Date:   Fri, 3 Jan 2020 04:14:10 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Ttttabcd <ttttabcd@...tonmail.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        "kuznet@....inr.ac.ru" <kuznet@....inr.ac.ru>,
        "yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH] tcp: Fix tcp_max_syn_backlog limit on connection requests

On Mon, Dec 30, 2019 at 5:21 PM Ttttabcd <ttttabcd@...tonmail.com> wrote:
>
> In the original logic of tcp_conn_request, the backlog parameter of the
> listen system call and net.ipv4.tcp_max_syn_backlog are independent of
> each other, which causes some confusion in the processing.
>
> The backlog determines the maximum length of request_sock_queue, hereafter
> referred to as backlog.
>
> In the original design, if syn_cookies is not turned on, a quarter of
> tcp_max_syn_backlog will be reserved for clients that have proven to
> exist, mitigating syn attacks.
>
> Suppose now that tcp_max_syn_backlog is 1000, but the backlog is only 200,
> then 1000 >> 2 = 250, the backlog is used up by the syn attack, and the
> above mechanism will not work.
>
> Is tcp_max_syn_backlog used to limit the
> maximum length of request_sock_queue?
>
> Now suppose sycookie is enabled, backlog is 1000, and tcp_max_syn_backlog
> is only 200. In this case tcp_max_syn_backlog will be useless.
>
> Because syn_cookies is enabled, the tcp_max_syn_backlog logic will
> be ignored, and the length of request_sock_queue will exceed
> tcp_max_syn_backlog until the backlog.
>
> I modified the original logic and set the minimum value in backlog and
> tcp_max_syn_backlog as the maximum length limit of request_sock_queue.
>
> Now there is only a unified limit.
>
> The maximum length limit variable is "max_syn_backlog".
>
> Use syn_cookies whenever max_syn_backlog is exceeded.
>
> If syn_cookies is not enabled, a quarter of the max_syn_backlog queue is
> reserved for hosts that have proven to exist.
>
> In any case, request_sock_queue will not exceed max_syn_backlog.
> When syn_cookies is not turned on, a quarter of the queue retention
> will not be preempted.
>
> Signed-off-by: AK Deng <ttttabcd@...tonmail.com>
> ---
>  net/ipv4/tcp_input.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 88b987ca9ebb..8f7e12844ae7 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6562,14 +6562,18 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>         struct request_sock *req;
>         bool want_cookie = false;
>         struct dst_entry *dst;
> +       u32 max_syn_backlog;
>         struct flowi fl;
>
> +       max_syn_backlog = min_t(u32, net->ipv4.sysctl_max_syn_backlog,
> +                             sk->sk_max_ack_backlog);
> +
>         /* TW buckets are converted to open requests without
>          * limitations, they conserve resources and peer is
>          * evidently real one.
>          */
>         if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
> -            inet_csk_reqsk_queue_is_full(sk)) && !isn) {
> +            inet_csk_reqsk_queue_len(sk) >= max_syn_backlog) && !isn) {
>                 want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
>                 if (!want_cookie)
>                         goto drop;
> @@ -6621,8 +6625,8 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>         if (!want_cookie && !isn) {
>                 /* Kill the following clause, if you dislike this way. */
>                 if (!net->ipv4.sysctl_tcp_syncookies &&
> -                   (net->ipv4.sysctl_max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
> -                    (net->ipv4.sysctl_max_syn_backlog >> 2)) &&

Note that the prior condition is using signed arithmetic.


> +                   (max_syn_backlog - inet_csk_reqsk_queue_len(sk) <
> +                    (max_syn_backlog >> 2)) &&
>                     !tcp_peer_is_proven(req, dst)) {
>                         /* Without syncookies last quarter of
>                          * backlog is filled with destinations,
> --
> 2.24.0

I would prefer not changing this code, unless you prove there is a real problem.

(sysctl_max_syn_backlog defauts to 4096, and syncookies are enabled by
default, for a good reason)

Basically, sysctl_max_syn_backlog is not used today (because
syncookies are enabled...)

Your change might break users that suddenly will get behavior changes
if their sysctl_max_syn_backlog was set to a small value.
Unfortunately  some sysctl values are often copied/pasted from various
web pages claiming how to get best TCP performance.

It would be quite silly to change the kernel to adapt a change
(sysctl_max_syn_backlog set to 200 ... ) done by one of these admins.

Thanks.