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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 28 Jul 2019 14:32:04 -0700 From: Josh Hunt <johunt@...mai.com> To: Eric Dumazet <edumazet@...gle.com> Cc: netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net> Subject: Re: [PATCH] tcp: add new tcp_mtu_probe_floor sysctl On 7/28/19 2:14 PM, Josh Hunt wrote: > On 7/28/19 6:54 AM, Eric Dumazet wrote: >> On Sun, Jul 28, 2019 at 1:21 AM Josh Hunt <johunt@...mai.com> wrote: >>> >>> On 7/27/19 12:05 AM, Eric Dumazet wrote: >>>> 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 ??? >>> >>> Reasonable for some may not be reasonable for others hence the new >>> sysctl :) We're currently running with a fairly high value based off of >>> the v6 min MTU minus headers and options, etc. We went conservative with >>> our setting initially as it seemed a reasonable first step when >>> re-enabling TCP MTU probing since with no configurable floor we saw a # >>> of cases where connections were using severely reduced mss b/c of loss >>> and not b/c of actual path restriction. I plan to reevaluate the setting >>> at some point, but since the probing method is still the same it means >>> the same clients who got stuck with mss of 48 before will land at >>> whatever floor we set. Looking forward we are interested in trying to >>> improve TCP MTU probing so it does not penalize clients like this. >>> >>> A suggestion for a more reasonable floor default would be 512, which is >>> the same as the min_pmtu. Given both mechanisms are trying to achieve >>> the same goal it seems like they should have a similar min/floor. >>> >>>> >>>>> >>>>> 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 ? >>> >>> I don't think so. Changing tcp_min_snd_mss could impact clients that >>> really want/need a small mss. When you added the new sysctl I tried to >>> analyze the mss values we're seeing to understand what we could possibly >>> raise it to. While not a huge amount, we see more clients than I >>> expected announcing mss values in the 180-512 range. Given that I would >>> not feel comfortable setting tcp_min_snd_mss to say 512 as I suggested >>> above. >> >> If these clients need mss values in 180-512 ranges, how MTU probing >> would work for them, >> if you set a floor to 512 ? > > First, we already seem to be fine with ignoring these paths with ICMP > based PMTU discovery b/c of our min_pmtu default of 512 and that is > configurable. Second by adding this sysctl we're giving administrators > the choice to decide if they'd like to attempt to support these very > very small # of paths which may be below 512 (MSS <= 512 does not mean > MTU <= 512) or cover themselves by being able to raise the floor to not > penalize clients who may be on very lossy networks. > >> >> Are we sure the intent of tcp_base_mss was not to act as a floor ? > > My understanding is that tcp_base_mss is meant to be the initial value > of search_low (as per Docs). Then in RFC 4821 [1] Sections 7.2, shows > search_low should be configurable, and 7.7 we see that in response to > successive black hole detection search_low should be halved. So I don't > think it was meant to be a floor, but just the initial search_low param. > Also note that in that same section they suggest a floor of 68 for v4, > but a floor of 1280 for v6 which we do not adhere to currently. > Clarification. We == Akamai in regards to setting tcp_base_mss to 1400-overheads. Upstream default is 1024. > We actually set tcp_base_mss to something close to the value suggested > towards the end of section 7.2 of the RFC of 1400 bytes minus IP and > Transport overheads and options. This way we have more realistic > searching based on the majority of clients that we see. The kernel winds > up using initial search_low/tcp_base_mss as initial eff_pmtu, so we see > something like: > > 21:03:41.314612 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:41.670307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:42.030308 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:42.534307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:43.198308 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:44.478307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq > 1:1461, ack 1, win 229, length 1460: HTTP > 21:03:47.742310 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [.], seq > 1:1349, ack 1, win 229, length 1348: HTTP > 21:03:56.702310 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [.], seq > 1:675, ack 1, win 229, length 674: HTTP > > For further evidence this is a real problem here's a sample of mss > values I found when originally investigating this problem for us: > > I dug up some #s I found when originally investigating this problem: > > # ss -emoitn | grep mss | sed "s/.*mss:\([0-9]*\).*/\1/" | sort -u | > sort -g | head -5 > > 36:11 > 64:7 > 72:1 > 128:13 > 144:4 > > From what I could tell these connections were on paths much larger than > the mss they were being forced to use. I determined this by looking at > the mss used for other objects fetched from the same IPs. > > Josh > > [1] - https://www.ietf.org/rfc/rfc4821.txt > >> >> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c >> index >> c801cd37cc2a9c11f2dd4b9681137755e501a538..6d15895e9dcfb2eff51bbcf3608c7e68c1970a9e >> >> 100644 >> --- a/net/ipv4/tcp_timer.c >> +++ b/net/ipv4/tcp_timer.c >> @@ -153,7 +153,7 @@ static void tcp_mtu_probing(struct >> inet_connection_sock *icsk, struct sock *sk) >> icsk->icsk_mtup.probe_timestamp = tcp_jiffies32; >> } else { >> mss = tcp_mtu_to_mss(sk, icsk->icsk_mtup.search_low) >> >> 1; >> - mss = min(net->ipv4.sysctl_tcp_base_mss, mss); >> + mss = max(net->ipv4.sysctl_tcp_base_mss, mss); >> mss = max(mss, 68 - tcp_sk(sk)->tcp_header_len); >> mss = max(mss, net->ipv4.sysctl_tcp_min_snd_mss); >> icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, mss); >> >> >> >>> >>>> >>>> tcp_min_snd_mss documentation could be slightly updated. >>>> >>>> And maybe its default value could be raised a bit. >>>> >>> >>> Thanks >>> Josh
Powered by blists - more mailing lists