Received-SPF: pass (google.com: domain of linux-crypto-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: <CACXcFmnPumpkfLLzzjqkBmxwtpMa0izNj3LOtf2ycTugAKAUwQ@mail.gmail.com>
 <CAHmME9pUW1o_QPfs45Q0JWucA5Qu1jhgMV7x2PycxosYV2wV7A@mail.gmail.com>
 <CACXcFmk049OXc16ynjHBa+OSEOMYB=nYE1MDM_oM=Maf8bfcEA@mail.gmail.com> <YflZw7B1F41KpJ0K@mit.edu>
In-Reply-To: <YflZw7B1F41KpJ0K@mit.edu>
From:   Sandy Harris <sandyinchina@gmail.com>
Date:   Thu, 3 Feb 2022 11:51:25 +0800
Message-ID: <CACXcFmkAiqg--ibgMNUQ_JWaK9RuskH1sWgUya0ZpmHxAKPyPg@mail.gmail.com>
Subject: Re: [PATCH] random.c Remove locking in extract_buf()
To:     "Theodore Ts'o" <tytso@mit.edu>
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        m@ib.tc, Herbert Xu <herbert@gondor.apana.org.au>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > Yes, but the right way to do that is to lock the chacha context
> > in the reseed function and call extract_buf() while that lock
> > is held. I'll send a patch for that soon.
>
> Extract_buf() is supposed to be able to reliably generate high quality
> randomness; that's why we use it for the chacha reseed.  If
> extract_buf() can return return the same value for two parallel calls
> to extract_buf(), that's a Bad Thing.

I agree completely.

> For example, suppose there were
> two chacha contexts reseeding using extract_buf(), and they were
> racing against each other on two different CPU's.  Having two of them
> reseed with the same value would be a cryptographic weakness.

This confuses me a bit. Are you saying two CPUs can have
different primary chacha contexts but reseed from the same
input pool? Why? Reading the code, I thought there'd be
only one primary crng & others would reseed from it.

> NACK to both patches.

OK, but as Mike wrote in the thread he started about his
proposed lockless driver:

" It is highly unusual that /dev/random is allowed to degrade the
" performance of all other subsystems - and even bring the
" system to a halt when it runs dry.  No other kernel feature
" is given this dispensation,

I don't think a completely lockless driver is at all a good
idea & I think he overstates the point a bit in the quoted
text. But I do think he has a point. Locking the input pool
while extract_buf() reads & hashes it seems wrong to me
because it can unnecessarily block other processes.

crng_reseed() already locks the crng context. My patch
(which I probably will not now write since it has already got
a NACK) would make it call extract_buf() while holding that
lock, which prevents any problem of duplicate outputs but
avoids locking the input pool during the hash.

If my proposed patch would be unacceptable, it seems
worth asking if there is a better way to eliminate the
unnecessary lock.