[Openvpn-devel,v4,2/8] Change the default for mssfix to mssfix 1492 mtu

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

Commit Message

Arne Schwabe Feb. 10, 2022, 4:26 p.m. UTC
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(-)

Comments

Gert Doering Feb. 11, 2022, 9:44 a.m. UTC | #1
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
Gert Doering Feb. 11, 2022, 9:55 a.m. UTC | #2
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
Arne Schwabe Feb. 11, 2022, 11:16 a.m. UTC | #3
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

Patch

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 */