[Openvpn-devel] [PATCH applied] Re: Set the correct mtu on windows based systems

[Gert D. <ge...@gr...>]

Acked-by: Gert Doering <ge...@gr...>

Selva has ACKed the version sent in April 2019, and it should have been
merged then and there - we (I) let it rot, tun.c got changed quite a bit
for wintun support, now we have a new patch.  Thanks!  (And thanks for
the repeated reminders, this is necessary at times)

I have compared both patches.  They differ in context, of course (*),
but the actual changes introduced by this patch are the same - so, I
think I can take the ACK from Selva for the new patch as well.


(*) one of the context differences is the iservice message types - we
have grown "msg_register_ring_buffers" in the meantime.  So to get this
patch into 2.4 would be somewhat complicated, to ensure no iservice
breakage.  But since 2.5 is really close now, I find that acceptable.


I have also reviewed this patch (so, two ACKs, one from Selva and one
from me) - code looks good.

The "SitePrefixLength = 0" call confused me a bit, but this is correct
according to (long URL):
  https://docs.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-setipinterfaceentry

I have tested this on Win10:

 - config with no "tun-mtu" and iservice -> interface MTU = 1500, good
 - config with "tun-mtu 1330" and iservice -> interface MTU = 1330, good
 - config with no "tun-mtu" and priv. gui -> interface MTU = 1500, good
 - config with "tun-mtu 1330" and priv. gui -> interface MTU = 1330, good

because I sometimes test things before I question myself whether this is
a useful thing to test:
 - config with "tun-mtu 1200" and priv. gui -> 
     *succeeds* for IPv4
     *fails* for IPv6
     "TUN: setting IPv6 mtu using service failed: Falscher Parameter.
      [status=87 if_index=8]

   (which is Windows telling me that "the IETF says that the mininum MTU
    for IPv6 is 1280, so go away and complain to them!" - technically
    correct, though slightly confusing)


Just for reference: "ipconfig /all" does not show the MTU, you need to
call "netsh interface ipv4/ipv6 show subinterfaces".


Remarks:

  - if using the iservice, openvpn will log
      "IPv<x> MTU set to 1330 on interface 8 using service"
    if using the gui in privileged mode, it will log
      "Successfully set IPv<x> mtu on interface 8"

    --> I think it would be good to use the same message here, and 
    include the numeric value in both cases, like

      "IPv<x> MTU set to 1330 on interface 8 using SetIpInterfaceEntry()"

    or so.  Can you send a followup patch?  It will be expedited!

  - shall we do "if MTU < 1280, log a notice and do not attempt to set
    IPv6 MTU"?  Or "just log the notice, in case windows will accept
    it nonetheless"?

    if (mtu<1280)
    {
      msg(M_INFO, "NOTE: IPv6 interface MTU < 1280 conflicts with IETF standards and might not work");
    }


Your patch has been applied to the master branch.

Thanks for your patience.


commit 0213f80ed72ad8b6bb43db3bbd72a66ec2e12fcd
Author: Christopher Schenk
Date:   Tue Apr 21 17:46:12 2020 +0200

     Set the correct mtu on windows based systems

     Signed-off-by: Christopher Schenk <cs...@ma...>
     Acked-by: Selva Nair <sel...@gm...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@ma...>
     URL: https://www.mail-archive.com/ope...@li.../msg19803.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering