On Tue, Jul 17, 2018 at 6:26 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Tue, Jul 17, 2018 at 6:54 AM Arnd Bergmann <a...@arndb.de> wrote:
>>
>> The newly added arch_get_random_int() call was done incorrectly,
>> using the output only if rdrand hardware was /not/ available. The
>> compiler points out that the data is uninitialized in this case:
>
> Ack.
>
> Except:
>
>> for (b = bytes ; b > 0 ; b -= sizeof(__u32), i++) {
>> - if (arch_get_random_int(&t))
>> + if (!arch_get_random_int(&t))
>> continue;
>> buf[i] ^= t;
>> }
>
> Why not just make that "continue" be a "break"? If you fail once, you
> will fail the next time too (whether the arch just doesn't support it
> at all, or whether the HW entropy is just temporarily exhausted).
>
> So no point in "continue". Just give up. Maybe it will work much
> later, but not _immediately_.
Makes sense. I found that a bit odd, but didn't think much of it:
on all architectures other than x86, arch_get_random_int()
will return a hardcoded 0 from an inline function, so the compiler
should be able to drop the entire loop either way.
On x86 however, it will keep evaluating arch_has_random()
pointlessly.
> (I don't actually see the commit in question - it's not in my pile of
> emails only in linux-next, maybe there's some reason further down
> prefers "continue" and the whole loop be finished).
I didn't see one.
Arnd
