Re: [Openvpn-devel] [PATCH] Improve ifconfig_sanity_check()

2016-12-09 Thread David Sommerseth
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()

2016-12-09 Thread Gert Doering
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()

2016-12-08 Thread David Sommerseth
- 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 *))[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, ),
+   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, ),
 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, ),
 ifconfig_warn_how_to_silence);
 }
   gc_free ();
-- 
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