| Message ID | 20220423212623.1957011-9-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | archs/random: fallback to best raw ktime when no cycle counter | expand |
On Sat, 23 Apr 2022 14:26:14 PDT (-0700), Jason@zx2c4.com wrote: > In the event that random_get_entropy() can't access a cycle counter or > similar, falling back to returning 0 is really not the best we can do. > Instead, at least calling random_get_entropy_fallback() would be > preferable, because that always needs to return _something_, even > falling back to jiffies eventually. It's not as though > random_get_entropy_fallback() is super high precision or guaranteed to > be entropic, but basically anything that's not zero all the time is > better than returning zero all the time. Makes sense: we had an architecturally-mandated timer at the time, but we don't any more. Every real implementation has a timer right now, but that may change in the future so it doesn't hurt to fix it before it's broken. > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Paul Walmsley <paul.walmsley@sifive.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > arch/riscv/include/asm/timex.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index 507cae273bc6..d6a7428f6248 100644 > --- a/arch/riscv/include/asm/timex.h > +++ b/arch/riscv/include/asm/timex.h > @@ -41,7 +41,7 @@ static inline u32 get_cycles_hi(void) > static inline unsigned long random_get_entropy(void) > { > if (unlikely(clint_time_val == NULL)) > - return 0; > + return random_get_entropy_fallback(); > return get_cycles(); > } > #define random_get_entropy() random_get_entropy() Fine for me if this goes in via some other tree, but also happy to take it via the RISC-V tree if you'd like. IMO we could just call this a fix, maybe Fixes: aa9887608e77 ("RISC-V: Check clint_time_val before use") (but that just brought this back, so there's likely older kernels broken too). Shouldn't be breaking any real hardware, though, so no rush on my end. Acked-by: Palmer Dabbelt <palmer@rivosinc.com> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> Thanks!
Hi Palmer, On Mon, Apr 25, 2022 at 4:55 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > Fine for me if this goes in via some other tree, but also happy to take > it via the RISC-V tree if you'd like. I'm going to take this series through the random.git tree, as I've got things that build on top of it for random.c slated for 5.19. > IMO we could just call this a > fix, maybe > > Fixes: aa9887608e77 ("RISC-V: Check clint_time_val before use") > > (but that just brought this back, so there's likely older kernels broken > too). Shouldn't be breaking any real hardware, though, so no rush on my > end. That'd be fine with me, but it'd involve also backporting the timekeeping patch, which adds a new API, so maybe we better err on the side of caution with that new code. > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> Thanks for the review. > Makes sense: we had an architecturally-mandated timer at the time, but > we don't any more. That's too bad. Out of curiosity, what happened? Was that deemed too expensive for certain types of chips that western digital wanted to produce for their hard drives, or some really constrained use case like that? Jason
On Mon, 25 Apr 2022 08:02:49 PDT (-0700), Jason@zx2c4.com wrote: > Hi Palmer, > > On Mon, Apr 25, 2022 at 4:55 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> Fine for me if this goes in via some other tree, but also happy to take >> it via the RISC-V tree if you'd like. > > I'm going to take this series through the random.git tree, as I've got > things that build on top of it for random.c slated for 5.19. > >> IMO we could just call this a >> fix, maybe >> >> Fixes: aa9887608e77 ("RISC-V: Check clint_time_val before use") >> >> (but that just brought this back, so there's likely older kernels broken >> too). Shouldn't be breaking any real hardware, though, so no rush on my >> end. > > That'd be fine with me, but it'd involve also backporting the > timekeeping patch, which adds a new API, so maybe we better err on the > side of caution with that new code. wFM. Like I said this isn't going to break any existing hardware, and anyone trying to ship something without the timers is likely going to be in for way more trouble than this so will probably be stuck with newer kernels anyway. >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> > > Thanks for the review. > >> Makes sense: we had an architecturally-mandated timer at the time, but >> we don't any more. > > That's too bad. Out of curiosity, what happened? Was that deemed too > expensive for certain types of chips that western digital wanted to > produce for their hard drives, or some really constrained use case > like that? No idea, but it was at the beginning of the "everything is optional"-ification of the ISA so I'm guessing it's just part of that.
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h index 507cae273bc6..d6a7428f6248 100644 --- a/arch/riscv/include/asm/timex.h +++ b/arch/riscv/include/asm/timex.h @@ -41,7 +41,7 @@ static inline u32 get_cycles_hi(void) static inline unsigned long random_get_entropy(void) { if (unlikely(clint_time_val == NULL)) - return 0; + return random_get_entropy_fallback(); return get_cycles(); } #define random_get_entropy() random_get_entropy()
In the event that random_get_entropy() can't access a cycle counter or similar, falling back to returning 0 is really not the best we can do. Instead, at least calling random_get_entropy_fallback() would be preferable, because that always needs to return _something_, even falling back to jiffies eventually. It's not as though random_get_entropy_fallback() is super high precision or guaranteed to be entropic, but basically anything that's not zero all the time is better than returning zero all the time. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Paul Walmsley <paul.walmsley@sifive.com> Cc: Palmer Dabbelt <palmer@dabbelt.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- arch/riscv/include/asm/timex.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)