Re: [PATCH v2 net-next 0/4] Introduce The SipHash PRF

Hi Eric,

Thanks for round two. I'll address these. Comments are inline below.

On Sat, Jan 7, 2017 at 8:54 PM, Eric Biggers [off-list ref] wrote:

Hi Jason, thanks for doing this!  Yes, I had gotten a little lost in the earlier
discussions about the 'random' driver and other potential users of SipHash.  I
agree with the approach to just introduce the two uses in net/ to start with,
and introduce more users later.  The changes from v1 to v2 look good too.

Indeed the initial patchset was _insane_ and the discussion became
sprawling and impossible. Everybody suggested I do baby steps, so
voila, there's this more manageable patchset now.

Now that the HalfSipHash patch is Cc'ed to me too I do have one other small
suggestion which is that this:

        #if BITS_PER_LONG == 64
        typedef siphash_key_t hsiphash_key_t;
        #define HSIPHASH_ALIGNMENT SIPHASH_ALIGNMENT
        #else
        typedef struct {
                u32 key[2];
        } hsiphash_key_t;
        #define HSIPHASH_ALIGNMENT __alignof__(u32)
        #endif

could cause confusion if someone accidentally uses 'siphash_key_t' instead of
'hsiphash_key_t', as their code would compile fine on a 64-bit platform but
would fail to compile on a 32-bit platform.  I think there should just always be
a hsiphash_key_t struct defined, and it can use unsigned long (no need for an
#ifdef):

        #define HSIPHASH_ALIGNMENT __alignof__(unsigned long)
        typedef struct {
                unsigned long key[2];
        } hsiphash_key_t;

Good idea. Will adjust. That makes things a lot simpler.

There's also a small error in Documentation/siphash.txt: hsiphash() is shown as
taking siphash_key_t instead of hsiphash_key_t.

Arg, nice catch, fixing.

The uses in net look good too.  Something to watch out for is accidentally
defining the structs in a way that leaves internal padding bytes, which could
theoretically take on any value and cause the same input to produce different
hashes.  But AFAICS, in the proposed patch all the structs are laid out
properly, so that won't happen.

Indeed that's something closely examined. In fact, originally, just to
be careful, I was using __packed, but David pointed out that using
__packed makes gcc resort to byte-by-byte assignment, even if the
alignment would otherwise be natural. So, instead I just made sure to
list the members in descending order of size, and made sure to use
offsetendof instead of sizeof. I'll be sure to document this
precaution in the Documentation/siphash.txt file for the next version,
alongside a simple example.

'net_secret' could also be marked as __read_mostly, like the keys in
syncookies.c, I suppose; it may not matter much.

Good point. Fixing.

New version coming your way soon.

Thanks again,
Jason