Gert Doering <g...@greenie.muc.de> writes:
> 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.This is exactly the NetBSD code I was mentioning. Here's the mail: https://sourceforge.net/p/openvpn/mailman/message/35887533/ (weird, the mail content appears to be truncated on the web interface, but at least you can get the patch in "Attachments"). >> > 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) Neither do I, I'm no sparcv9 hacker. > ... and I can see that the "master" version is going to fail on > unaligned packets... so, needs fixing. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
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
