On Fri, Feb 04, 2022 at 02:53:24PM +0100, Jason A. Donenfeld wrote: > What if we're wrong and the above is nonsense? It's not, but let's > assume we don't want the actual _behavior_ of the code to change much. > Currently that behavior is not extracting from the input pool until it > has 128 bits of entropy in it. With the old algorithm, we'd hit that > magic 128 number after roughly 256 calls to credit_entropy_bits(1). So, > we can retain more or less the old behavior by waiting to extract from > the input pool until it hits 256 bits of entropy using the new code. For > people concerned about this change, it means that there's not that much > practical behavioral change. And for folks actually trying to model > the behavior rigorously, it means that we have an even higher margin > against attacks. I tested this, and it actually was 205 calls prior to patch 1 in this series, and 267 calls after patch 1. That's in contrast to 256 calls after this patch. Not a big difference, but this is going to result in ~25% more single-bit calls being needed compared to the old version. It's unclear whether you're arguing that's basically the same, or whether you thought it was a smaller difference. > +enum { > POOL_BITS = BLAKE2S_HASH_SIZE * 8, > - POOL_BITSHIFT = ilog2(POOL_BITS), > - POOL_MIN_BITS = POOL_BITS / 2, > - > - /* To allow fractional bits to be tracked, the entropy_count field is > - * denominated in units of 1/8th bits. */ > - POOL_ENTROPY_SHIFT = 3, > -#define POOL_ENTROPY_BITS() (input_pool.entropy_count >> POOL_ENTROPY_SHIFT) > - POOL_FRACBITS = POOL_BITS << POOL_ENTROPY_SHIFT, > - POOL_MIN_FRACBITS = POOL_MIN_BITS << POOL_ENTROPY_SHIFT > + POOL_MIN_BITS = POOL_BITS /* No point in settling for less. */ > }; Doesn't the default value of random_write_wakeup_bits need to be increased to this value? Otherwise, the pool can get stuck with entropy_count greater than or equal to random_write_wakeup_bits (192) but less than POOL_MIN_BITS (256). In fact, the only correct value of random_write_wakeup_bits will be 256, i.e. the entire pool. Perhaps it should no longer be configurable via /proc/sys? Note that there's also an off-by one bug that will need to be fixed: add_hwgenerator_randomness() checks entropy_count <= random_write_wakeup_bits rather than entropy_count < random_write_wakeup_bits as random_poll() does. Also: given all these considerations, perhaps the increase in POOL_MIN_BITS is best done in a separate patch? > + do { > + entropy_count = orig = READ_ONCE(input_pool.entropy_count); > + entropy_count = min(POOL_BITS, entropy_count + nbits); > + } while (cmpxchg(&input_pool.entropy_count, orig, entropy_count) != orig); This can be simplified slightly: do { orig = READ_ONCE(input_pool.entropy_count); entropy_count = min(POOL_BITS, orig + nbits); } while (cmpxchg(&input_pool.entropy_count, orig, entropy_count) != orig); - Eric