Re: [Openvpn-devel] [PATCH] Improve ifconfig_sanity_check()
On 09/12/16 09:15, Gert Doering wrote: > Hi, > > On Fri, Dec 09, 2016 at 03:52:32AM +0100, David Sommerseth wrote: >> - Instead of checking the complete in_addr_t (which lacked proper htonl()), >> just do a simple peek at the last byte which contains the first octet >> of an IP address or subnet mask. > > Have you *tested* this on a non-intel platform? > > I know that I said on IRC that we might do it that way, but looking > at stuff like print_in_addr_t() I'm not sure our usage of in_addr_t is > consistant with "normal usage of it" (namely, always in network byte > order). I don't have access to any big-endian machines. I might still have access to a virtual POWER8 machine, but that is configured in little-endian mode - so no difference from the Intel platform. After I sent this patch, I started to ponder on if there were any performance reasons why to change the in_addr_t check. And I've come to the conclusion I doubt it makes much of a difference. First of all, an IPv4 address is 32-bits, which is the smallest CPU variants we do support. So 0xff00 or 0xff will just be copied into a 32-bit register anyway. Secondly, checking for just 0xff will remove one AND operation on the CPU and the mov(e) operation will just be slightly different as it only loads 8 bits from a memory region vs 32 bits. So we might end up with or without saving just a couple of CPU cycles. And when considering that this is code paths which is not called very often, it starts to smell very much like pointless micro-optimization. I suggest we leave the in_addr_t parsing unchanged, and fix the message handling to differentiate between --ifconfig and --ifconfig-push. And after 2.4.0, we can look at the in_addr_t tackling to see if it behaves correctly on both big- and little-endian. If it doesn't behave well then we'll fix it. Thoughts? -- kind regards, David Sommerseth OpenVPN Technologies, Inc signature.asc Description: OpenPGP digital signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve ifconfig_sanity_check()
Hi, On Fri, Dec 09, 2016 at 03:52:32AM +0100, David Sommerseth wrote: > - Instead of checking the complete in_addr_t (which lacked proper htonl()), > just do a simple peek at the last byte which contains the first octet > of an IP address or subnet mask. Have you *tested* this on a non-intel platform? I know that I said on IRC that we might do it that way, but looking at stuff like print_in_addr_t() I'm not sure our usage of in_addr_t is consistant with "normal usage of it" (namely, always in network byte order). gert -- USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany g...@greenie.muc.de fax: +49-89-35655025g...@net.informatik.tu-muenchen.de signature.asc Description: PGP signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Improve ifconfig_sanity_check()
- Instead of checking the complete in_addr_t (which lacked proper htonl()), just do a simple peek at the last byte which contains the first octet of an IP address or subnet mask. - Improve error messages to also report errornous IP address usage when being in TOP_SUBNET - Improve the TAP check too, providing the IP address used instead of the subnet mask Signed-off-by: David Sommerseth --- src/openvpn/tun.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 8df3489..07748b1 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -304,18 +304,28 @@ void ifconfig_sanity_check (bool tun, in_addr_t addr, int topology) { struct gc_arena gc = gc_new (); - const bool looks_like_netmask = ((addr & 0xFF00) == 0xFF00); + const bool looks_like_netmask = (((unsigned char *)&addr)[3] == 0xff); + if (tun) { if (looks_like_netmask && (topology == TOP_NET30 || topology == TOP_P2P)) - msg (M_WARN, "WARNING: Since you are using --dev tun with a point-to-point topology, the second argument to --ifconfig must be an IP address. You are using something (%s) that looks more like a netmask. %s", +{ + msg (M_WARN, "WARNING: Since you are using --dev tun with a point-to-point topology, the second argument to --ifconfig must be an IP address. You are using something (%s) that looks more like a netmask. %s", + print_in_addr_t (addr, 0, &gc), + ifconfig_warn_how_to_silence); +} + else if (!looks_like_netmask && topology == TOP_SUBNET) +{ + msg (M_WARN, "WARNING: Since you are using --dev tun with subnet topology, the second argument to --ifconfig must be a netmask, for example something like 255.255.255.0. You are using something (%s) that looks more like an IP address. %s", print_in_addr_t (addr, 0, &gc), ifconfig_warn_how_to_silence); +} } else /* tap */ { if (!looks_like_netmask) - msg (M_WARN, "WARNING: Since you are using --dev tap, the second argument to --ifconfig must be a netmask, for example something like 255.255.255.0. %s", + msg (M_WARN, "WARNING: Since you are using --dev tap, the second argument to --ifconfig must be a netmask, for example something like 255.255.255.0. You are using something (%s) that looks more like an IP address. %s", + print_in_addr_t (addr, 0, &gc), ifconfig_warn_how_to_silence); } gc_free (&gc); -- 1.8.3.1 -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel