diff mbox series

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

Message ID 20211207170211.3275837-9-arne@rfc2549.org
State Superseded
Headers
Series Big buffer/frame refactoring patch set |

Commit Message

Arne Schwabe Dec. 7, 2021, 6:01 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.

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. 9, 2021, 2:20 a.m. UTC | #1 
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
[...]
> diff --git a/src/openvpn/mss.c b/src/openvpn/mss.c
> index aa5b68ce9..56dea0292 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 TCP header of the payload,

"20 bytes each" would make the sentence much easier to parse, IMHO.

> +     * the mssfix routes 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;
> +}

Regards,
  Frank
--
Frank Lichtenheld
Frank Lichtenheld Dec. 13, 2021, 1:35 a.m. UTC | #2 
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
[...]
> diff --git a/src/openvpn/mss.c b/src/openvpn/mss.c
> index aa5b68ce9..56dea0292 100644
> --- a/src/openvpn/mss.c
> +++ b/src/openvpn/mss.c
[...]
> @@ -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;
> +    }

Wouldn't an ASSERT be better here? This essentially should never happen because ce.mssfix is initialized
to MSSFIX_DEFAULT, anyway. If you handle this without an error here it looks like this will just explode
later in mss_fixup_dowork. An ASSERT might be easier and cleaner.

> +    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 TCP header of the payload,
> +     * the mssfix routes 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

Regards,
--
Frank Lichtenheld
Arne Schwabe Dec. 13, 2021, 5:31 a.m. UTC | #3 
Am 13.12.21 um 13:35 schrieb Frank Lichtenheld:
> 
>> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
> [...]
>> diff --git a/src/openvpn/mss.c b/src/openvpn/mss.c
>> index aa5b68ce9..56dea0292 100644
>> --- a/src/openvpn/mss.c
>> +++ b/src/openvpn/mss.c
> [...]
>> @@ -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;
>> +    }
> 
> Wouldn't an ASSERT be better here? This essentially should never happen because ce.mssfix is initialized
> to MSSFIX_DEFAULT, anyway. If you handle this without an error here it looks like this will just explode
> later in mss_fixup_dowork. An ASSERT might be easier and cleaner.

--mssfix 0 can be used to disable MSSFIX but I broke that in this patch. 
So basically that test is testing if mssfix is enabled.

Arne
diff mbox series

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 85d664ad6..b22ce60af 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..56dea0292 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 TCP header of the payload,
+     * the mssfix routes 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 25b943722..e7ff477cd 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -205,15 +205,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 303e3fe8f..608b30110 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");
 
     /*