[Openvpn-devel,v2,08/21] Decouple MSS fix calculation from frame calculation

Message ID 20211214150901.4118886-1-arne@rfc2549.org
State Accepted
Headers show
Series None | expand

Commit Message

Arne Schwabe Dec. 14, 2021, 4:09 a.m. UTC
This consolidates the MSS fix calculation into a single function
instead having it distributed all over the code. It also calculates
the real wire overhead without extra sizes for buffer etc.

Patch v2: improve comment

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/forward.c |  5 ++---
 src/openvpn/init.c    |  3 ++-
 src/openvpn/mss.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 src/openvpn/mss.h     |  6 ++++++
 src/openvpn/mtu.c     |  9 ---------
 src/openvpn/mtu.h     | 10 ++++++----
 src/openvpn/proto.h   | 11 -----------
 src/openvpn/ssl.c     |  3 ++-
 8 files changed, 58 insertions(+), 29 deletions(-)

Comments

Frank Lichtenheld Dec. 14, 2021, 6:10 a.m. UTC | #1
> Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 16:09 geschrieben:
> 
>  
> This consolidates the MSS fix calculation into a single function
> instead having it distributed all over the code. It also calculates
> the real wire overhead without extra sizes for buffer etc.
> 
> Patch v2: improve comment
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/forward.c |  5 ++---
>  src/openvpn/init.c    |  3 ++-
>  src/openvpn/mss.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>  src/openvpn/mss.h     |  6 ++++++
>  src/openvpn/mtu.c     |  9 ---------
>  src/openvpn/mtu.h     | 10 ++++++----
>  src/openvpn/proto.h   | 11 -----------
>  src/openvpn/ssl.c     |  3 ++-
>  8 files changed, 58 insertions(+), 29 deletions(-)
> 
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 29efcd3b9..f82386a1d 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1493,7 +1493,7 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
>                  /* possibly alter the TCP MSS */
>                  if (flags & PIP_MSSFIX)
>                  {
> -                    mss_fixup_ipv4(&ipbuf, MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
> +                    mss_fixup_ipv4(&ipbuf, c->c2.frame.mss_fix);

I still think this will badly explode in the ce.mssfix == 0 code path. In that case frame.mss_fix == 0 AFAICT
and I see no handling of that possibility in mss_fixup_ipv4/6.

Regards,
--
Frank Lichtenheld
Arne Schwabe Dec. 14, 2021, 11:38 a.m. UTC | #2
Am 14.12.21 um 18:10 schrieb Frank Lichtenheld:
> 
> 
>> Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 16:09 geschrieben:
>>
>>   
>> This consolidates the MSS fix calculation into a single function
>> instead having it distributed all over the code. It also calculates
>> the real wire overhead without extra sizes for buffer etc.
>>
>> Patch v2: improve comment
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>   src/openvpn/forward.c |  5 ++---
>>   src/openvpn/init.c    |  3 ++-
>>   src/openvpn/mss.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>>   src/openvpn/mss.h     |  6 ++++++
>>   src/openvpn/mtu.c     |  9 ---------
>>   src/openvpn/mtu.h     | 10 ++++++----
>>   src/openvpn/proto.h   | 11 -----------
>>   src/openvpn/ssl.c     |  3 ++-
>>   8 files changed, 58 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
>> index 29efcd3b9..f82386a1d 100644
>> --- a/src/openvpn/forward.c
>> +++ b/src/openvpn/forward.c
>> @@ -1493,7 +1493,7 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
>>                   /* possibly alter the TCP MSS */
>>                   if (flags & PIP_MSSFIX)
>>                   {
>> -                    mss_fixup_ipv4(&ipbuf, MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
>> +                    mss_fixup_ipv4(&ipbuf, c->c2.frame.mss_fix);
> 
> I still think this will badly explode in the ce.mssfix == 0 code path. In that case frame.mss_fix == 0 AFAICT
> and I see no handling of that possibility in mss_fixup_ipv4/6.

I won't. This is part of the older parts of OpenVPN that are more 
obscure. You overlooking the implicit assumption that PIP_MSSFIX is only 
set if also c->c2.frame.mssfix is != 0

See the top of the function. If ce.mssfix is 0 then you never have the 
PIP_MSSFIX flag in the flags:

void
process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
{
     if (!c->options.ce.mssfix)
     {
         flags &= ~PIP_MSSFIX;
     }


Arne
Frank Lichtenheld Dec. 14, 2021, 10:58 p.m. UTC | #3
> Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 23:38 geschrieben:
> Am 14.12.21 um 18:10 schrieb Frank Lichtenheld:
> > 
> > 
> >> Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 16:09 geschrieben:
> >> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> >> index 29efcd3b9..f82386a1d 100644
> >> --- a/src/openvpn/forward.c
> >> +++ b/src/openvpn/forward.c
> >> @@ -1493,7 +1493,7 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
> >>                   /* possibly alter the TCP MSS */
> >>                   if (flags & PIP_MSSFIX)
> >>                   {
> >> -                    mss_fixup_ipv4(&ipbuf, MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
> >> +                    mss_fixup_ipv4(&ipbuf, c->c2.frame.mss_fix);
> > 
> > I still think this will badly explode in the ce.mssfix == 0 code path. In that case frame.mss_fix == 0 AFAICT
> > and I see no handling of that possibility in mss_fixup_ipv4/6.
> 
> I won't. This is part of the older parts of OpenVPN that are more 
> obscure. You overlooking the implicit assumption that PIP_MSSFIX is only 
> set if also c->c2.frame.mssfix is != 0
> 
> See the top of the function. If ce.mssfix is 0 then you never have the 
> PIP_MSSFIX flag in the flags:

You're right. Should've not only looked at mss_fixup_*, but also at process_ip_header...

Regards,
--
Frank Lichtenheld
Frank Lichtenheld Dec. 16, 2021, 4:38 a.m. UTC | #4
Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

I'm pretty sure that this patch is correct if 07/21 is correct.

> Frank Lichtenheld <frank@lichtenheld.com> hat am 15.12.2021 10:58 geschrieben:
> 
>  
> > Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 23:38 geschrieben:
> > Am 14.12.21 um 18:10 schrieb Frank Lichtenheld:
> > > 
> > > 
> > >> Arne Schwabe <arne@rfc2549.org> hat am 14.12.2021 16:09 geschrieben:
> > >> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> > >> index 29efcd3b9..f82386a1d 100644
> > >> --- a/src/openvpn/forward.c
> > >> +++ b/src/openvpn/forward.c
> > >> @@ -1493,7 +1493,7 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
> > >>                   /* possibly alter the TCP MSS */
> > >>                   if (flags & PIP_MSSFIX)
> > >>                   {
> > >> -                    mss_fixup_ipv4(&ipbuf, MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
> > >> +                    mss_fixup_ipv4(&ipbuf, c->c2.frame.mss_fix);
> > > 
> > > I still think this will badly explode in the ce.mssfix == 0 code path. In that case frame.mss_fix == 0 AFAICT
> > > and I see no handling of that possibility in mss_fixup_ipv4/6.
> > 
> > I won't. This is part of the older parts of OpenVPN that are more 
> > obscure. You overlooking the implicit assumption that PIP_MSSFIX is only 
> > set if also c->c2.frame.mssfix is != 0
> > 
> > See the top of the function. If ce.mssfix is 0 then you never have the 
> > PIP_MSSFIX flag in the flags:
> 
> You're right. Should've not only looked at mss_fixup_*, but also at process_ip_header...
> 
> Regards,
> --
> Frank Lichtenheld
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

--
Frank Lichtenheld
Gert Doering Dec. 30, 2021, 5:38 a.m. UTC | #5
I've stared at the code (nice, things get simpler :-) ) and done
a few tests (v4 over v4, v4 over v6, ...) with "--mssfix 1000" and
looked at the resulting MSS values.  These are way different from
"master without this" - but arguably, closer to reality than what
we had before.

Old: BF-CBC,    --mssfix 1000 -> MSS 804 / 784
     AES256GCM, --mssfix 1000 -> MSS 856 / 876
New: BF-CBC,    --mssfix 1000 -> MSS 892 / 872
     AES256GCM, --mssfix 1000 -> MSS 904 / 884 

I have not done more sophisticated counting on whether the new values
are *correct*, but tcpdump claims they are:

- "AES256GCM, mssfix 1000", "ssh -4 $remote ls -lR"

16:55:40.171255 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
16:55:40.171267 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
16:55:40.171270 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000

- "AES256GCM, mssfix 1000", "ssh -6 $remote ls -lR"

16:57:04.871238 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000

- IPv6 transport is a also correct (which surprises me a bit, I have
  not seen a reference to the underlying protocol, which changes 
  the "headroom" we have).  At least, sometimes... with AES256GCM:

16:58:31.681280 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.65297: UDP, length 1000
16:58:31.681289 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.65297: UDP, length 1000

- over IPv6 transport, with BF, max packets are off by 8 bytes

17:13:54.561605 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.28958: UDP, length 1008
17:13:54.561760 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.28958: UDP, length 1008

  (which is closer to what I'd expect, but still weird, why not 1020?
   ... my machines do weird stuff here...)


That said, the code has been subjected to t_client tests as well, which were
uneventful (there is nothing that excercises TCP in there), as expected.


The new frame_calculate_mssfix() looks a bit confused regarding
"do we declare + initialize variables in one or two steps", and
"do we call it 'unsigned' or 'unsigned int'".  But this can be 
fixed in a followup cleanup patch ;-)

Not sure I understand what the old code does here, looking at
"tun_mtu_dynamic" - this looks like "adjust to discovered MTU",
but the documentation of --mssfix does not speak about this ... so
it will be interesting to revisit this (documentation + option)
eventually and see if/how we want to consider MTU discovery.

Your patch has been applied to the master branch.

commit d4458eed0c3cf582f787893f033a19cb4629cd76
Author: Arne Schwabe
Date:   Tue Dec 14 16:09:01 2021 +0100

     Decouple MSS fix calculation from frame calculation

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20211214150901.4118886-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23423.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Arne Schwabe Dec. 30, 2021, 6:28 a.m. UTC | #6
Am 30.12.21 um 17:38 schrieb Gert Doering:
> I've stared at the code (nice, things get simpler :-) ) and done
> a few tests (v4 over v4, v4 over v6, ...) with "--mssfix 1000" and
> looked at the resulting MSS values.  These are way different from
> "master without this" - but arguably, closer to reality than what
> we had before.
> 
> Old: BF-CBC,    --mssfix 1000 -> MSS 804 / 784
>       AES256GCM, --mssfix 1000 -> MSS 856 / 876
> New: BF-CBC,    --mssfix 1000 -> MSS 892 / 872
>       AES256GCM, --mssfix 1000 -> MSS 904 / 884
> 
> I have not done more sophisticated counting on whether the new values
> are *correct*, but tcpdump claims they are:
> 
> - "AES256GCM, mssfix 1000", "ssh -4 $remote ls -lR"
> 
> 16:55:40.171255 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
> 16:55:40.171267 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
> 16:55:40.171270 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
> 
> - "AES256GCM, mssfix 1000", "ssh -6 $remote ls -lR"
> 
> 16:57:04.871238 IP 194.97.140.11.51194 > 193.149.48.178.50377: UDP, length 1000
> 
> - IPv6 transport is a also correct (which surprises me a bit, I have
>    not seen a reference to the underlying protocol, which changes
>    the "headroom" we have).  At least, sometimes... with AES256GCM:
> 
> 16:58:31.681280 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.65297: UDP, length 1000
> 16:58:31.681289 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.65297: UDP, length 1000
> 
> - over IPv6 transport, with BF, max packets are off by 8 bytes
> 
> 17:13:54.561605 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.28958: UDP, length 1008
> 17:13:54.561760 IP6 2001:608:0:814::f000:11.51194 > 2001:608:4::ce:c0f.28958: UDP, length 1008
> 
>    (which is closer to what I'd expect, but still weird, why not 1020?
>     ... my machines do weird stuff here...)


That BF-CBC seems have an extra 8 bytes that I somehow missed. CBC is a 
odd since it always gives you a multiple of the blocksize (64 bit or 8 
byte) and if you evenly divide by the blocksize you get an extra block 
just for the padding. I need to reinvestigate that code and send a fixup 
patch for it.

> That said, the code has been subjected to t_client tests as well, which were
> uneventful (there is nothing that excercises TCP in there), as expected.
> 
> 
> The new frame_calculate_mssfix() looks a bit confused regarding
> "do we declare + initialize variables in one or two steps", and
> "do we call it 'unsigned' or 'unsigned int'".  But this can be
> fixed in a followup cleanup patch ;-)

Some of those are due trying to not to break the 80 char limit which 
they would do if we declare them just in one line.

> Not sure I understand what the old code does here, looking at
> "tun_mtu_dynamic" - this looks like "adjust to discovered MTU",
> but the documentation of --mssfix does not speak about this ... so
> it will be interesting to revisit this (documentation + option)
> eventually and see if/how we want to consider MTU discovery.

MTU discovery is only enabled if we also have fragment enabled and then 
it affects both fragment and mss since they both use the mtu_dynamic 
variable.

Arne
Steffan Karger Dec. 30, 2021, 7:16 a.m. UTC | #7
Hi,

On 30-12-2021 18:28, Arne Schwabe wrote:
> That BF-CBC seems have an extra 8 bytes that I somehow missed. CBC is a
> odd since it always gives you a multiple of the blocksize (64 bit or 8
> byte) and if you evenly divide by the blocksize you get an extra block
> just for the padding. I need to reinvestigate that code and send a fixup
> patch for it.

You probably know this, but for clarity: this is how CBC padding works,
not just for BF. It is easier to trigger with BF though, because of the
smaller (64-bit) block, compared to AES (128-bit block).

-Steffan
Gert Doering Dec. 30, 2021, 7:54 a.m. UTC | #8
Hi,

On Thu, Dec 30, 2021 at 07:16:25PM +0100, Steffan Karger wrote:
> On 30-12-2021 18:28, Arne Schwabe wrote:
> > That BF-CBC seems have an extra 8 bytes that I somehow missed. CBC is a
> > odd since it always gives you a multiple of the blocksize (64 bit or 8
> > byte) and if you evenly divide by the blocksize you get an extra block
> > just for the padding. I need to reinvestigate that code and send a fixup
> > patch for it.
> 
> You probably know this, but for clarity: this is how CBC padding works,
> not just for BF. It is easier to trigger with BF though, because of the
> smaller (64-bit) block, compared to AES (128-bit block).

The comment in the code acknowledges this :-) - but the math seems to
be not quite right.

We've tested with a few different --mssfix values and BF-CBC + AES-CBC
and packets (UDP payload) are consistently up to 8 bytes larger than
ordered...

18:34 <@plaisthos> I think I am missing the rounding up to blocksize step 
18:35 <@plaisthos> I basically handle the corner that you do NOT round up and 
                   get an extra block but I completely forgot the rounding up 
                   for all other values
18:40 <@plaisthos> I will look into that CBC thing later, that needs more 
                   testing than just writing a small quick fix

gert

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 29efcd3b9..f82386a1d 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1493,7 +1493,7 @@  process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
                 /* possibly alter the TCP MSS */
                 if (flags & PIP_MSSFIX)
                 {
-                    mss_fixup_ipv4(&ipbuf, MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
+                    mss_fixup_ipv4(&ipbuf, c->c2.frame.mss_fix);
                 }
 
                 /* possibly do NAT on packet */
@@ -1517,8 +1517,7 @@  process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
                 /* possibly alter the TCP MSS */
                 if (flags & PIP_MSSFIX)
                 {
-                    mss_fixup_ipv6(&ipbuf,
-                                   MTU_TO_MSS(TUN_MTU_SIZE_DYNAMIC(&c->c2.frame)));
+                    mss_fixup_ipv6(&ipbuf, c->c2.frame.mss_fix);
                 }
                 if (!(flags & PIP_OUTGOING) && (flags
                                                 &(PIPV6_IMCP_NOHOST_CLIENT | PIPV6_IMCP_NOHOST_SERVER)))
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index eb2678116..462edc01e 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -53,6 +53,7 @@ 
 #include "tls_crypt.h"
 #include "forward.h"
 #include "auth_token.h"
+#include "mss.h"
 
 #include "memdbg.h"
 
@@ -4156,7 +4157,7 @@  init_instance(struct context *c, const struct env_set *env, const unsigned int f
 #endif
 
     /* initialize dynamic MTU variable */
-    frame_init_mssfix(&c->c2.frame, &c->options);
+    frame_calculate_mssfix(&c->c2.frame, &c->c1.ks.key_type, &c->options);
 
     /* bind the TCP/UDP socket */
     if (c->mode == CM_P2P || c->mode == CM_TOP || c->mode == CM_CHILD_TCP)
diff --git a/src/openvpn/mss.c b/src/openvpn/mss.c
index aa5b68ce9..d2842aac5 100644
--- a/src/openvpn/mss.c
+++ b/src/openvpn/mss.c
@@ -30,6 +30,8 @@ 
 #include "syshead.h"
 #include "error.h"
 #include "mss.h"
+#include "crypto.h"
+#include "ssl_common.h"
 #include "memdbg.h"
 
 /*
@@ -204,3 +206,41 @@  mss_fixup_dowork(struct buffer *buf, uint16_t maxmss)
         }
     }
 }
+
+void
+frame_calculate_mssfix(struct frame *frame, struct key_type *kt,
+                       const struct options *options)
+{
+    if (options->ce.mssfix == 0)
+    {
+        return;
+    }
+
+    unsigned int payload_size;
+    unsigned int overhead;
+
+
+    payload_size = frame_calculate_payload_size(frame, options);
+
+    overhead = frame_calculate_protocol_header_size(kt, options,
+                                                    payload_size, false);
+
+    /* Calculate the number of bytes that the payload differs from the payload
+     * MTU. This are fragment/compression/ethernet headers */
+    unsigned payload_overhead = frame_calculate_payload_overhead(frame, options, true);
+
+    /* We are in a "liberal" position with respect to MSS,
+     * i.e. we assume that MSS can be calculated from MTU
+     * by subtracting out only the IP and TCP header sizes
+     * without options.
+     *
+     * (RFC 879, section 7). */
+
+    /* Add 20 bytes for the IPv4 header and 20 byte for the TCP header of the
+     * payload, the mssfix method will add 20 extra if payload is IPv6 */
+    overhead += 20 + 20;
+
+    /* Calculate the maximum MSS value from the max link layer size specified
+     * by ce.mssfix */
+    frame->mss_fix = options->ce.mssfix - overhead - payload_overhead;
+}
\ No newline at end of file
diff --git a/src/openvpn/mss.h b/src/openvpn/mss.h
index 41254e2a6..856f4c4e3 100644
--- a/src/openvpn/mss.h
+++ b/src/openvpn/mss.h
@@ -26,6 +26,8 @@ 
 
 #include "proto.h"
 #include "error.h"
+#include "mtu.h"
+#include "ssl_common.h"
 
 void mss_fixup_ipv4(struct buffer *buf, int maxmss);
 
@@ -33,4 +35,8 @@  void mss_fixup_ipv6(struct buffer *buf, int maxmss);
 
 void mss_fixup_dowork(struct buffer *buf, uint16_t maxmss);
 
+/** Set the --mssfix option. */
+void frame_calculate_mssfix(struct frame *frame, struct key_type *kt,
+                            const struct options *options);
+
 #endif
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index 0da1dadfa..7e5c3c24d 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -211,15 +211,6 @@  frame_subtract_extra(struct frame *frame, const struct frame *src)
     frame->extra_tun   += src->extra_frame;
 }
 
-void
-frame_init_mssfix(struct frame *frame, const struct options *options)
-{
-    if (options->ce.mssfix)
-    {
-        frame_set_mtu_dynamic(frame, options->ce.mssfix, SET_MTU_UPPER_BOUND);
-    }
-}
-
 void
 frame_print(const struct frame *frame,
             int level,
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 5ad0931fd..ae83d3e7a 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -94,6 +94,12 @@  struct frame {
     int link_mtu;               /**< Maximum packet size to be sent over
                                  *   the external network interface. */
 
+    unsigned int mss_fix;      /**< The actual MSS value that should be
+                                 *   written to the payload packets. This
+                                 *   is the value for IPv4 TCP packets. For
+                                 *   IPv6 packets another 20 bytes must
+                                 *   be subtracted */
+
     int link_mtu_dynamic;       /**< Dynamic MTU value for the external
                                  *   network interface. */
 
@@ -152,7 +158,6 @@  struct options;
  * This is the size to "ifconfig" the tun or tap device.
  */
 #define TUN_MTU_SIZE(f)          ((f)->link_mtu - TUN_LINK_DELTA(f))
-#define TUN_MTU_SIZE_DYNAMIC(f)  ((f)->link_mtu_dynamic - TUN_LINK_DELTA(f))
 
 /*
  * This is the maximum packet size that we need to be able to
@@ -291,9 +296,6 @@  void alloc_buf_sock_tun(struct buffer *buf,
                         const struct frame *frame,
                         const bool tuntap_buffer);
 
-/** Set the --mssfix option. */
-void frame_init_mssfix(struct frame *frame, const struct options *options);
-
 /*
  * EXTENDED_SOCKET_ERROR_CAPABILITY functions -- print extra error info
  * on socket errors, such as PMTU size.  As of 2003.05.11, only works
diff --git a/src/openvpn/proto.h b/src/openvpn/proto.h
index f73e50c07..94010a98f 100644
--- a/src/openvpn/proto.h
+++ b/src/openvpn/proto.h
@@ -247,17 +247,6 @@  struct ip_tcp_udp_hdr {
         acc -= (u32) >> 16;    \
 }
 
-/*
- * We are in a "liberal" position with respect to MSS,
- * i.e. we assume that MSS can be calculated from MTU
- * by subtracting out only the IP and TCP header sizes
- * without options.
- *
- * (RFC 879, section 7).
- */
-#define MTU_TO_MSS(mtu) (mtu - sizeof(struct openvpn_iphdr) \
-                         - sizeof(struct openvpn_tcphdr))
-
 /*
  * This returns an ip protocol version of packet inside tun
  * and offset of IP header (via parameter).
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 8cbb129d2..96c78199a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -62,6 +62,7 @@ 
 #include "ssl_ncp.h"
 #include "ssl_util.h"
 #include "auth_token.h"
+#include "mss.h"
 
 #include "memdbg.h"
 
@@ -1893,7 +1894,7 @@  tls_session_update_crypto_params_do_work(struct tls_session *session,
                                    options->replay, packet_id_long_form);
     frame_finalize(frame, options->ce.link_mtu_defined, options->ce.link_mtu,
                    options->ce.tun_mtu_defined, options->ce.tun_mtu);
-    frame_init_mssfix(frame, options);
+    frame_calculate_mssfix(frame, &session->opt->key_type, options);
     frame_print(frame, D_MTU_INFO, "Data Channel MTU parms");
 
     /*