| Message ID | 20180819200703.20362-1-gert@greenie.muc.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Fix combination of --dev tap and --topology subnet across multiple platforms. | expand |
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
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>
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 --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);
--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(-)