| Message ID | 20220210162632.3309974-2-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v4,1/8] Replace TUN_MTU_SIZE with frame->tun_mtu | expand |
Hi, On Thu, Feb 10, 2022 at 05:26:26PM +0100, Arne Schwabe wrote: > The current default is 1450, which translates to 1478 byte packets for udp4 > and 1498 byte packets for udp6. This commit changes the mssfix default > to take the outer IP overhead into account as well and changes the target to > 1492. 1492 was picked in our community meeting for being a very common > encapsulation upper bound. > > The change also disables an mssfix default if tun-mtu is set to a value > different than 1500. I think this needs a followup patch... I'll apply it (as it does what it says on the lid), but it needs further work, see below. Feature-ACK on having "1492 mtu" by default, so that is good. It should have a Changes.rst entry for "User-visible Changes" - I tried to draft something, but then decided to send it back via the list. Here's my draft text - :code:`--mssfix` default has been changed from 1450 to ``1492 mtu`` to take IPv4 or IPv6 encap and today's typical SoHo internet links into account. If :code:`--tun-mtu` is changed from the default setting, the default for :code:`--mssfix` is now ``off`` Also, the patch needs to change the manpage from "Default value of 1450 allows ..." to "Default value of ``1492 mtu`` allows packets to be transmitted over a link with MTU 1492 or higher without IP level fragmentation. If :code:`tun-mtu` is used to set a value != 1500, mssfix needs to be configured with an explicit value, as no default applies." (or such) The code itself looks a bit fumbly with changing "o->ce.mssfix_encap = true" in the defaults section, just to change it back to "false" in the options.c handler - why not leave it at false, as it's set to "true" anyway at setting MSSFIX_DEFAULT? But my main concern is this combination of options: --tun_mtu 1400 --mssfix what would the user expect OpenVPN to do here? I would expect "apply mssfix handling, in a reasonable fashion for the configured tun_mtu", but what OpenVPN does is "turn off mssfix, because, not 1500". So the default "no mssfix in the config" and "mssfix without arguments" are handled the same way. If this is intentional ("mssfix without arguments does nothing if the tun_mtu is not 1500") it should be documented. This said, the patch applies and works fine. gert
Acked-by: Gert Doering <gert@greenie.muc.de> The code does what it says on the lid, and the resulting program run confirms it: 2022-02-11 10:43:41 us=304654 mssfix = 1492 2022-02-11 10:43:41 us=304660 mssfix_encap = ENABLED I have not actually tested the resulting SYN/SYN-ACK values for IPv4 transport, as the other end of my test setup is on an older code base and would interfere with the tests (both sides modify MSS by default, in both directions, and the one with the lower MSS value "wins"). I have tested "mssfix ... mtu" before, so I'm reasonably sure it will do the right thing for "1492 mtu" now. It does the right thing, for IPv6, though :-) - IPv4 over IPv6, BF-CBC -> mss 1362 -> UDP payload 1440, IPv6 len 1488 - IPv6 over IPv6, BF-CBC -> mss 1342 -> UDP payload 1440, IPv6 len 1488 - IPv6 over IPv4, AES-GCM -> mss 1379 -> UDP payload 1444, IPv6 len 1492 - IPv6 over IPv6, AES-GCM -> mss 1359 -> UDP payload 1444, IPv6 len 1492 For reference, this was 07/14 in v3 of the patchset, and was neither ACKed nor NAKed there (I had the same concerns as I voiced in my preceding e-mail here, but did only mention them on IRC). Your patch has been applied to the master branch. commit 0d86da32695539a96848b96149484af41bba83c5 Author: Arne Schwabe Date: Thu Feb 10 17:26:26 2022 +0100 Change the default for mssfix to mssfix 1492 mtu Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20220210162632.3309974-2-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23754.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
Am 11.02.22 um 10:44 schrieb Gert Doering: > Hi, > > On Thu, Feb 10, 2022 at 05:26:26PM +0100, Arne Schwabe wrote: >> The current default is 1450, which translates to 1478 byte packets for udp4 >> and 1498 byte packets for udp6. This commit changes the mssfix default >> to take the outer IP overhead into account as well and changes the target to >> 1492. 1492 was picked in our community meeting for being a very common >> encapsulation upper bound. >> >> The change also disables an mssfix default if tun-mtu is set to a value >> different than 1500. > > I think this needs a followup patch... I'll apply it (as it does what > it says on the lid), but it needs further work, see below. > > > Feature-ACK on having "1492 mtu" by default, so that is good. > > It should have a Changes.rst entry for "User-visible Changes" - I tried > to draft something, but then decided to send it back via the list. Here's > my draft text > > - :code:`--mssfix` default has been changed from 1450 to ``1492 mtu`` to > take IPv4 or IPv6 encap and today's typical SoHo internet links into > account. If :code:`--tun-mtu` is changed from the default setting, > the default for :code:`--mssfix` is now ``off`` Sounds good. > > Also, the patch needs to change the manpage from > > "Default value of 1450 allows ..." > to > "Default value of ``1492 mtu`` allows packets to be transmitted > over a link with MTU 1492 or higher without IP level fragmentation. > If :code:`tun-mtu` is used to set a value != 1500, mssfix needs > to be configured with an explicit value, as no default applies." > (or such) > > > The code itself looks a bit fumbly with changing "o->ce.mssfix_encap = true" > in the defaults section, just to change it back to "false" in the options.c > handler - why not leave it at false, as it's set to "true" anyway at > setting MSSFIX_DEFAULT? Yeah, it is all a bit finnicky and I am not super happy with it. I change the default of it to false if you prefer that. > > > But my main concern is this combination of options: > > --tun_mtu 1400 --mssfix > > what would the user expect OpenVPN to do here? I would expect "apply > mssfix handling, in a reasonable fashion for the configured tun_mtu", > but what OpenVPN does is "turn off mssfix, because, not 1500". > > So the default "no mssfix in the config" and "mssfix without arguments" > are handled the same way. If this is intentional ("mssfix without > arguments does nothing if the tun_mtu is not 1500") it should be > documented. So my reason is that if you have a tun-mtu > 1500, that you want to had a reason and want use larger packet and then a mssfix would break whatever you are trying to do. And if you have a tun-tmu < 1500, then the smaller tun-mtu should already give you a MSS value that is small enough. If the MTU value is between something like 1450 and 1500, mssfix probably still does something but it felt to odd of a corner to still enable it. Arne
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h index 7a6cdcb4..3a8faec1 100644 --- a/src/openvpn/mtu.h +++ b/src/openvpn/mtu.h @@ -77,7 +77,7 @@ /* * Default MSSFIX value, used for reducing TCP MTU size */ -#define MSSFIX_DEFAULT 1450 +#define MSSFIX_DEFAULT 1492 /* * Alignment of payload data such as IP packet or diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 705f7e0c..491edbe5 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -803,7 +803,9 @@ init_options(struct options *o, const bool init_gc) o->ce.tun_mtu = TUN_MTU_DEFAULT; o->ce.link_mtu = LINK_MTU_DEFAULT; o->ce.mtu_discover_type = -1; - o->ce.mssfix = MSSFIX_DEFAULT; + o->ce.mssfix = 0; + o->ce.mssfix_default = true; + o->ce.mssfix_encap = true; o->route_delay_window = 30; o->resolve_retry_seconds = RESOLV_RETRY_INFINITE; o->resolve_in_advance = false; @@ -1511,6 +1513,7 @@ show_connection_entry(const struct connection_entry *o) SHOW_INT(fragment); #endif SHOW_INT(mssfix); + SHOW_BOOL(mssfix_encap); SHOW_INT(explicit_exit_notification); @@ -2887,22 +2890,6 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce) ce->flags |= CE_DISABLED; } - /* - * If --mssfix is supplied without a parameter, default - * it to --fragment value, if --fragment is specified. - */ - if (o->ce.mssfix_default) - { -#ifdef ENABLE_FRAGMENT - if (ce->fragment) - { - ce->mssfix = ce->fragment; - } -#else - msg(M_USAGE, "--mssfix must specify a parameter"); -#endif - } - /* our socks code is not fully IPv6 enabled yet (TCP works, UDP not) * so fall back to IPv4-only (trac #1221) */ @@ -2936,6 +2923,36 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce) } } + /* + * If --mssfix is supplied without a parameter or not specified at all, + * default it to --fragment value, if --fragment is specified and otherwise + * to the default if tun-mtu is 1500 + */ + if (o->ce.mssfix_default) + { +#ifdef ENABLE_FRAGMENT + if (ce->fragment) + { + ce->mssfix = ce->fragment; + } + else +#endif + if (ce->tun_mtu_defined && o->ce.tun_mtu == TUN_MTU_DEFAULT) + { + /* We want to only set mssfix default value if we use a default + * MTU Size, otherwise the different size of tun should either + * already solve the problem or mssfix might artifically make the + * payload packets smaller without mssfix 0 */ + ce->mssfix = MSSFIX_DEFAULT; + ce->mssfix_encap = true; + } + else + { + msg(D_MTU_INFO, "Note: not enabling mssfix for non-default value " + "of --tun-mtu"); + } + } + /* * Set per-connection block tls-auth/crypt/crypto-v2 fields if undefined. * @@ -6812,12 +6829,17 @@ add_option(struct options *options, VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION); if (p[1]) { + /* value specified, assume encapsulation is not + * included unles "mtu" follows later */ options->ce.mssfix = positive_atoi(p[1]); + options->ce.mssfix_encap = false; + options->ce.mssfix_default = false; } - - if (!p[1]) + else { + /* Set MTU to default values */ options->ce.mssfix_default = true; + options->ce.mssfix_encap = true; } if (p[2] && streq(p[2], "mtu")) diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 13d6b0da..3d0f7fe7 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -126,7 +126,7 @@ struct connection_entry int fragment; /* internal fragmentation size */ int mssfix; /* Upper bound on TCP MSS */ - bool mssfix_default; /* true if --mssfix was supplied without a parameter */ + bool mssfix_default; /* true if --mssfix should use the default parameters */ bool mssfix_encap; /* true if --mssfix had the "mtu" parameter to include * overhead from IP and TCP/UDP encapsulation */
The current default is 1450, which translates to 1478 byte packets for udp4 and 1498 byte packets for udp6. This commit changes the mssfix default to take the outer IP overhead into account as well and changes the target to 1492. 1492 was picked in our community meeting for being a very common encapsulation upper bound. The change also disables an mssfix default if tun-mtu is set to a value different than 1500. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/mtu.h | 2 +- src/openvpn/options.c | 60 +++++++++++++++++++++++++++++-------------- src/openvpn/options.h | 2 +- 3 files changed, 43 insertions(+), 21 deletions(-)