Fix --mtu-disc option with IPv6 transport

PATCH In review Julien Muchembled 4 participants 12 messages Jul 29, 2014 – Oct 10, 2015

Share & tools

Julien Muchembled

author

2014-07-29 17:28 UTC #0 lore raw

PATCH 1/1 Fix --mtu-disc option with IPv6 transport

Socket configuration of MTU discovery was done unconditionally at IP level, which has no effect for other protocols. This fixes the issue of OpenVPN sending fragmented tcp6/udp6 packets even when 'mtu-disc yes' option is passed.

Signed-off-by Julien Muchembled <[email protected]...>

---
 src/openvpn/mtu.c    | 23 +++++++++++++++++------
 src/openvpn/mtu.h    |  2 +-
 src/openvpn/socket.c |  2 +-
 3 files changed, 19 insertions(+), 8 deletions(-)

src/openvpn/mtu.c +17 -6

index 13f3f6c..426c487 100644
@@ -153,15 +153,26 @@ frame_print (const struct frame *frame,
 #define MTUDISC_NOT_SUPPORTED_MSG "--mtu-disc is not supported on this OS"
 
 void
-set_mtu_discover_type (int sd, int mtu_type)
+set_mtu_discover_type (int sd, int mtu_type, sa_family_t proto_af)
 {
   if (mtu_type >= 0)
     {
-#if defined(HAVE_SETSOCKOPT) && defined(SOL_IP) && defined(IP_MTU_DISCOVER)
-      if (setsockopt
-	  (sd, SOL_IP, IP_MTU_DISCOVER, &mtu_type, sizeof (mtu_type)))
-	msg (M_ERR, "Error setting IP_MTU_DISCOVER type=%d on TCP/UDP socket",
-	     mtu_type);
+#if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER) && defined(IPV6_MTU_DISCOVER)
+      switch (proto_af)
+	{
+	case AF_INET:
+	  if (setsockopt
+	      (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof (mtu_type)))
+	    msg (M_ERR, "Error setting IP_MTU_DISCOVER type=%d on TCP/UDP socket",
+		 mtu_type);
+	  break;
+	case AF_INET6:
+	  if (setsockopt
+	      (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &mtu_type, sizeof (mtu_type)))
+	    msg (M_ERR, "Error setting IPV6_MTU_DISCOVER type=%d on TCP6/UDP6 socket",
+		 mtu_type);
+	  break;
+	}
 #else
       msg (M_FATAL, MTUDISC_NOT_SUPPORTED_MSG);
 #endif

src/openvpn/mtu.h +1 -1

index 29ec21f..cb41dc6 100644
@@ -207,7 +207,7 @@ void frame_print (const struct frame *frame,
 		  int level,
 		  const char *prefix);
 
-void set_mtu_discover_type (int sd, int mtu_type);
+void set_mtu_discover_type (int sd, int mtu_type, sa_family_t proto_af);
 int translate_mtu_discover_type_name (const char *name);
 
 /*

src/openvpn/socket.c +1 -1

index 9e6bd10..344a9f7 100644
@@ -1685,7 +1685,7 @@ phase2_set_socket_flags (struct link_socket* sock)
     set_cloexec (sock->ctrl_sd);
 
   /* set Path MTU discovery options on the socket */
-  set_mtu_discover_type (sock->sd, sock->mtu_discover_type);
+  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.af);
 
 #if EXTENDED_SOCKET_ERROR_CAPABILITY
   /* if the OS supports it, enable extended error passing on the socket */

Signature · 1 line

1.8.5.2.988.g9b015e5.dirty

Gert Doering

2014-07-29 18:57 UTC #1 lore raw

Hi,

Quoted 3 lines · Julien Muchembled

Socket configuration of MTU discovery was done unconditionally at IP level, which has no effect for other protocols. This fixes the issue of OpenVPN sending fragmented tcp6/udp6 packets even when 'mtu-disc yes' option is passed.

Sounds good. I'm not sure I understand what that socket option *does*, though... will it make the kernel report back any ICMP packet too big messages received? Or will it not honour those unless IPV6_MTU_DISCOVER is set?

Is there documentation for that socket option somewhere?

(Last not least, because I'm too lazy to look: is that patch against git master or against release/2.3?)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Julien Muchembled

author

2014-07-29 20:27 UTC #2 lore raw

Quoted 11 lines · Gert Doering

Quoted 3 lines · Julien Muchembled
Socket configuration of MTU discovery was done unconditionally at IP level, which has no effect for other protocols. This fixes the issue of OpenVPN sending fragmented tcp6/udp6 packets even when 'mtu-disc yes' option is passed.
Sounds good. I'm not sure I understand what that socket option *does*, though... will it make the kernel report back any ICMP packet too big messages received? Or will it not honour those unless IPV6_MTU_DISCOVER is set?
Is there documentation for that socket option somewhere?

'man 7 ip' and 'man 7 ipv6'

For example:
A --[mtu=1500]-- B --[mtu=1400]-- C

Given a UDP6 socket on A with IPV6_MTU_DISCOVER=IP_PMTUDISC_DO,
sendto(1452 bytes, B) will succeed
but sendto(1452 bytes, C) will return EMSGSIZE (except for the first send to C)

Yes, at some point, there are icmp packets.

Quoted 2 lines

(Last not least, because I'm too lazy to look: is that patch against git master or against release/2.3?)

master

Gert Doering

2014-07-29 21:05 UTC #3 lore raw

Hi,

Quoted 3 lines · Julien Muchembled

Quoted 1 line
Is there documentation for that socket option somewhere?
'man 7 ip' and 'man 7 ipv6'

Thanks, will read up.

Quoted 7 lines

For example:
A --[mtu=1500]-- B --[mtu=1400]-- C
Given a UDP6 socket on A with IPV6_MTU_DISCOVER=IP_PMTUDISC_DO,
sendto(1452 bytes, B) will succeed
but sendto(1452 bytes, C) will return EMSGSIZE (except for the first send to C) Yes, at some point, there are icmp packets.

So (being lazy again, sorry) how is the maximum message size communicated back to OpenVPN? Ancilliary data again?

Quoted 4 lines

Quoted 2 lines
(Last not least, because I'm too lazy to look: is that patch against git master or against release/2.3?)
master

Thanks. I'll need to look at the code paths, but I think it warrants inclusion in 2.3 as well - it's not affecting anything that is IPv4, and fixes a bug for IPv6.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Julien Muchembled

author

2014-07-30 01:15 UTC #4 lore raw

Quoted 10 lines · Gert Doering

Quoted 7 lines
For example:
A --[mtu=1500]-- B --[mtu=1400]-- C
Given a UDP6 socket on A with IPV6_MTU_DISCOVER=IP_PMTUDISC_DO,
sendto(1452 bytes, B) will succeed
but sendto(1452 bytes, C) will return EMSGSIZE (except for the first send to C) Yes, at some point, there are icmp packets.
So (being lazy again, sorry) how is the maximum message size communicated back to OpenVPN? Ancilliary data again?

ICMP packets contains the size and ping/ping6 even reports it (this is real example with mtu=1433 between B & C):

# ping6 -c 1 -M do -s 1452 B
PING B (...) 1452 data bytes
1460 bytes from B: icmp_seq=1 ttl=255 time=0.430 ms
# ping6 -c 1 -M do -s 1454 B
PING B(...) 1454 data bytes

Gert Doering

2014-07-30 05:42 UTC #5 lore raw

Hi,

Quoted 9 lines · Julien Muchembled

Quoted 8 lines · Gert Doering
Quoted 7 lines
For example:
A --[mtu=1500]-- B --[mtu=1400]-- C
Given a UDP6 socket on A with IPV6_MTU_DISCOVER=IP_PMTUDISC_DO,
sendto(1452 bytes, B) will succeed
but sendto(1452 bytes, C) will return EMSGSIZE (except for the first send to C) Yes, at some point, there are icmp packets.

Quoted 4 lines

Quoted 2 lines
So (being lazy again, sorry) how is the maximum message size communicated back to OpenVPN? Ancilliary data again?
ICMP packets contains the size and ping/ping6 even reports it (this is real example with mtu=1433 between B & C):

I know that part, but the ICMP packets are seen by the kernel, not by OpenVPN. So how does the information arrive in OpenVPN? sendto() fails, but how does OpenVPN know which max message size will be acceptable?

[..]

Quoted 2 lines

What I've experienced is that fragmentation is bad for perf and that some routers on the internet don't route fragmented UDP packets properly, which is why I've switched to TCP-only for the moment:
http://git.erp5.org/gitweb/re6stnet.git/commit/cd653523a1474ff9f8956a331c42235454887f65

Of course. Having that functionality for IPv6 is a good thing. I was just trying to understand code that I've not yet had to look into :-)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Julien Muchembled

author

2014-07-30 09:29 UTC #6 lore raw

Hello,

Quoted 10 lines · Gert Doering

Quoted 9 lines · Julien Muchembled
Quoted 8 lines · Gert Doering
Quoted 7 lines
For example:
A --[mtu=1500]-- B --[mtu=1400]-- C
Given a UDP6 socket on A with IPV6_MTU_DISCOVER=IP_PMTUDISC_DO,
sendto(1452 bytes, B) will succeed
but sendto(1452 bytes, C) will return EMSGSIZE (except for the first send to C) Yes, at some point, there are icmp packets.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
So (being lazy again, sorry) how is the maximum message size communicated back to OpenVPN? Ancilliary data again?
ICMP packets contains the size and ping/ping6 even reports it (this is real example with mtu=1433 between B & C):
I know that part, but the ICMP packets are seen by the kernel, not by OpenVPN. So how does the information arrive in OpenVPN? sendto() fails, but how does OpenVPN know which max message size will be acceptable?

Although I didn't try all MTU-related options of OpenVPN, it seems to ignore EMSGSIZE. It's not something I planned to study immediately and I don't know how to get the maximum message size programmatically (I could look at the code of ping).

What I can say is by only adding '--mtu-disc yes' option, the MTU of the tun/tap interface remains 1500 and does not change automatically to anything else. If a packet is small enough for mtu=1500 but too big for the underlying network, then it's simply dropped.

Quoted 6 lines

[..]
Quoted 2 lines
What I've experienced is that fragmentation is bad for perf and that some routers on the internet don't route fragmented UDP packets properly, which is why I've switched to TCP-only for the moment:
http://git.erp5.org/gitweb/re6stnet.git/commit/cd653523a1474ff9f8956a331c42235454887f65
Of course. Having that functionality for IPv6 is a good thing. I was just trying to understand code that I've not yet had to look into :-)

By the way, I forgot to mention that this patch could be a regression for platforms that define IP_MTU_DISCOVER but not IPV6_MTU_DISCOVER. I don't know if such platform exist. Maybe the 'switch' clause added by the patch should be splitted.

Gert Doering

2014-07-30 09:37 UTC #7 lore raw

Hi,

Quoted 6 lines · Julien Muchembled

Quoted 3 lines
I know that part, but the ICMP packets are seen by the kernel, not by OpenVPN. So how does the information arrive in OpenVPN? sendto() fails, but how does OpenVPN know which max message size will be acceptable?
Although I didn't try all MTU-related options of OpenVPN, it seems to ignore EMSGSIZE.

Well, handling that error will not lead anywhere, as it just tells us "the packet got lost, because it was too big". More interesting is the information "so, how big is 'not too big'" and that won't be in the error code, but most likely in ancilliary socket data.

I'll go and find out...

Quoted 3 lines

It's not something I planned to study immediately and I don't know how to get the maximum message size programmatically (I could look at the code of ping).
What I can say is by only adding '--mtu-disc yes' option, the MTU of the tun/tap interface remains 1500 and does not change automatically to anything else. If a packet is small enough for mtu=1500 but too big for the underlying network, then it's simply dropped.

We won't change the interface MTU (because on half the platforms, we can't, or it won't have an effect), but OpenVPN is likely to fragment internally. So "--mtu-disc yes" would need to go along with "--fragment".

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Julien Muchembled

author

2014-07-30 09:54 UTC #8 lore raw

Quoted 5 lines · Gert Doering

Quoted 2 lines
Although I didn't try all MTU-related options of OpenVPN, it seems to ignore EMSGSIZE.
Well, handling that error will not lead anywhere, as it just tells us "the packet got lost, because it was too big".

Maybe not such a good idea but could it somehow be translated to tun/tap interface, like returning an ICMP packet ?

Quoted 5 lines

Quoted 1 line
What I can say is by only adding '--mtu-disc yes' option, the MTU of the tun/tap interface remains 1500 and does not change automatically to anything else. If a packet is small enough for mtu=1500 but too big for the underlying network, then it's simply dropped.
We won't change the interface MTU (because on half the platforms, we can't, or it won't have an effect), but OpenVPN is likely to fragment internally. So "--mtu-disc yes" would need to go along with "--fragment".

I've always been dubious about the efficiency of automatic fragmentation (wherever it happens) because it would often split each big packet into 1 big (but little smaller) and 1 very small.

So I would leave a way for OpenVPN users to disable IP fragmentation (probably to do MTU discovery) but not automatically enable --fragment (which would become totally useless since the MTU of the tun/tap is small enough).

Andre Valentin

2014-07-30 10:30 UTC #9 lore raw

In reply to Gert Doering

Hi!

I followed your discussion because I also tested the fragmentation with pmtu discovery and came to the results that it does not help me. Therefore I currently use fragmentation.

Quoted 10 lines · Gert Doering

Hi,
Quoted 6 lines · Julien Muchembled
Quoted 3 lines
I know that part, but the ICMP packets are seen by the kernel, not by OpenVPN. So how does the information arrive in OpenVPN? sendto() fails, but how does OpenVPN know which max message size will be acceptable?
Although I didn't try all MTU-related options of OpenVPN, it seems to ignore EMSGSIZE.

Quoted 6 lines

Well, handling that error will not lead anywhere, as it just tells us "the packet got lost, because it was too big". More interesting is the information "so, how big is 'not too big'" and that won't be in the error code, but most likely in ancilliary socket data.
I'll go and find out...

If the mtu discovery currently is not implemented, how about this idea:
-A client sends a big packet.
-the vpn server encapsulates it
-a router on the path sends back the corresponding ICMP packet "too big".
-the vpn server gets the packet and reads the max mtu size
-the vpn server creates a fake icmp packet for the client to tell him the mtu for this connection -the client uses this information and sends a perfect fitting packet, or 2 :-)

This way it is all at the client and the server does not have to implement fragmentation.

Just an idea ;-)

André

Arne Schwabe

2015-10-10 09:44 UTC #10 lore raw

PATCH v2 1/1 ] Fix --mtu-disc option with IPv6 transport

In reply to Julien Muchembled

From: Julien Muchembled <[email protected]...>

Patch V2 (by Arne Schwabe): Rebase to current master and have separate #ifdefs for IPv4 an IPv6

Signed-off-by Julien Muchembled <[email protected]...>

---
 src/openvpn/mtu.c    | 29 ++++++++++++++++++++++-------
 src/openvpn/mtu.h    |  2 +-
 src/openvpn/socket.c |  2 +-
 3 files changed, 24 insertions(+), 9 deletions(-)

src/openvpn/mtu.c +22 -7

index 3665a34..24531c9 100644
@@ -153,17 +153,32 @@ frame_print (const struct frame *frame,
 #define MTUDISC_NOT_SUPPORTED_MSG "--mtu-disc is not supported on this OS"
 
 void
-set_mtu_discover_type (int sd, int mtu_type)
+set_mtu_discover_type (int sd, int mtu_type, sa_family_t proto_af)
 {
   if (mtu_type >= 0)
     {
-#if defined(HAVE_SETSOCKOPT) && defined(SOL_IP) && defined(IP_MTU_DISCOVER)
-      if (setsockopt (sd, SOL_IP, IP_MTU_DISCOVER, (void *) &mtu_type, sizeof (mtu_type)))
-	msg (M_ERR, "Error setting IP_MTU_DISCOVER type=%d on TCP/UDP socket",
-	     mtu_type);
-#else
-      msg (M_FATAL, MTUDISC_NOT_SUPPORTED_MSG);
+      switch (proto_af)
+	{
+#if defined(HAVE_SETSOCKOPT) && defined(IP_MTU_DISCOVER)
+	case AF_INET:
+	  if (setsockopt
+	      (sd, IPPROTO_IP, IP_MTU_DISCOVER, &mtu_type, sizeof (mtu_type)))
+	    msg (M_ERR, "Error setting IP_MTU_DISCOVER type=%d on TCP/UDP socket",
+		 mtu_type);
+	  break;
+#endif
+#if defined(HAVE_SETSOCKOPT) && defined(IPV6_MTU_DISCOVER)
+	case AF_INET6:
+	  if (setsockopt
+	      (sd, IPPROTO_IPV6, IPV6_MTU_DISCOVER, &mtu_type, sizeof (mtu_type)))
+	    msg (M_ERR, "Error setting IPV6_MTU_DISCOVER type=%d on TCP6/UDP6 socket",
+		 mtu_type);
+	  break;
 #endif
+	default:
+	  msg (M_FATAL, MTUDISC_NOT_SUPPORTED_MSG);
+	  break;
+	}
     }
 }

src/openvpn/mtu.h +1 -1

index bccd681..f94de89 100644
@@ -207,7 +207,7 @@ void frame_print (const struct frame *frame,
 		  int level,
 		  const char *prefix);
 
-void set_mtu_discover_type (int sd, int mtu_type);
+void set_mtu_discover_type (int sd, int mtu_type, sa_family_t proto_af);
 int translate_mtu_discover_type_name (const char *name);
 
 /*

src/openvpn/socket.c +1 -1

index bd8dcb1..925665c 100644
@@ -1676,7 +1676,7 @@ phase2_set_socket_flags (struct link_socket* sock)
     set_cloexec (sock->ctrl_sd);
 
   /* set Path MTU discovery options on the socket */
-  set_mtu_discover_type (sock->sd, sock->mtu_discover_type);
+  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.af);
 
 #if EXTENDED_SOCKET_ERROR_CAPABILITY
   /* if the OS supports it, enable extended error passing on the socket */

Signature · 1 line

1.9.5 (Apple Git-50.3)

Gert Doering

2015-10-10 09:59 UTC #11 lore raw

ACK (to v2). Your patch has been applied to the master branch.

commit 2bed089d31a12c2d0277e36a64964ebab6640f75

Author: Julien Muchembled
Date:   Sat Oct 10 11:44:51 2015 +0200

     Fix --mtu-disc option with IPv6 transport

     Signed-off-by: Julien Muchembled <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10229
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering