| Message ID | 20190328122701.7396-1-cschenk@mail.uni-paderborn.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Setting adapter mtu on windows systems | expand |
Am 28.03.19 um 13:27 schrieb Christopher Schenk: > From: Christopher Schenk <Christopher.Schenk@hotmail.com> > > --- > src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 48a8fdf7..93d028c8 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct tuntap_options *to, > const in_addr_t netmask, > const unsigned int flags); > > +static void netsh_set_mtu_ipv4(const char *flex_name, > + const int mtu); > + > +static void netsh_set_mtu_ipv6(const char *flex_name, > + const int mtu); > + > static void netsh_set_dns6_servers(const struct in6_addr *addr_list, > const int addr_len, > const char *flex_name); > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, > netsh_command(&argv, 4, M_FATAL); > /* set ipv6 dns servers if any are specified */ > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname); > + netsh_set_mtu_ipv6(ifname, tun_mtu); > } > > /* explicit route needed */ > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, > case IPW32_SET_NETSH: > netsh_ifconfig(&tt->options, ifname, tt->local, > tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS); > - > break; > } > + netsh_set_mtu_ipv4(ifname, tun_mtu); > } > > #else /* if defined(TARGET_LINUX) */ > @@ -5236,6 +5243,36 @@ out: > return ret; > } > > +static void > +netsh_set_mtu_ipv4(const char *flex_name, > + const int mtu) > +{ > + struct argv argv = argv_new(); > + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = %d store=active", The spacing here looks a bit odd. I would have expected mtu=%d instead. Is this necessary or was there a reason for doing this? Patch looks okay enough to ACK but: In general, this patch adds a missing feature (setting MTU) with one windows interface only (netsh). And more commonly used interface (interactive service)would be different then leading to harder to debug problems. Are you planning on adding MTU setting to the interactive service as well? If we have patches for MTU setting for both, I have problem to ACK both. Arne
Hi On Thu, Mar 28, 2019 at 9:13 AM Arne Schwabe <arne@rfc2549.org> wrote: > Am 28.03.19 um 13:27 schrieb Christopher Schenk: > > From: Christopher Schenk <Christopher.Schenk@hotmail.com> > > > > --- > > src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > > index 48a8fdf7..93d028c8 100644 > > --- a/src/openvpn/tun.c > > +++ b/src/openvpn/tun.c > > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct > tuntap_options *to, > > const in_addr_t netmask, > > const unsigned int flags); > > > > +static void netsh_set_mtu_ipv4(const char *flex_name, > > + const int mtu); > > + > > +static void netsh_set_mtu_ipv6(const char *flex_name, > > + const int mtu); > > + > > static void netsh_set_dns6_servers(const struct in6_addr *addr_list, > > const int addr_len, > > const char *flex_name); > > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char > *ifname, int tun_mtu, > > netsh_command(&argv, 4, M_FATAL); > > /* set ipv6 dns servers if any are specified */ > > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, > ifname); > > + netsh_set_mtu_ipv6(ifname, tun_mtu); > > } > > > > /* explicit route needed */ > > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char > *ifname, int tun_mtu, > > case IPW32_SET_NETSH: > > netsh_ifconfig(&tt->options, ifname, tt->local, > > tt->adapter_netmask, > NI_IP_NETMASK|NI_OPTIONS); > > - > > break; > > } > > + netsh_set_mtu_ipv4(ifname, tun_mtu); > > } > > > > #else /* if defined(TARGET_LINUX) */ > > @@ -5236,6 +5243,36 @@ out: > > return ret; > > } > > > > +static void > > +netsh_set_mtu_ipv4(const char *flex_name, > > + const int mtu) > > +{ > > + struct argv argv = argv_new(); > > + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = > %d store=active", > > The spacing here looks a bit odd. I would have expected mtu=%d instead. > Is this necessary or was there a reason for doing this? > > Patch looks okay enough to ACK but: > > In general, this patch adds a missing feature (setting MTU) with one > windows interface only (netsh). And more commonly used interface > (interactive service)would be different then leading to harder to debug > problems. > I would go a step further to say we should not add new features that do not work when started using the interactive service. Secondly, we should avoid the old style use of netsh and instead use the iphelper API as far as possible. It should be possible to set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I haven't checked which one is appropriate here. Selva <div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 28, 2019 at 9:13 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 28.03.19 um 13:27 schrieb Christopher Schenk:<br> > From: Christopher Schenk <<a href="mailto:Christopher.Schenk@hotmail.com" target="_blank">Christopher.Schenk@hotmail.com</a>><br> > <br> > ---<br> > src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++-<br> > 1 file changed, 38 insertions(+), 1 deletion(-)<br> > <br> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br> > index 48a8fdf7..93d028c8 100644<br> > --- a/src/openvpn/tun.c<br> > +++ b/src/openvpn/tun.c<br> > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct tuntap_options *to,<br> > const in_addr_t netmask,<br> > const unsigned int flags);<br> > <br> > +static void netsh_set_mtu_ipv4(const char *flex_name,<br> > + const int mtu);<br> > +<br> > +static void netsh_set_mtu_ipv6(const char *flex_name,<br> > + const int mtu);<br> > +<br> > static void netsh_set_dns6_servers(const struct in6_addr *addr_list,<br> > const int addr_len,<br> > const char *flex_name);<br> > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,<br> > netsh_command(&argv, 4, M_FATAL);<br> > /* set ipv6 dns servers if any are specified */<br> > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname);<br> > + netsh_set_mtu_ipv6(ifname, tun_mtu);<br> > }<br> > <br> > /* explicit route needed */<br> > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,<br> > case IPW32_SET_NETSH:<br> > netsh_ifconfig(&tt->options, ifname, tt->local,<br> > tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS);<br> > -<br> > break;<br> > }<br> > + netsh_set_mtu_ipv4(ifname, tun_mtu);<br> > }<br> > <br> > #else /* if defined(TARGET_LINUX) */<br> > @@ -5236,6 +5243,36 @@ out:<br> > return ret;<br> > }<br> > <br> > +static void<br> > +netsh_set_mtu_ipv4(const char *flex_name,<br> > + const int mtu)<br> > +{<br> > + struct argv argv = argv_new();<br> > + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = %d store=active",<br> <br> The spacing here looks a bit odd. I would have expected mtu=%d instead.<br> Is this necessary or was there a reason for doing this?<br> <br> Patch looks okay enough to ACK but:<br> <br> In general, this patch adds a missing feature (setting MTU) with one<br> windows interface only (netsh). And more commonly used interface<br> (interactive service)would be different then leading to harder to debug<br> problems.<br></blockquote><div><br></div><div>I would go a step further to say we should not add new features that</div><div>do not work when started using the interactive service.</div><div><br></div><div>Secondly, we should avoid the old style use of netsh and instead use the</div><div>iphelper API as far as possible. It should be possible to</div><div>set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I haven't</div><div>checked which one is appropriate here.</div><div><br></div><div>Selva </div></div></div>
Hi, From a user perspective, there should probably be a way to disable any programmatic setting of mtu. In our usage, the same network interface that OpenVPN traffic goes out on often also must carry large traffic to other destinations that not uncommonly go through some low mtu path. In these cases we have to manually set mtu to a setting that most likely is much lower than OpenVPN might sense it as. Marvin > On Mar 28, 2019, at 8:00 AM, Selva Nair <selva.nair@gmail.com> wrote: > > Hi > >> On Thu, Mar 28, 2019 at 9:13 AM Arne Schwabe <arne@rfc2549.org> wrote: >> Am 28.03.19 um 13:27 schrieb Christopher Schenk: >> > From: Christopher Schenk <Christopher.Schenk@hotmail.com> >> > >> > --- >> > src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++- >> > 1 file changed, 38 insertions(+), 1 deletion(-) >> > >> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c >> > index 48a8fdf7..93d028c8 100644 >> > --- a/src/openvpn/tun.c >> > +++ b/src/openvpn/tun.c >> > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct tuntap_options *to, >> > const in_addr_t netmask, >> > const unsigned int flags); >> > >> > +static void netsh_set_mtu_ipv4(const char *flex_name, >> > + const int mtu); >> > + >> > +static void netsh_set_mtu_ipv6(const char *flex_name, >> > + const int mtu); >> > + >> > static void netsh_set_dns6_servers(const struct in6_addr *addr_list, >> > const int addr_len, >> > const char *flex_name); >> > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, >> > netsh_command(&argv, 4, M_FATAL); >> > /* set ipv6 dns servers if any are specified */ >> > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname); >> > + netsh_set_mtu_ipv6(ifname, tun_mtu); >> > } >> > >> > /* explicit route needed */ >> > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, >> > case IPW32_SET_NETSH: >> > netsh_ifconfig(&tt->options, ifname, tt->local, >> > tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS); >> > - >> > break; >> > } >> > + netsh_set_mtu_ipv4(ifname, tun_mtu); >> > } >> > >> > #else /* if defined(TARGET_LINUX) */ >> > @@ -5236,6 +5243,36 @@ out: >> > return ret; >> > } >> > >> > +static void >> > +netsh_set_mtu_ipv4(const char *flex_name, >> > + const int mtu) >> > +{ >> > + struct argv argv = argv_new(); >> > + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = %d store=active", >> >> The spacing here looks a bit odd. I would have expected mtu=%d instead. >> Is this necessary or was there a reason for doing this? >> >> Patch looks okay enough to ACK but: >> >> In general, this patch adds a missing feature (setting MTU) with one >> windows interface only (netsh). And more commonly used interface >> (interactive service)would be different then leading to harder to debug >> problems. > > I would go a step further to say we should not add new features that > do not work when started using the interactive service. > > Secondly, we should avoid the old style use of netsh and instead use the > iphelper API as far as possible. It should be possible to > set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I haven't > checked which one is appropriate here. > > Selva > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel <html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto">Hi,<div><br></div><div>From a user perspective, there should probably be a way to disable any programmatic setting of mtu. In our usage, the same network interface that OpenVPN traffic goes out on often also must carry large traffic to other destinations that not uncommonly go through some low mtu path. In these cases we have to manually set mtu to a setting that most likely is much lower than OpenVPN might sense it as. </div><div><br></div><div>Marvin<br><br><div dir="ltr"><br>On Mar 28, 2019, at 8:00 AM, Selva Nair <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> wrote:<br><br></div><blockquote type="cite"><div dir="ltr"><div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 28, 2019 at 9:13 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 28.03.19 um 13:27 schrieb Christopher Schenk:<br> > From: Christopher Schenk <<a href="mailto:Christopher.Schenk@hotmail.com" target="_blank">Christopher.Schenk@hotmail.com</a>><br> > <br> > ---<br> > src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++-<br> > 1 file changed, 38 insertions(+), 1 deletion(-)<br> > <br> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br> > index 48a8fdf7..93d028c8 100644<br> > --- a/src/openvpn/tun.c<br> > +++ b/src/openvpn/tun.c<br> > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct tuntap_options *to,<br> > const in_addr_t netmask,<br> > const unsigned int flags);<br> > <br> > +static void netsh_set_mtu_ipv4(const char *flex_name,<br> > + const int mtu);<br> > +<br> > +static void netsh_set_mtu_ipv6(const char *flex_name,<br> > + const int mtu);<br> > +<br> > static void netsh_set_dns6_servers(const struct in6_addr *addr_list,<br> > const int addr_len,<br> > const char *flex_name);<br> > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,<br> > netsh_command(&argv, 4, M_FATAL);<br> > /* set ipv6 dns servers if any are specified */<br> > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname);<br> > + netsh_set_mtu_ipv6(ifname, tun_mtu);<br> > }<br> > <br> > /* explicit route needed */<br> > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,<br> > case IPW32_SET_NETSH:<br> > netsh_ifconfig(&tt->options, ifname, tt->local,<br> > tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS);<br> > -<br> > break;<br> > }<br> > + netsh_set_mtu_ipv4(ifname, tun_mtu);<br> > }<br> > <br> > #else /* if defined(TARGET_LINUX) */<br> > @@ -5236,6 +5243,36 @@ out:<br> > return ret;<br> > }<br> > <br> > +static void<br> > +netsh_set_mtu_ipv4(const char *flex_name,<br> > + const int mtu)<br> > +{<br> > + struct argv argv = argv_new();<br> > + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = %d store=active",<br> <br> The spacing here looks a bit odd. I would have expected mtu=%d instead.<br> Is this necessary or was there a reason for doing this?<br> <br> Patch looks okay enough to ACK but:<br> <br> In general, this patch adds a missing feature (setting MTU) with one<br> windows interface only (netsh). And more commonly used interface<br> (interactive service)would be different then leading to harder to debug<br> problems.<br></blockquote><div><br></div><div>I would go a step further to say we should not add new features that</div><div>do not work when started using the interactive service.</div><div><br></div><div>Secondly, we should avoid the old style use of netsh and instead use the</div><div>iphelper API as far as possible. It should be possible to</div><div>set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I haven't</div><div>checked which one is appropriate here.</div><div><br></div><div>Selva </div></div></div> </div></blockquote><blockquote type="cite"><div dir="ltr"></div></blockquote><blockquote type="cite"><div dir="ltr"><span>_______________________________________________</span><br><span>Openvpn-devel mailing list</span><br><span><a href="mailto:Openvpn-devel@lists.sourceforge.net">Openvpn-devel@lists.sourceforge.net</a></span><br><span><a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a></span><br></div></blockquote></div></body></html>
Hi,
On Thu, Mar 28, 2019 at 09:33:43AM -0700, Marvin Adeff wrote:
> From a user perspective, there should probably be a way to disable any programmatic setting of mtu. In our usage, the same network interface that OpenVPN traffic goes out on often also must carry large traffic to other destinations that not uncommonly go through some low mtu path. In these cases we have to manually set mtu to a setting that most likely is much lower than OpenVPN might sense it as.
OpenVPN will not touch the LAN MTU, just its own tun/tap interface
(and it does not look at "outgoing interface MTU" anyway - it will
do path MTU sensing if supported, or just statically calculated).
gert
Hi, On 28/03/2019 16:00, Selva Nair wrote: > I would go a step further to say we should not add new features that > do not work when started using the interactive service. > > Secondly, we should avoid the old style use of netsh and instead use the > iphelper API as far as possible. It should be possible to > set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I > haven't > checked which one is appropriate here. I agree on the point that we should add this functionality to the interactive service as well. So i modified the patch accordingly. I also modified my patch so that it uses the SetIpInterfaceEnrty function instead of netsh. --- diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h index 66177a21..f59c5a21 100644 --- a/include/openvpn-msg.h +++ b/include/openvpn-msg.h @@ -39,6 +39,7 @@ typedef enum { msg_del_block_dns, msg_register_dns, msg_enable_dhcp, + msg_set_mtu, } message_type_t; typedef struct { @@ -117,4 +118,11 @@ typedef struct { interface_t iface; } enable_dhcp_message_t; +typedef struct { + message_header_t header; + interface_t iface; + short family; + int mtu; +} set_mtu_message_t; + #endif /* ifndef OPENVPN_MSG_H_ */ diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 48a8fdf7..9fe8444f 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -69,6 +69,10 @@ static void netsh_ifconfig(const struct tuntap_options *to, const in_addr_t netmask, const unsigned int flags); +static void windows_set_mtu(const int iface_indey, + short family, + const int mtu); + static void netsh_set_dns6_servers(const struct in6_addr *addr_list, const int addr_len, const char *flex_name); @@ -201,6 +205,63 @@ out: return ret; } +static bool +do_set_mtu_service(const struct tuntap *tt, const short family, const int mtu) +{ + DWORD len; + bool ret = false; + ack_message_t ack; + struct gc_arena gc = gc_new(); + HANDLE pipe = tt->options.msg_channel; + + set_mtu_message_t mtu_msg = { + .header = { + msg_set_mtu, + sizeof(set_mtu_message_t), + 0 + }, + .iface = {.index = tt->adapter_index,.name = tt->actual_name }, + .mtu = mtu, + .family = family + }; + + if (!send_msg_iservice(pipe, &mtu_msg, sizeof(mtu_msg), &ack, "Set_mtu")) + { + goto out; + } + + if (family == AF_INET) + { + if (ack.error_number != NO_ERROR) + { + msg(M_NONFATAL, "TUN: setting IPv4 mtu using service failed: %s [status=%u if_index=%d]", + strerror_win32(ack.error_number, &gc), ack.error_number, mtu_msg.iface.index); + } + else + { + msg(M_INFO, "IPv4 MTU set to %d on interface %d using service", mtu, mtu_msg.iface.index); + ret = true; + } + } + else if (family == AF_INET6) + { + if (ack.error_number != NO_ERROR) + { + msg(M_NONFATAL, "TUN: setting IPv6 mtu using service failed: %s [status=%u if_index=%d]", + strerror_win32(ack.error_number, &gc), ack.error_number, mtu_msg.iface.index); + } + else + { + msg(M_INFO, "IPv6 MTU set to %d on interface %d using service", mtu, mtu_msg.iface.index); + ret = true; + } + } + +out: + gc_free(&gc); + return ret; +} + #endif /* ifdef _WIN32 */ #ifdef TARGET_SOLARIS @@ -984,6 +1045,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, { do_address_service(true, AF_INET6, tt); do_dns6_service(true, tt); + do_set_mtu_service(tt, AF_INET6, tun_mtu); } else { @@ -1000,6 +1062,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, netsh_command(&argv, 4, M_FATAL); /* set ipv6 dns servers if any are specified */ netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname); + windows_set_mtu(tt->adapter_index, AF_INET6, tun_mtu); } /* explicit route needed */ @@ -1391,9 +1454,16 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, case IPW32_SET_NETSH: netsh_ifconfig(&tt->options, ifname, tt->local, tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS); - break; } + if (tt->options.msg_channel) + { + do_set_mtu_service(tt, AF_INET, tun_mtu); + } + else + { + windows_set_mtu(tt->adapter_index, AF_INET, tun_mtu); + } } #else /* if defined(TARGET_LINUX) */ @@ -5236,6 +5306,46 @@ out: return ret; } +static void +windows_set_mtu(const int iface_index, const short family, + const int mtu) +{ + DWORD err = 0; + struct gc_arena gc = gc_new(); + MIB_IPINTERFACE_ROW row; + InitializeIpInterfaceEntry(&row); + row.Family = family; + row.InterfaceIndex = iface_index; + row.NlMtu = mtu; + + err = SetIpInterfaceEntry(&row); + if (family == AF_INET) + { + if (err != NO_ERROR) + { + msg(M_WARN, "TUN: Setting IPv4 mtu failed: %s [status=%u if_index=%d]", + strerror_win32(err, &gc), err, iface_index); + } + else + { + msg(M_INFO, "Successfully set IPv4 mtu on interface %d", iface_index); + } + } + else if (family == AF_INET6) + { + if (err != NO_ERROR) + { + msg(M_WARN, "TUN: Setting IPv6 mtu failed: %s [status=%u if_index=%d]", + strerror_win32(err, &gc), err, iface_index); + } + else + { + msg(M_INFO, "Successfully set IPv6 mtu on interface %d", iface_index); + } + } +} + + /* * Return a TAP name for netsh commands. */ diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 623c3ff7..6e6a63fe 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -33,6 +33,7 @@ #include <sddl.h> #include <shellapi.h> #include <mstcpip.h> +#include <netioapi.h> #ifdef HAVE_VERSIONHELPERS_H #include <versionhelpers.h> @@ -1198,6 +1199,20 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp) return err; } +static DWORD +HandleMTUMessage(const set_mtu_message_t *mtu) +{ + DWORD err = 0; + MIB_IPINTERFACE_ROW row; + InitializeIpInterfaceEntry(&row); + row.Family = mtu->family; + row.InterfaceIndex = mtu->iface.index; + row.NlMtu = mtu->mtu; + + err = SetIpInterfaceEntry(&row); + return err; +} + static VOID HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists_t *lists) { @@ -1210,6 +1225,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists block_dns_message_t block_dns; dns_cfg_message_t dns; enable_dhcp_message_t dhcp; + set_mtu_message_t mtu; } msg; ack_message_t ack = { .header = { @@ -1276,6 +1292,12 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists ack.error_number = HandleEnableDHCPMessage(&msg.dhcp); } break; + case msg_set_mtu: + if (msg.header.size == sizeof(msg.mtu)) + { + ack.error_number = HandleMTUMessage(&msg.mtu); + } + break; default: ack.error_number = ERROR_MESSAGE_TYPE;
Hi, On Fri, Mar 29, 2019 at 6:25 AM Christopher Schenk <cschenk@mail.uni-paderborn.de> wrote: > > Hi, > > On 28/03/2019 16:00, Selva Nair wrote: > > I would go a step further to say we should not add new features that > > do not work when started using the interactive service. > > > > Secondly, we should avoid the old style use of netsh and instead use the > > iphelper API as far as possible. It should be possible to > > set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I > > haven't > > checked which one is appropriate here. > > I agree on the point that we should add this functionality to the > interactive service as well. So i modified the patch accordingly. > I also modified my patch so that it uses the SetIpInterfaceEnrty > function instead of netsh. Thanks for the update. Looks okay, but would prefer a version sent out by git-send-email so that we can apply and test the patch. Some minor comments below. > > --- > diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h > index 66177a21..f59c5a21 100644 > --- a/include/openvpn-msg.h > +++ b/include/openvpn-msg.h > @@ -39,6 +39,7 @@ typedef enum { > msg_del_block_dns, > msg_register_dns, > msg_enable_dhcp, > + msg_set_mtu, > } message_type_t; > > typedef struct { > @@ -117,4 +118,11 @@ typedef struct { > interface_t iface; > } enable_dhcp_message_t; > > +typedef struct { > + message_header_t header; > + interface_t iface; > + short family; > + int mtu; > +} set_mtu_message_t; > + > #endif /* ifndef OPENVPN_MSG_H_ */ > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 48a8fdf7..9fe8444f 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -69,6 +69,10 @@ static void netsh_ifconfig(const struct > tuntap_options *to, > const in_addr_t netmask, > const unsigned int flags); > > +static void windows_set_mtu(const int iface_indey, iface_indey -> iface_index ? > + short family, > + const int mtu); > + > static void netsh_set_dns6_servers(const struct in6_addr *addr_list, > const int addr_len, > const char *flex_name); > @@ -201,6 +205,63 @@ out: > return ret; > } > > +static bool > +do_set_mtu_service(const struct tuntap *tt, const short family, const > int mtu) > +{ > + DWORD len; > + bool ret = false; > + ack_message_t ack; > + struct gc_arena gc = gc_new(); > + HANDLE pipe = tt->options.msg_channel; > + > + set_mtu_message_t mtu_msg = { > + .header = { > + msg_set_mtu, > + sizeof(set_mtu_message_t), > + 0 > + }, > + .iface = {.index = tt->adapter_index,.name = tt->actual_name }, > + .mtu = mtu, > + .family = family > + }; > + > + if (!send_msg_iservice(pipe, &mtu_msg, sizeof(mtu_msg), &ack, > "Set_mtu")) > + { > + goto out; > + } > + > + if (family == AF_INET) > + { > + if (ack.error_number != NO_ERROR) > + { > + msg(M_NONFATAL, "TUN: setting IPv4 mtu using service > failed: %s [status=%u if_index=%d]", > + strerror_win32(ack.error_number, &gc), > ack.error_number, mtu_msg.iface.index); > + } > + else > + { > + msg(M_INFO, "IPv4 MTU set to %d on interface %d using > service", mtu, mtu_msg.iface.index); > + ret = true; > + } > + } > + else if (family == AF_INET6) > + { > + if (ack.error_number != NO_ERROR) > + { > + msg(M_NONFATAL, "TUN: setting IPv6 mtu using service > failed: %s [status=%u if_index=%d]", > + strerror_win32(ack.error_number, &gc), > ack.error_number, mtu_msg.iface.index); > + } Though this repetition is fine with me and may be we do the same in other similar contexts, cant we combine these two cases into one by defining a string "IPv4" or "IPv6" based on the value of family? The same applies to windows_set_mtu() as well. > + else > + { > + msg(M_INFO, "IPv6 MTU set to %d on interface %d using > service", mtu, mtu_msg.iface.index); > + ret = true; > + } > + } > + > +out: > + gc_free(&gc); > + return ret; > +} > + > #endif /* ifdef _WIN32 */ > > #ifdef TARGET_SOLARIS > @@ -984,6 +1045,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char > *ifname, int tun_mtu, > { > do_address_service(true, AF_INET6, tt); > do_dns6_service(true, tt); > + do_set_mtu_service(tt, AF_INET6, tun_mtu); > } > else > { > @@ -1000,6 +1062,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char > *ifname, int tun_mtu, > netsh_command(&argv, 4, M_FATAL); > /* set ipv6 dns servers if any are specified */ > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, > ifname); > + windows_set_mtu(tt->adapter_index, AF_INET6, tun_mtu); > } > > /* explicit route needed */ > @@ -1391,9 +1454,16 @@ do_ifconfig_ipv4(struct tuntap *tt, const char > *ifname, int tun_mtu, > case IPW32_SET_NETSH: > netsh_ifconfig(&tt->options, ifname, tt->local, > tt->adapter_netmask, > NI_IP_NETMASK|NI_OPTIONS); > - > break; > } > + if (tt->options.msg_channel) > + { > + do_set_mtu_service(tt, AF_INET, tun_mtu); > + } > + else > + { > + windows_set_mtu(tt->adapter_index, AF_INET, tun_mtu); > + } > } > > #else /* if defined(TARGET_LINUX) */ > @@ -5236,6 +5306,46 @@ out: > return ret; > } > > +static void > +windows_set_mtu(const int iface_index, const short family, > + const int mtu) > +{ > + DWORD err = 0; > + struct gc_arena gc = gc_new(); > + MIB_IPINTERFACE_ROW row; > + InitializeIpInterfaceEntry(&row); > + row.Family = family; > + row.InterfaceIndex = iface_index; > + row.NlMtu = mtu; > + > + err = SetIpInterfaceEntry(&row); > + if (family == AF_INET) > + { > + if (err != NO_ERROR) > + { > + msg(M_WARN, "TUN: Setting IPv4 mtu failed: %s [status=%u > if_index=%d]", > + strerror_win32(err, &gc), err, iface_index); > + } > + else > + { > + msg(M_INFO, "Successfully set IPv4 mtu on interface %d", > iface_index); > + } > + } > + else if (family == AF_INET6) > + { > + if (err != NO_ERROR) > + { > + msg(M_WARN, "TUN: Setting IPv6 mtu failed: %s [status=%u > if_index=%d]", > + strerror_win32(err, &gc), err, iface_index); > + } > + else > + { > + msg(M_INFO, "Successfully set IPv6 mtu on interface %d", > iface_index); > + } > + } See the comment above.. > +} > + > + > /* > * Return a TAP name for netsh commands. > */ > diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > index 623c3ff7..6e6a63fe 100644 > --- a/src/openvpnserv/interactive.c > +++ b/src/openvpnserv/interactive.c > @@ -33,6 +33,7 @@ > #include <sddl.h> > #include <shellapi.h> > #include <mstcpip.h> > +#include <netioapi.h> > > #ifdef HAVE_VERSIONHELPERS_H > #include <versionhelpers.h> > @@ -1198,6 +1199,20 @@ HandleEnableDHCPMessage(const > enable_dhcp_message_t *dhcp) > return err; > } > > +static DWORD > +HandleMTUMessage(const set_mtu_message_t *mtu) > +{ > + DWORD err = 0; > + MIB_IPINTERFACE_ROW row; > + InitializeIpInterfaceEntry(&row); > + row.Family = mtu->family; > + row.InterfaceIndex = mtu->iface.index; > + row.NlMtu = mtu->mtu; > + > + err = SetIpInterfaceEntry(&row); > + return err; > +} > + > static VOID > HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, > undo_lists_t *lists) > { > @@ -1210,6 +1225,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD > count, LPHANDLE events, undo_lists > block_dns_message_t block_dns; > dns_cfg_message_t dns; > enable_dhcp_message_t dhcp; > + set_mtu_message_t mtu; > } msg; > ack_message_t ack = { > .header = { > @@ -1276,6 +1292,12 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD > count, LPHANDLE events, undo_lists > ack.error_number = HandleEnableDHCPMessage(&msg.dhcp); > } > break; > + case msg_set_mtu: > + if (msg.header.size == sizeof(msg.mtu)) > + { > + ack.error_number = HandleMTUMessage(&msg.mtu); > + } > + break; > > default: > ack.error_number = ERROR_MESSAGE_TYPE; > Selva
Hi, On Fri, Mar 29, 2019 at 03:33:24PM -0400, Selva Nair wrote: > > + else if (family == AF_INET6) > > + { > > + if (ack.error_number != NO_ERROR) > > + { > > + msg(M_NONFATAL, "TUN: setting IPv6 mtu using service > > failed: %s [status=%u if_index=%d]", > > + strerror_win32(ack.error_number, &gc), > > ack.error_number, mtu_msg.iface.index); > > + } > > Though this repetition is fine with me and may be we do > the same in other similar contexts, cant we combine these > two cases into one by defining a string "IPv4" or "IPv6" > based on the value of family? > > The same applies to windows_set_mtu() as well. Yes please, Newer code already tries to do this (like most of the service-related stuff calling a common function for ipv4-or-ipv6 stuff). (We don't do this for the existing ifconfig orgy because it would not really help readibility... but hopefully that one will get cleaned up more thoroughly eventually) gert
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 48a8fdf7..93d028c8 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct tuntap_options *to, const in_addr_t netmask, const unsigned int flags); +static void netsh_set_mtu_ipv4(const char *flex_name, + const int mtu); + +static void netsh_set_mtu_ipv6(const char *flex_name, + const int mtu); + static void netsh_set_dns6_servers(const struct in6_addr *addr_list, const int addr_len, const char *flex_name); @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, netsh_command(&argv, 4, M_FATAL); /* set ipv6 dns servers if any are specified */ netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, ifname); + netsh_set_mtu_ipv6(ifname, tun_mtu); } /* explicit route needed */ @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, case IPW32_SET_NETSH: netsh_ifconfig(&tt->options, ifname, tt->local, tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS); - break; } + netsh_set_mtu_ipv4(ifname, tun_mtu); } #else /* if defined(TARGET_LINUX) */ @@ -5236,6 +5243,36 @@ out: return ret; } +static void +netsh_set_mtu_ipv4(const char *flex_name, + const int mtu) +{ + struct argv argv = argv_new(); + argv_printf(&argv, "%s%sc interface ipv4 set subinterface %s mtu = %d store=active", + get_win_sys_path(), + NETSH_PATH_SUFFIX, + flex_name, + mtu); + + netsh_command(&argv, 3, M_WARN); + argv_reset(&argv); +} + +static void +netsh_set_mtu_ipv6(const char *flex_name, + const int mtu) +{ + struct argv argv = argv_new(); + argv_printf(&argv, "%s%sc interface ipv6 set subinterface %s mtu = %d store=active", + get_win_sys_path(), + NETSH_PATH_SUFFIX, + flex_name, + mtu); + + netsh_command(&argv, 3, M_WARN); + argv_reset(&argv); +} + /* * Return a TAP name for netsh commands. */
From: Christopher Schenk <Christopher.Schenk@hotmail.com> --- src/openvpn/tun.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)