[Openvpn-devel,06/21] Remove post_open_mtu code

Message ID 20211207170211.3275837-7-arne@rfc2549.org
State Accepted
Headers show
Series
  • Big buffer/frame refactoring patch set
Related show

Commit Message

Arne Schwabe Dec. 7, 2021, 5:01 p.m.
This code is probably from a time when we could not set the MTU on
the Windows tap6 driver. Nowadays we can set the MTU on this device,
so this code is a noop now.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/init.c | 10 ----------
 src/openvpn/ssl.c  |  2 +-
 src/openvpn/tun.c  |  1 -
 src/openvpn/tun.h  |  4 ----
 4 files changed, 1 insertion(+), 16 deletions(-)

Comments

Frank Lichtenheld Dec. 9, 2021, 10:47 a.m. | #1
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 8cbb129d2..303e3fe8f 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1897,7 +1897,7 @@ tls_session_update_crypto_params_do_work(struct tls_session *session,
>      frame_print(frame, D_MTU_INFO, "Data Channel MTU parms");
>  
>      /*
> -     * mssfix uses data channel framing, which at this point contains
> +     * mssfix uses data channel framing, which at this poipnt contains

Spurious change

Regards,
  Frank
--
Frank Lichtenheld
Frank Lichtenheld Dec. 10, 2021, 4:17 p.m. | #2
> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
> This code is probably from a time when we could not set the MTU on
> the Windows tap6 driver. Nowadays we can set the MTU on this device,
> so this code is a noop now.
[...]
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 75d5eaf7b..12bdd2005 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6071,7 +6071,6 @@ tuntap_get_mtu(struct tuntap *tt)
>                          &mtu, sizeof(mtu),
>                          &mtu, sizeof(mtu), &len, NULL))
>      {
> -        tt->post_open_mtu = (int)mtu;
>          msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu);
>      }
>  }

This change makes no sense to me. Why would you not remove the complete tuntap_get_mtu call? If the whole point was to get the MTU, and now it will be the one we set, why even to the ioctl at all?

Regards,
  Frank
--
Frank Lichtenheld
Arne Schwabe Dec. 13, 2021, 3:39 p.m. | #3
Am 10.12.21 um 17:17 schrieb Frank Lichtenheld:
>> Arne Schwabe <arne@rfc2549.org> hat am 07.12.2021 18:01 geschrieben:
>> This code is probably from a time when we could not set the MTU on
>> the Windows tap6 driver. Nowadays we can set the MTU on this device,
>> so this code is a noop now.
> [...]
>> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
>> index 75d5eaf7b..12bdd2005 100644
>> --- a/src/openvpn/tun.c
>> +++ b/src/openvpn/tun.c
>> @@ -6071,7 +6071,6 @@ tuntap_get_mtu(struct tuntap *tt)
>>                           &mtu, sizeof(mtu),
>>                           &mtu, sizeof(mtu), &len, NULL))
>>       {
>> -        tt->post_open_mtu = (int)mtu;
>>           msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu);
>>       }
>>   }
> 
> This change makes no sense to me. Why would you not remove the complete tuntap_get_mtu call? If the whole point was to get the MTU, and now it will be the one we set, why even to the ioctl at all?

I wanted to keep the message that is currently printed at verb 1. If we 
decide that the message can be removed I can remove the calling of the 
ioctl as well.

Arne
Gert Doering Dec. 14, 2021, 8:42 a.m. | #4
Acked-by: Gert Doering <gert@greenie.muc.de>

I follow the reasoning - since we set the MTU now, we do not need to
adjust the frame afterwards.  Logging the MTU might be useful (Windows
is weird, maybe it does not always "stick"...), so let's keep this in 
for the time being.

Spurious comment change in ssl.c removed from the patch.

I like the comment

 /* Some TUN/TAP drivers like to be ioctled for mtu after open */

"like to be ioctled", hah :-) - but why this is there is lost lore from
the long gone past...

I have test compiled this on mingw.


Your patch has been applied to the master branch.

commit c27868bfc2ad6ee21c28f4addb1c2fc003bfa61a
Author: Arne Schwabe
Date:   Tue Dec 7 18:01:56 2021 +0100

     Remove post_open_mtu code

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20211207170211.3275837-7-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23327.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 0009bcb72..85d664ad6 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1809,16 +1809,6 @@  do_open_tun(struct context *c)
                  c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx);
     }
 
-    /*
-     * Did tun/tap driver give us an MTU?
-     */
-    if (c->c1.tuntap->post_open_mtu)
-    {
-        frame_set_mtu_dynamic(&c->c2.frame,
-                              c->c1.tuntap->post_open_mtu,
-                              SET_MTU_TUN | SET_MTU_UPPER_BOUND);
-    }
-
     ret = true;
     static_context = c;
 #ifndef TARGET_ANDROID
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 8cbb129d2..303e3fe8f 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1897,7 +1897,7 @@  tls_session_update_crypto_params_do_work(struct tls_session *session,
     frame_print(frame, D_MTU_INFO, "Data Channel MTU parms");
 
     /*
-     * mssfix uses data channel framing, which at this point contains
+     * mssfix uses data channel framing, which at this poipnt contains
      * actual overhead. Fragmentation logic uses frame_fragment, which
      * still contains worst case overhead. Replace it with actual overhead
      * to prevent unneeded fragmentation.
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 75d5eaf7b..12bdd2005 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6071,7 +6071,6 @@  tuntap_get_mtu(struct tuntap *tt)
                         &mtu, sizeof(mtu),
                         &mtu, sizeof(mtu), &len, NULL))
     {
-        tt->post_open_mtu = (int)mtu;
         msg(D_MTU_INFO, "TAP-Windows MTU=%d", (int)mtu);
     }
 }
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index aa1e47b5a..d4657537c 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -214,10 +214,6 @@  struct tuntap
 #endif
     /* used for printing status info only */
     unsigned int rwflags_debug;
-
-    /* Some TUN/TAP drivers like to be ioctled for mtu
-     * after open */
-    int post_open_mtu;
 };
 
 static inline bool