Re: Patches fixing unaligned access in the networking code

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Mon, 27 May 2019 21:36:28 +0200
    From:        Kamil Rytarowski <n54%gmx.com@localhost>
    Message-ID:  <882a832c-b37f-0e12-294f-9e46c145dbde%gmx.com@localhost>

  | http://netbsd.org/~kamil/patch-00115-tcp_input.2.txt

With 2 (minor) caveats this one looks fine to me.

The first (and lesser) is that the name get_unaligned32() is
ugly -- and assumes the data is unaligned which in practice it
almost never is - senders take care to align the data for any
rational receiver (and our implementation is rational in that way).

The original (current) code uses

	*(u_int32_t *)optp

so how about making the replacement be

	uint32(optp)

which is less chars, ties the function name to the type name,
and doesn't imply that the data is unaligned.   (deleting the _
between u and int is just the more modern style, right?  if not
put it back...)

It might even be worth adding static inline functions like that
(and uint16() and the signed variants perhaps) to one of the header
files, do they're available throughout the kernel.


And second, before doing the change, it would be nice to work
out which implementation of fetching a 32 bit word from an
arbitrary pointer gcc optimises away best in two cases:

1. where the processor does not have any alignment constraints
2. where the data bytes are already aligned

(the 2nd of those might be irrelevant, the cost to find out whether
the pointer is aligned probably exceeds any benefit if it is).

That is, where one implementation is the one you give, and the second
is the old stile (endian dependant ... though here since the data is
being compared to htonl() of a const, we can assume big endian - ie:
network byte order, and don't need an #ifdef to do it little endian
instead (I think, needs checking... perhaps the htonl() also would need
to go away)

	uint8_t *cp = p;

	return (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3];

Then use whichever implementation gcc is able to optimise into the
same code it currently generates on processors that have no alignment
constraints.

  | http://netbsd.org/~kamil/patch-00117-netinet6.txt

It is hard ti imagine an implementation where this would make a
difference, but if that's required to meet the letter of the C
standard, then go ahead ... The IPv6 addr struct is designed to
always be packed with any even half rational compiler.  Saying so
can't hurt anything.

kre