Hi,
On Sat, Jun 10, 2017 at 07:58:13PM +0200, Jeremie Courreges-Anglas wrote:
> OpenBSD:
>
> struct ip {
> #if _BYTE_ORDER == _LITTLE_ENDIAN
> u_int ip_hl:4, /* header length */
> ip_v:4; /* version */
> #endif
> #if _BYTE_ORDER == _BIG_ENDIAN
> u_int ip_v:4, /* version */
> ip_hl:4; /* header length */
> #endif
> ...
>
> u_char -> u_int. Dunno if it matters to the compiler, I can take a look
> later.
I'm fairly sure it does matter. An int is likely to be at least 16 bit,
maybe 32 bit, so the compiler can assume "word aligned" - and *boom*.
(Wonder why it doesn't crash for Mator, as our buffer alignment in memory
should be fairly consistent - it might depend on crypto and compression
options, though)
> > old:
> > if (iph->ip_v == 6)
> >
> > new:
> > if (OPENVPN_IPH_GET_VER(*buf) == 6)
> >
> > this is an existing macro which takes an int argument, shifting and
> > masking the right bits, and since *buf is "uint8_t *buf", this should
> > both be correct and alignment-ignorant.
>
> See my proposed patch, also in this thread.
"Copying around 20 bytes to ensure a single-byte access is really a
single-byte access" is definitely *not* the right way here. So NAK on
that patch.
It will stop the crashes, of course, but since no other parts of
struct ip are ever needed, it's just waste of CPU cycles.
> I merely copy/pasted the NetBSD code.
Uh? Is this from the NetBSD ports tree? Because our tun.c does not
have a single memcpy() today... :-)
Actually, my tun.c's TARGET_NETBSD version of write_tun() has something
similar to what I suggested:
struct openvpn_iphdr *iph;
iph = (struct openvpn_iphdr *) buf;
if (OPENVPN_IPH_GET_VER(iph->version_len) == 6)
... slightly more speaking, and since "openvpn_iphdr" is a locally
defined structure with "uchar version_len" at the very beginning, this
amounts to the same thing.
> > It might be less efficient than accessing the bit field - but then, it's
> > the same work to do, so hopefully the compiler is smart enough to compile
> > it to the same instructions.
>
> master (32 bits access):
> 0x0000000000081844 <write_tun_header+100>: ld [ %i1 ], %g1
> 0x0000000000081848 <write_tun_header+104>: sethi %hi(0xf0000000), %g2
> 0x000000000008184c <write_tun_header+108>: sethi %hi(0x60000000), %g3
> 0x0000000000081850 <write_tun_header+112>: and %g1, %g2, %g1
> 0x0000000000081854 <write_tun_header+116>: cmp %g1, %g3
>
> patched:
> 0x0000000000081844 <write_tun_header+100>: ldub [ %i1 ], %g1
> 0x0000000000081848 <write_tun_header+104>: srl %g1, 4, %g1
> 0x000000000008184c <write_tun_header+108>: cmp %g1, 6
Sounds like an improvement, at least in code size :-) (but I have no
idea how the performance of these operations compares)
... and I can see that the "master" version is going to fail on
unaligned packets... so, needs fixing.
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-35655025 g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
