diff mbox series

[Openvpn-devel] Fix combination of --dev tap and --topology subnet across multiple platforms.

Message ID 20180819200703.20362-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel] Fix combination of --dev tap and --topology subnet across multiple platforms. |

Commit Message

Gert Doering Aug. 19, 2018, 10:07 a.m. UTC 
  --topology should have no effect in tap mode (tap is always "subnet"),
but due to the way options are checked, setting "topology subnet" caught
an improper branch on all non-linux and non-win32 platforms.

Easily tested by adding "--topology subnet" to a "--dev tap" t_client
test.

Tested, verified, and fixed on FreeBSD 10.4, NetBSD 7.0.1, OpenBSD 6.0,
and OpenSolaris 10.  Compile-tested on MacOS X.

Trac: #1085

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/tun.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Gert Doering Aug. 19, 2018, 10:16 a.m. UTC | #1 
Hi,

On Sun, Aug 19, 2018 at 10:07:03PM +0200, Gert Doering wrote:
> --topology should have no effect in tap mode (tap is always "subnet"),
> but due to the way options are checked, setting "topology subnet" caught
> an improper branch on all non-linux and non-win32 platforms.
> 
> Easily tested by adding "--topology subnet" to a "--dev tap" t_client
> test.
> 
> Tested, verified, and fixed on FreeBSD 10.4, NetBSD 7.0.1, OpenBSD 6.0,
> and OpenSolaris 10.  Compile-tested on MacOS X.
> 
> Trac: #1085

Oh, because the comments omits that notice (it is in the ticket) - this
is 2.4 only.  Master looks sufficiently different that this won't apply,
and Antonio and I have already conspired and decided that this needs to
be fixed for good in master - like:

 - introduce a new enum "interface_style" or so, which can be
    "TUN_MODE_P2P", "TUN_MODE_SUBNET" and "TAP_MODE" (or so)

 - drop "tun", which tries to do the above in boolean form and fails

 - in all platform code, have a switch/case statement instead of a
   chained or nested if/else, making more explicit what the variants
   are - or, if multiple if/else, at least have unambiguous conditions

(Before you ask: "net30" is not handled as subnet internally - except
on windows - but as point-to-point tun)

gert
Antonio Quartulli Sept. 28, 2018, 12:09 a.m. UTC | #2 
Hi,

On 20/08/18 04:07, Gert Doering wrote:
> --topology should have no effect in tap mode (tap is always "subnet"),
> but due to the way options are checked, setting "topology subnet" caught
> an improper branch on all non-linux and non-win32 platforms.
> 
> Easily tested by adding "--topology subnet" to a "--dev tap" t_client
> test.
> 
> Tested, verified, and fixed on FreeBSD 10.4, NetBSD 7.0.1, OpenBSD 6.0,
> and OpenSolaris 10.  Compile-tested on MacOS X.
> 
> Trac: #1085
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>

After staring at the code I couldn't identify any issue and the logic
followed by the code is not more clear (for what it can be ..).

I had a discussion with Gert on IRC and he clarified some doubts I had
in a clear manner, therefore I am happy with this patch.


Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering Oct. 6, 2018, 2:56 a.m. UTC | #3 
Thanks for the review.

Patch has been applied to the release/2.4 branch.

For reference: this is still broken in master, but warrants a proper
cleanup/refactor approach there.

commit 6c13e24e5709f404231632f14758ea8f6bd9ec83
Author: Gert Doering
Date:   Sun Aug 19 22:07:03 2018 +0200

     Fix combination of --dev tap and --topology subnet across multiple platforms.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20180819200703.20362-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17414.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index db7c25ff..8fa07afa 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1091,7 +1091,7 @@  do_ifconfig(struct tuntap *tt,
                         actual
                         );
         }
-        else if (tt->topology == TOP_SUBNET)
+        else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             argv_printf(&argv,
                         "%s %s %s %s netmask %s mtu %d up",
@@ -1173,7 +1173,7 @@  do_ifconfig(struct tuntap *tt,
             }
         }
 
-        if (!tun && tt->topology == TOP_SUBNET)
+        if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             /* Add a network route for the local tun interface */
             struct route_ipv4 r;
@@ -1210,7 +1210,7 @@  do_ifconfig(struct tuntap *tt,
                         tun_mtu
                         );
         }
-        else if (tt->topology == TOP_SUBNET)
+        else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             remote_end = create_arbitrary_remote( tt );
             argv_printf(&argv,
@@ -1239,7 +1239,7 @@  do_ifconfig(struct tuntap *tt,
         openvpn_execve_check(&argv, es, S_FATAL, "OpenBSD ifconfig failed");
 
         /* Add a network route for the local tun interface */
-        if (!tun && tt->topology == TOP_SUBNET)
+        if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             struct route_ipv4 r;
             CLEAR(r);
@@ -1282,7 +1282,7 @@  do_ifconfig(struct tuntap *tt,
                         tun_mtu
                         );
         }
-        else if (tt->topology == TOP_SUBNET)
+        else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             remote_end = create_arbitrary_remote( tt );
             argv_printf(&argv,
@@ -1316,7 +1316,7 @@  do_ifconfig(struct tuntap *tt,
         openvpn_execve_check(&argv, es, S_FATAL, "NetBSD ifconfig failed");
 
         /* Add a network route for the local tun interface */
-        if (!tun && tt->topology == TOP_SUBNET)
+        if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             struct route_ipv4 r;
             CLEAR(r);
@@ -1372,7 +1372,7 @@  do_ifconfig(struct tuntap *tt,
         }
         else
         {
-            if (tt->topology == TOP_SUBNET)
+            if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
             {
                 argv_printf(&argv,
                             "%s %s %s %s netmask %s mtu %d up",
@@ -1402,7 +1402,7 @@  do_ifconfig(struct tuntap *tt,
         tt->did_ifconfig = true;
 
         /* Add a network route for the local tun interface */
-        if (!tun && tt->topology == TOP_SUBNET)
+        if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             struct route_ipv4 r;
             CLEAR(r);
@@ -1445,7 +1445,7 @@  do_ifconfig(struct tuntap *tt,
                         tun_mtu
                         );
         }
-        else if (tt->topology == TOP_SUBNET)
+        else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             remote_end = create_arbitrary_remote( tt );
             argv_printf(&argv,
@@ -1475,7 +1475,7 @@  do_ifconfig(struct tuntap *tt,
         tt->did_ifconfig = true;
 
         /* Add a network route for the local tun interface */
-        if (!tun && tt->topology == TOP_SUBNET)
+        if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
         {
             struct route_ipv4 r;
             CLEAR(r);