| Message ID | 20220413115411.21489-2-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 Wed, Apr 13, 2022 at 6:55 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > The addition of random_get_entropy_fallback() provides access to > whichever time source has the highest frequency, which is useful for > gathering entropy on platforms without available cycle counters. It's > not necessarily as good as being able to quickly access a cycle counter > that the CPU has, but it's still something, even when it falls back to > being jiffies-based. > > In the event that a given arch does not define get_cycles(), falling > back to the get_cycles() default implementation that returns 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: Theodore Ts'o <tytso@mit.edu> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > include/linux/timex.h | 8 ++++++++ > kernel/time/timekeeping.c | 10 ++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/include/linux/timex.h b/include/linux/timex.h > index 5745c90c8800..fbbe34226044 100644 > --- a/include/linux/timex.h > +++ b/include/linux/timex.h > @@ -62,6 +62,8 @@ > #include <linux/types.h> > #include <linux/param.h> > > +extern unsigned long random_get_entropy_fallback(void); > + > #include <asm/timex.h> > > #ifndef random_get_entropy > @@ -74,8 +76,14 @@ > * > * By default we use get_cycles() for this purpose, but individual > * architectures may override this in their asm/timex.h header file. > + * If a given arch does not have get_cycles(), then we fallback to 'does not have a usable get_cycles(), ...' as clearly some arches have get_cycles() and yet still need a fallback. Why not handle the 'if get_cycles() returns 0 do the fallback' within a weak random_get_entropy() function? Then more arches don't need any random_get_entropy() implementation. Rob
Hi Rob, On Wed, Apr 13, 2022 at 4:32 PM Rob Herring <robh@kernel.org> wrote: > 'does not have a usable get_cycles(), ...' as clearly some arches have > get_cycles() and yet still need a fallback. > > Why not handle the 'if get_cycles() returns 0 do the fallback' within > a weak random_get_entropy() function? Then more arches don't need any > random_get_entropy() implementation. No, this doesn't really work. Actually, most archs don't need a random_get_entropy() function, because it exists in asm-generic doing the thing we want. So that's taken care of. But weak functions as you suggested would be quite suboptimal, because on, e.g. x86, what we have now gets inlined into a single rdtsc instruction. Also, the relation between get_cycles() and random_get_entropy() doesn't always hold; some archs may not have a working get_cycles() function but do have a path for a random_get_entropy(). Etc, etc. So I'm pretty sure that this commit is really the most simple and optimal thing to do. I really don't want to go the weak functions route. Jason
On Wed, Apr 13, 2022 at 01:54:01PM +0200, Jason A. Donenfeld wrote: > The addition of random_get_entropy_fallback() provides access to > whichever time source has the highest frequency, which is useful for > gathering entropy on platforms without available cycle counters. It's > not necessarily as good as being able to quickly access a cycle counter > that the CPU has, but it's still something, even when it falls back to > being jiffies-based. > > In the event that a given arch does not define get_cycles(), falling > back to the get_cycles() default implementation that returns 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: Theodore Ts'o <tytso@mit.edu> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > include/linux/timex.h | 8 ++++++++ > kernel/time/timekeeping.c | 10 ++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/include/linux/timex.h b/include/linux/timex.h > index 5745c90c8800..fbbe34226044 100644 > --- a/include/linux/timex.h > +++ b/include/linux/timex.h > @@ -62,6 +62,8 @@ > #include <linux/types.h> > #include <linux/param.h> > > +extern unsigned long random_get_entropy_fallback(void); Hi I'm surprised this didn't trigger checkpatch to warn. From coding-style: 6.1) Function prototypes Do not use the ``extern`` keyword with function declarations as this makes lines longer and isn't strictly necessary. Thanks!
Hi Russell, On Thu, Apr 14, 2022 at 12:12 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > I'm surprised this didn't trigger checkpatch to warn. From > coding-style: > > 6.1) Function prototypes > Do not use the ``extern`` keyword with function declarations as this makes > lines longer and isn't strictly necessary. Okay, will do for v+1. Jason
On Thu, Apr 14, 2022 at 12:38:49AM +0200, Jason A. Donenfeld wrote: > Hi Rob, > > On Wed, Apr 13, 2022 at 4:32 PM Rob Herring <robh@kernel.org> wrote: > > 'does not have a usable get_cycles(), ...' as clearly some arches have > > get_cycles() and yet still need a fallback. > > > > Why not handle the 'if get_cycles() returns 0 do the fallback' within > > a weak random_get_entropy() function? Then more arches don't need any > > random_get_entropy() implementation. > > No, this doesn't really work. Actually, most archs don't need a > random_get_entropy() function, because it exists in asm-generic doing > the thing we want. So that's taken care of. But weak functions as you > suggested would be quite suboptimal, because on, e.g. x86, what we > have now gets inlined into a single rdtsc instruction. Also, the > relation between get_cycles() and random_get_entropy() doesn't always > hold; some archs may not have a working get_cycles() function but do > have a path for a random_get_entropy(). Etc, etc. So I'm pretty sure > that this commit is really the most simple and optimal thing to do. I > really don't want to go the weak functions route. Is random_get_entropy() a hot path? It doesn't have to be a weak function, but look at it this way. We have the following possibilities for what random_get_entropy() does: - get_cycles() - get_cycles() but returns 0 sometimes - returns 0 - something else You're handling the 3rd case. For the 2nd case, that's riscv, arm, nios2, and x86. That's not a lot, but is 2 or 3 of the most widely used architectures. Is it really too much to ask to support the 2nd case in the generic code/header? Rob
On April 14, 2022 1:41:38 PM PDT, Rob Herring <robh@kernel.org> wrote: >On Thu, Apr 14, 2022 at 12:38:49AM +0200, Jason A. Donenfeld wrote: >> Hi Rob, >> >> On Wed, Apr 13, 2022 at 4:32 PM Rob Herring <robh@kernel.org> wrote: >> > 'does not have a usable get_cycles(), ...' as clearly some arches have >> > get_cycles() and yet still need a fallback. >> > >> > Why not handle the 'if get_cycles() returns 0 do the fallback' within >> > a weak random_get_entropy() function? Then more arches don't need any >> > random_get_entropy() implementation. >> >> No, this doesn't really work. Actually, most archs don't need a >> random_get_entropy() function, because it exists in asm-generic doing >> the thing we want. So that's taken care of. But weak functions as you >> suggested would be quite suboptimal, because on, e.g. x86, what we >> have now gets inlined into a single rdtsc instruction. Also, the >> relation between get_cycles() and random_get_entropy() doesn't always >> hold; some archs may not have a working get_cycles() function but do >> have a path for a random_get_entropy(). Etc, etc. So I'm pretty sure >> that this commit is really the most simple and optimal thing to do. I >> really don't want to go the weak functions route. > >Is random_get_entropy() a hot path? > > >It doesn't have to be a weak function, but look at it this way. We have >the following possibilities for what random_get_entropy() does: > >- get_cycles() >- get_cycles() but returns 0 sometimes >- returns 0 >- something else > >You're handling the 3rd case. > >For the 2nd case, that's riscv, arm, nios2, and x86. That's not a lot, >but is 2 or 3 of the most widely used architectures. Is it really too >much to ask to support the 2nd case in the generic code/header? > >Rob It goes into interrupts, which means it is latency critical.
diff --git a/include/linux/timex.h b/include/linux/timex.h index 5745c90c8800..fbbe34226044 100644 --- a/include/linux/timex.h +++ b/include/linux/timex.h @@ -62,6 +62,8 @@ #include <linux/types.h> #include <linux/param.h> +extern unsigned long random_get_entropy_fallback(void); + #include <asm/timex.h> #ifndef random_get_entropy @@ -74,8 +76,14 @@ * * By default we use get_cycles() for this purpose, but individual * architectures may override this in their asm/timex.h header file. + * If a given arch does not have get_cycles(), then we fallback to + * using random_get_entropy_fallback(). */ +#ifdef get_cycles #define random_get_entropy() ((unsigned long)get_cycles()) +#else +#define random_get_entropy() random_get_entropy_fallback() +#endif #endif /* diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index dcdcb85121e4..7cd2ec239cae 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -17,6 +17,7 @@ #include <linux/clocksource.h> #include <linux/jiffies.h> #include <linux/time.h> +#include <linux/timex.h> #include <linux/tick.h> #include <linux/stop_machine.h> #include <linux/pvclock_gtod.h> @@ -2380,6 +2381,15 @@ static int timekeeping_validate_timex(const struct __kernel_timex *txc) return 0; } +/** + * random_get_entropy_fallback - Returns the raw clock source value, + * used by random.c for platforms with no valid random_get_entropy(). + */ +unsigned long random_get_entropy_fallback(void) +{ + return tk_clock_read(&tk_core.timekeeper.tkr_mono); +} +EXPORT_SYMBOL_GPL(random_get_entropy_fallback); /** * do_adjtimex() - Accessor function to NTP __do_adjtimex function
The addition of random_get_entropy_fallback() provides access to whichever time source has the highest frequency, which is useful for gathering entropy on platforms without available cycle counters. It's not necessarily as good as being able to quickly access a cycle counter that the CPU has, but it's still something, even when it falls back to being jiffies-based. In the event that a given arch does not define get_cycles(), falling back to the get_cycles() default implementation that returns 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: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- include/linux/timex.h | 8 ++++++++ kernel/time/timekeeping.c | 10 ++++++++++ 2 files changed, 18 insertions(+)