[Openvpn-devel] [PATCH v5] Add mtu paramter to --fragment and change fragment calculation
Instead relying on the link_mtu_dynamic field and its calculation
in the frame struct, add a new field max_fragment_size and add
a calculation of it similar to mssfix.
Also whenever mssfix value is calculated, we also want to calculate
the values for fragment as both options need to be calculated from
the real overhead.
Patch v2: Fix syntax in rst man page
Patch v5: fix segfault when get_ip_encap_overhead gets called early in
init_instance and note that these calls will always be
overwritten by NCP in tls_session_update_crypto_params
Signed-off-by: Arne Schwabe
---
Changes.rst| 9 +-
doc/man-sections/link-options.rst | 20 -
src/openvpn/forward.c | 3 +-
src/openvpn/fragment.c | 4 +-
src/openvpn/init.c | 15 ++--
src/openvpn/mss.c | 111 +++--
src/openvpn/mss.h | 13 ++-
src/openvpn/mtu.c | 48 +--
src/openvpn/mtu.h | 21 +++--
src/openvpn/options.c | 12 ++-
src/openvpn/options.h | 2 +
src/openvpn/socket.c | 11 ---
src/openvpn/socket.h | 2 -
src/openvpn/ssl.c | 20 +
tests/unit_tests/openvpn/test_crypto.c | 8 +-
15 files changed, 185 insertions(+), 114 deletions(-)
diff --git a/Changes.rst b/Changes.rst
index 7d6fb7f7..ceb0b268 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -63,10 +63,11 @@ Optional ciphers in ``--data-ciphers``
those as optional and only use them if the SSL library supports them.
-Improved ``--mssfix`` calculation
-The ``--mssfix`` option now allows an optional :code:`mtu` parameter to
specify
-that different overhead for IPv4/IPv6 should taken into account and the
resulting
-size is specified as the total size of the VPN packets including IP and
UDP headers.
+Improved ``--mssfix`` and ``--fragment`` calculation
+The ``--mssfix`` and ``--fragment`` options now allow an optional
:code:`mtu`
+parameter to specify that different overhead for IPv4/IPv6 should taken
into
+account and the resulting size is specified as the total size of the VPN
packets
+including IP and UDP headers.
Deprecated features
---
diff --git a/doc/man-sections/link-options.rst
b/doc/man-sections/link-options.rst
index 1792aaec..1cf6dd84 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -24,13 +24,25 @@ the local and the remote host.
from any address, not only the address which was specified in the
``--remote`` option.
---fragment max
+--fragment args
+
+ Valid syntax:
+ ::
+
+ fragment max
+ fragment max mtu
+
Enable internal datagram fragmentation so that no UDP datagrams are sent
which are larger than ``max`` bytes.
- The ``max`` parameter is interpreted in the same way as the
- ``--link-mtu`` parameter, i.e. the UDP packet size after encapsulation
- overhead has been added in, but not including the UDP header itself.
+ If the :code:`mtu` parameter is present the ``max`` parameter is
+ interpreted to include IP and UDP encapsulation overhead. The
+ :code:`mtu` parameter is introduced in OpenVPN version 2.6.0.
+
+ If the :code:`mtu` parameter is absent, the ``max`` parameter is
+ interpreted in the same way as the ``--link-mtu`` parameter, i.e.
+ the UDP packet size after encapsulation overhead has been added in,
+ but not including the UDP header itself.
The ``--fragment`` option only makes sense when you are using the UDP
protocol (``--proto udp``).
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index dcc430d4..37554fc9 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -482,8 +482,7 @@ check_fragment(struct context *c)
/* OS MTU Hint? */
if (lsi->mtu_changed && lsi->lsa)
{
-frame_adjust_path_mtu(&c->c2.frame_fragment, c->c2.link_socket->mtu,
- lsi->lsa->actual.dest.addr.sa.sa_family,
lsi->proto);
+frame_adjust_path_mtu(c);
lsi->mtu_changed = false;
}
diff --git a/src/openvpn/fragment.c b/src/openvpn/fragment.c
index 6ede4b95..f10fa9ac 100644
--- a/src/openvpn/fragment.c
+++ b/src/openvpn/fragment.c
@@ -335,12 +335,12 @@ fragment_outgoing(struct fragment_master *f, struct
buffer *buf,
msg(D_FRAG_ERRORS, "FRAG: outgoing buffer is not empty,
len=[%d,%d]",
buf->len, f->outgoing.len);
}
-if (buf->len > PAYLOAD_SIZE_DYNAMIC(frame)) /* should we fragment? */
+if (buf->len > frame->max_fragment_size) /* should we fragment? */
{
/*
* Send the datagram as a series of 2 or more fragments.
*/
-f->outgoing_frag_size = optimal_fragment_size(buf->len,
PAYLOAD_SIZE_DYNAMIC(frame));
+f->outgoing_frag_size = optima
Re: [Openvpn-devel] [PATCH v4 3/8] Add mtu paramter to --fragment and change fragment calculation
... moves the crash onward to 0x555891b6 in datagram_overhead (proto=, af=10) at socket.h:617 617 overhead += (proto == PROTO_UDP) ? 8 : 20; (gdb) where #0 0x555891b6 in datagram_overhead (proto=, af=10) at socket.h:617 #1 get_ip_encap_overhead (lsi=0x0, options=0x55642280) at mss.c:248 #2 frame_calculate_mssfix (lsi=0x0, options=0x55642280, kt=0x55642b40, frame=0x55642dd0) at mss.c:305 #3 frame_calculate_dynamic (frame=frame@entry=0x55642dd0, kt=kt@entry=0x55642b40, options=options@entry=0x55642280, lsi=0x0) at mss.c:334 #4 0x5557b2bb in init_instance (c=c@entry=0x55642280, env=, flags=flags@entry=10) at init.c:4234 #5 0x5557c4a6 in inherit_context_child (dest=dest@entry=0x55642280, src=src@entry=0x7fffc4b0) at init.c:4459 which looks weird, but is likely due to inlining. The culprit is this one: return datagram_overhead(af, lsi->proto); "lsi" is still NULL... Not sure how to fix this in a good way. This function is called to early now and doesn't anything useful anymore apart when used in --static/crypto none context. So I changed it to not crash in these cases and added a comment that the call is superflous for most of OpenVPN's operation. If we at some point remove --static/no crypto (non TLS modes), we can do a lot of cleanup. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3 v2] doc/options: clean up documentation for --proto and related options
On 10/02/2022 11:21, Frank Lichtenheld wrote: The family specific options were generally omitted. Signed-off-by: Frank Lichtenheld --- doc/man-sections/client-options.rst | 5 + doc/man-sections/link-options.rst | 5 - src/openvpn/options.c | 17 + 3 files changed, 18 insertions(+), 9 deletions(-) v2: move #define around diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index 73f1ea51..4c4a8707 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -198,6 +198,11 @@ configuration. When iterating through connection profiles, only consider profiles using protocol ``p`` (:code:`tcp` \| :code:`udp`). + Note that this specifically only affects the protocol, not the inet + family (i.e. IPv4 vs. IPv6). While the option actually accepts + values like :code:`udp6`, there is no difference to specifying + :code:`udp`. + I'm sorry I didn't catch this at the previous round; somehow I mixed it with the --proto change below. The section above is related to --proto-force. May I suggest we clarify this text a bit more? As we have multiple definitions of protocol - whether it is the IP layer protocol or the TCP/UDP protocol on top of the IP layer. And in --proto we do indeed mix them up into a single option. Perhaps something like the suggestion below might be somewhat clearer? Note that this specifically only affects the TCP/IP transport layer protocol (UDP/TCP), not the TCP/IP network layers (IPv4/IPv6). In practice it means it will only consider connection profiles using either TCP or UDP. This does not affect whether IPv4 or IPv6 is used as IP protocols. In this context, :code:`udp`, :code:`udp4` and :code:`udp6` are all considered the same. And similar with :code:`tcp`, :code:`tcp4` and :code:`tcp6` The rest of the changes looks good now, and the relocation of the #define is better as well. -- kind regards, David Sommerseth OpenVPN Inc OpenPGP_signature Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 3/8] Add mtu paramter to --fragment and change fragment calculation
Hi,
On Fri, Feb 11, 2022 at 12:27:37PM +0100, Gert Doering wrote:
> the crucial part is "lsi=0x0" here, I think, but I'm not sure why... we do
> have a socket, we know we bound it to IPv6 (+dual-stack)...
Trying this trivial fix...
--- a/src/openvpn/mss.c
+++ b/src/openvpn/mss.c
@@ -234,7 +234,7 @@ get_ip_encap_overhead(const struct options *options,
{
/* Add the overhead of the encapsulating IP packets */
sa_family_t af;
-if (lsi->lsa)
+if (lsi && lsi->lsa)
{
af = lsi->lsa->actual.dest.addr.sa.sa_family;
}
... moves the crash onward to
0x555891b6 in datagram_overhead (proto=, af=10) at socket.h:617
617 overhead += (proto == PROTO_UDP) ? 8 : 20;
(gdb) where
#0 0x555891b6 in datagram_overhead (proto=, af=10) at socket.h:617
#1 get_ip_encap_overhead (lsi=0x0, options=0x55642280) at mss.c:248
#2 frame_calculate_mssfix (lsi=0x0, options=0x55642280, kt=0x55642b40,
frame=0x55642dd0) at mss.c:305
#3 frame_calculate_dynamic (frame=frame@entry=0x55642dd0,
kt=kt@entry=0x55642b40, options=options@entry=0x55642280, lsi=0x0)
at mss.c:334
#4 0x5557b2bb in init_instance (c=c@entry=0x55642280,
env=, flags=flags@entry=10) at init.c:4234
#5 0x5557c4a6 in inherit_context_child
(dest=dest@entry=0x55642280, src=src@entry=0x7fffc4b0) at init.c:4459
which looks weird, but is likely due to inlining.
The culprit is this one:
return datagram_overhead(af, lsi->proto);
"lsi" is still NULL...
Not sure how to fix this in a good way.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 3/8] Add mtu paramter to --fragment and change fragment calculation
Hi, On Thu, Feb 10, 2022 at 05:26:27PM +0100, Arne Schwabe wrote: > Instead relying on the link_mtu_dynamic field and its calculation > in the frame struct, add a new field max_fragment_size and add > a calculation of it similar to mssfix. > > Also whenever mssfix value is calculated, we also want to calculate > the values for fragment as both options need to be calculated from > the real overhead. > > Patch v2: Fix syntax in rst man page Not sure what exactly caused this (maybe the reordering), but applying 3/8 after 1+2 (in tree) leads to "UDP p2mp server crashing" 2022-02-11 12:24:07 us=152359 Initialization Sequence Completed 2022-02-11 12:24:18 us=290966 MULTI: multi_create_instance called 2022-02-11 12:24:18 us=291063 194.97.140.21:62328 Re-using SSL/TLS context 2022-02-11 12:24:18 us=291116 194.97.140.21:62328 LZO compression initializing 2022-02-11 12:24:18 us=291376 194.97.140.21:62328 Control Channel MTU parms [ mss_fix:0 max_frag:0 tun_mtu:1250 headroom:126 payload:1376 tailroom:126 L:1622 EF:38 EB:0 ET:0 EL:3 ] Program received signal SIGSEGV, Segmentation fault. frame_calculate_mssfix (lsi=0x0, options=0x55642280, kt=0x55642b40, frame=0x55642dd0) at mss.c:305 305 overhead += get_ip_encap_overhead(options, lsi); (gdb) where #0 frame_calculate_mssfix (lsi=0x0, options=0x55642280, kt=0x55642b40, frame=0x55642dd0) at mss.c:305 #1 frame_calculate_dynamic (frame=frame@entry=0x55642dd0, kt=kt@entry=0x55642b40, options=options@entry=0x55642280, lsi=0x0) at mss.c:334 #2 0x5557b2bb in init_instance (c=c@entry=0x55642280, env=, flags=flags@entry=10) at init.c:4234 #3 0x5557c4a6 in inherit_context_child (dest=dest@entry=0x55642280, src=src@entry=0x7fffc4b0) at init.c:4459 #4 0x55591af3 in multi_create_instance (m=m@entry=0x7fffc3f0, real=real@entry=0x7fffc2b0) at multi.c:759 #5 0x5558b89e in multi_get_create_instance_udp (m=0x7fffc3f0, floated=floated@entry=0x7fffc33f) at mudp.c:103 #6 0x55591efa in multi_process_incoming_link (m=m@entry=0x7fffc3f0, instance=instance@entry=0x0, mpp_flags=mpp_flags@entry=5) at multi.c:3107 the crucial part is "lsi=0x0" here, I think, but I'm not sure why... we do have a socket, we know we bound it to IPv6 (+dual-stack)... gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 2/8] Change the default for mssfix to mssfix 1492 mtu
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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Change the default for mssfix to mssfix 1492 mtu
Acked-by: Gert Doering 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 Acked-by: Gert Doering Message-Id: <20220210162632.3309974-2-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23754.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4 2/8] Change the default for mssfix to mssfix 1492 mtu
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
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Replace TUN_MTU_SIZE with frame->tun_mtu
Acked-by: Gert Doering Code change looks very straight forward :-) Passes all my torture tests on client and server (except frame max setting on P2P TAP, but that's known and will be fixed at the end of the series). For reference, this was 19/21 in v1 and 12/14 in v3 of the series, which could not go in because it broke compilation. The difference v3 -> v4 is "more TUN_MTU_SIZE() calls get converted" - all of these in code parts that got removed in other parts of the v3 patch series, but breaking compilation nonetheless. Doing this as 01/08 now will also magically fix the "ifconfig has the wrong mtu value" problem that made me stall 09/14 v3. Your patch has been applied to the master branch. commit 364b7b4f04985600e0a0a46e79555379b5df9977 Author: Arne Schwabe Date: Thu Feb 10 17:26:25 2022 +0100 Replace TUN_MTU_SIZE with frame->tun_mtu Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20220210162632.3309974-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23752.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
