Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220201161342.154666-1-Jason@zx2c4.com> <Yflyfk8BbGQvN3os@kroah.com>
In-Reply-To: <Yflyfk8BbGQvN3os@kroah.com>
From:   Sandy Harris <sandyinchina@gmail.com>
Date:   Thu, 3 Feb 2022 14:45:00 +0800
Message-ID: <CACXcFmkWBKCHLoqHZXk3_aVY+U8Ct8ZfOCe2T0FS0SrFSeFsjw@mail.gmail.com>
Subject: Re: [PATCH] random: use computational hash for entropy extraction
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        "Theodore Ts'o" <tytso@mit.edu>,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> > Another aspect of the current mix_pool_bytes() function is that, while
> > its performance was arguably competitive for the time in which it was
> > created, it's no longer considered so. This patch improves performance
> > significantly: ...
...
> From a "this looks sane by reading the code" type of review here's my:
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I agree, it looks sane. & worthwhile.

Should we be concerned about relying too much on one piece of crypto,
though? Blake was derived from Chacha, which we already use in the
crng & we already use Blake in extract_buf().

Also, the Blake people now have Blake3 which they say is faster.
https://github.com/BLAKE3-team/BLAKE3
Why are we sticking with Blake2?

If overhead was the only objection to the current mixer, we
could probably speed it up some by eliminating indirection
as in my code below:

/***********************************************************
 * main function for mixing into input pool
 *
 * modified version of
 * mix_pool_bytes(struct entropy_store *r, const void *in, int nbytes)
 * from drivers/char/random.c
 *
 * always mix to input pool
 * (as do most or all calls in current driver)
 * so struct entropy_store *r is not needed
 * my version is
 * mix_to_input(const void *in, int nbytes)
 *
 * make things constants or globals wherever possible
 * instead of struct components
***********************************************************/

static u32 const twist_table[8] = {
    0x00000000, 0x3b6e20c8, 0x76dc4190, 0x4db26158,
    0xedb88320, 0xd6d6a3e8, 0x9b64c2b0, 0xa00ae278 };

#define tap1    104
#define tap2     76
#define tap3     51
#define tap4     25
#define tap5      1

#define wordmask    (INPUT_POOL_WORDS - 1)

/*
 * I see no initialisation of these in random.c
 * but initialise here anyway
 */
static int input_rotate = 1 ;
static int add_ptr = 3 ;

static void mix_to_input(const void *in, int nbytes)
{
        int i ;
        u32 w ;                    // __u32 in random.c
        const char *bytes = in;
        u32 *pool = input_pool.data ;

        spin_lock( &input_pool.lock ) ;

    i = add_ptr;

    /* mix one byte at a time to simplify size handling and churn faster */
    while (nbytes--) {
        w = rol32(*bytes, input_rotate);
                bytes++ ;

        i = (i - 1) & wordmask;

        /* XOR in the various taps */
        w ^= pool[i];
        w ^= pool[(i + tap1) & wordmask];
        w ^= pool[(i + tap2) & wordmask];
        w ^= pool[(i + tap3) & wordmask];
        w ^= pool[(i + tap4) & wordmask];
        w ^= pool[(i + tap5) & wordmask];

        /* Mix the result back in with a twist */
        pool[i] = (w >> 3) ^ twist_table[w & 7];

        /*
         * Normally, we add 7 bits of rotation to the pool.
         * At the beginning of the pool, add an extra 7 bits
         * rotation, so that successive passes spread the
         * input bits across the pool evenly.
         */
        input_rotate = (input_rotate + (i ? 7 : 14)) & 31;
    }
    add_ptr = i;

        spin_unlock( &input_pool.lock ) ;
}