netdev - Re: [PATCH] tcp: add new tcp_mtu_probe

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: <CANn89iJtw_XrU-F0-frE=P6egH99kF0W0kTzReK701LmigcJ4Q@mail.gmail.com>
Date:   Sat, 27 Jul 2019 09:05:39 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Josh Hunt <johunt@...mai.com>
Cc:     netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] tcp: add new tcp_mtu_probe_floor sysctl

On Sat, Jul 27, 2019 at 4:23 AM Josh Hunt <johunt@...mai.com> wrote:
>
> The current implementation of TCP MTU probing can considerably
> underestimate the MTU on lossy connections allowing the MSS to get down to
> 48. We have found that in almost all of these cases on our networks these
> paths can handle much larger MTUs meaning the connections are being
> artificially limited. Even though TCP MTU probing can raise the MSS back up
> we have seen this not to be the case causing connections to be "stuck" with
> an MSS of 48 when heavy loss is present.
>
> Prior to pushing out this change we could not keep TCP MTU probing enabled
> b/c of the above reasons. Now with a reasonble floor set we've had it
> enabled for the past 6 months.

And what reasonable value have you used ???

>
> The new sysctl will still default to TCP_MIN_SND_MSS (48), but gives
> administrators the ability to control the floor of MSS probing.
>
> Signed-off-by: Josh Hunt <johunt@...mai.com>
> ---
>  Documentation/networking/ip-sysctl.txt | 6 ++++++
>  include/net/netns/ipv4.h               | 1 +
>  net/ipv4/sysctl_net_ipv4.c             | 9 +++++++++
>  net/ipv4/tcp_ipv4.c                    | 1 +
>  net/ipv4/tcp_timer.c                   | 2 +-
>  5 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> index df33674799b5..49e95f438ed7 100644
> --- a/Documentation/networking/ip-sysctl.txt
> +++ b/Documentation/networking/ip-sysctl.txt
> @@ -256,6 +256,12 @@ tcp_base_mss - INTEGER
>         Path MTU discovery (MTU probing).  If MTU probing is enabled,
>         this is the initial MSS used by the connection.
>
> +tcp_mtu_probe_floor - INTEGER
> +       If MTU probing is enabled this caps the minimum MSS used for search_low
> +       for the connection.
> +
> +       Default : 48
> +
>  tcp_min_snd_mss - INTEGER
>         TCP SYN and SYNACK messages usually advertise an ADVMSS option,
>         as described in RFC 1122 and RFC 6691.
> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
> index bc24a8ec1ce5..c0c0791b1912 100644
> --- a/include/net/netns/ipv4.h
> +++ b/include/net/netns/ipv4.h
> @@ -116,6 +116,7 @@ struct netns_ipv4 {
>         int sysctl_tcp_l3mdev_accept;
>  #endif
>         int sysctl_tcp_mtu_probing;
> +       int sysctl_tcp_mtu_probe_floor;
>         int sysctl_tcp_base_mss;
>         int sysctl_tcp_min_snd_mss;
>         int sysctl_tcp_probe_threshold;
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 0b980e841927..59ded25acd04 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -820,6 +820,15 @@ static struct ctl_table ipv4_net_table[] = {
>                 .extra2         = &tcp_min_snd_mss_max,
>         },
>         {
> +               .procname       = "tcp_mtu_probe_floor",
> +               .data           = &init_net.ipv4.sysctl_tcp_mtu_probe_floor,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = &tcp_min_snd_mss_min,
> +               .extra2         = &tcp_min_snd_mss_max,
> +       },
> +       {
>                 .procname       = "tcp_probe_threshold",
>                 .data           = &init_net.ipv4.sysctl_tcp_probe_threshold,
>                 .maxlen         = sizeof(int),
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index d57641cb3477..e0a372676329 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -2637,6 +2637,7 @@ static int __net_init tcp_sk_init(struct net *net)
>         net->ipv4.sysctl_tcp_min_snd_mss = TCP_MIN_SND_MSS;
>         net->ipv4.sysctl_tcp_probe_threshold = TCP_PROBE_THRESHOLD;
>         net->ipv4.sysctl_tcp_probe_interval = TCP_PROBE_INTERVAL;
> +       net->ipv4.sysctl_tcp_mtu_probe_floor = TCP_MIN_SND_MSS;
>
>         net->ipv4.sysctl_tcp_keepalive_time = TCP_KEEPALIVE_TIME;
>         net->ipv4.sysctl_tcp_keepalive_probes = TCP_KEEPALIVE_PROBES;
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index c801cd37cc2a..dbd9d2d0ee63 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -154,7 +154,7 @@ static void tcp_mtu_probing(struct inet_connection_sock *icsk, struct sock *sk)
>         } else {
>                 mss = tcp_mtu_to_mss(sk, icsk->icsk_mtup.search_low) >> 1;
>                 mss = min(net->ipv4.sysctl_tcp_base_mss, mss);
> -               mss = max(mss, 68 - tcp_sk(sk)->tcp_header_len);
> +               mss = max(mss, net->ipv4.sysctl_tcp_mtu_probe_floor);
>                 mss = max(mss, net->ipv4.sysctl_tcp_min_snd_mss);
>                 icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, mss);
>         }


Existing sysctl should be enough ?

tcp_min_snd_mss  documentation could be slightly updated.

And maybe its default value could be raised a bit.