get_cycles from modular code in jitterentropy, was Re: [PATCH] clocksource: clint: Export clint_time_val for modules

get_cycles from modular code in jitterentropy, was Re: [PATCH] clocksource: clint: Export clint_time_val for modules

[Christoph Hellwig]

On Tue, Sep 29, 2020 at 11:56:18PM -0700, Palmer Dabbelt wrote:
> clint_time_val will soon be used by the RISC-V implementation of
> random_get_entropy(), which is a static inline function that may be used by
> modules (at least CRYPTO_JITTERENTROPY=m).

At very least this needs to be an EXPORT_SYMBOL_GPL.  But I really don't
think modules have any business using get_cycles, so I'd much rather
fix CRYPTO_JITTERENTROPY to be required to be build in.

> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> ---
>  drivers/clocksource/timer-clint.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index d17367dee02c..6cfe2ab73eb0 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -38,6 +38,7 @@ static unsigned int clint_timer_irq;
>  
>  #ifdef CONFIG_RISCV_M_MODE
>  u64 __iomem *clint_time_val;
> +EXPORT_SYMBOL(clint_time_val);
>  #endif
>  
>  static void clint_send_ipi(const struct cpumask *target)
> -- 
> 2.28.0.709.gb0816b6eb0-goog
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
---end quoted text---

Re: get_cycles from modular code in jitterentropy, was Re: [PATCH] clocksource: clint: Export clint_time_val for modules

[Stephan Mueller]

Am Freitag, 2. Oktober 2020, 08:49:05 CEST schrieb Christoph Hellwig:

Hi Christoph,

> On Tue, Sep 29, 2020 at 11:56:18PM -0700, Palmer Dabbelt wrote:
> > clint_time_val will soon be used by the RISC-V implementation of
> > random_get_entropy(), which is a static inline function that may be used
> > by
> > modules (at least CRYPTO_JITTERENTROPY=m).
> 
> At very least this needs to be an EXPORT_SYMBOL_GPL.  But I really don't
> think modules have any business using get_cycles, so I'd much rather
> fix CRYPTO_JITTERENTROPY to be required to be build in.

Changing CRYPTO_JITTERENTROPY from tistate to bool should be no problem.

I will provide a patch.

Ciao
Stephan

[PATCH] crypto: jitterentropy - bind statically into kernel

[Stephan Müller]

The RISC-V architecture is about to implement the callback
random_get_entropy with a function that is not exported to modules.
Thus, the Jitter RNG is changed to be only bound statically into the
kernel removing the option to compile it as module.

Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 094ef56ab7b4..5b20087b117f 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1853,7 +1853,7 @@ config CRYPTO_DRBG
 endif	# if CRYPTO_DRBG_MENU
 
 config CRYPTO_JITTERENTROPY
-	tristate "Jitterentropy Non-Deterministic Random Number Generator"
+	bool "Jitterentropy Non-Deterministic Random Number Generator"
 	select CRYPTO_RNG
 	help
 	  The Jitterentropy RNG is a noise that is intended
-- 
2.26.2

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Ard Biesheuvel]

On Sun, 4 Oct 2020 at 20:48, Stephan Müller <smueller@chronox.de> wrote:
>
> The RISC-V architecture is about to implement the callback
> random_get_entropy with a function that is not exported to modules.

Why is that? Wouldn't it be better to export the symbol instead?

> Thus, the Jitter RNG is changed to be only bound statically into the
> kernel removing the option to compile it as module.
>
> Reported-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 094ef56ab7b4..5b20087b117f 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1853,7 +1853,7 @@ config CRYPTO_DRBG
>  endif  # if CRYPTO_DRBG_MENU
>
>  config CRYPTO_JITTERENTROPY
> -       tristate "Jitterentropy Non-Deterministic Random Number Generator"
> +       bool "Jitterentropy Non-Deterministic Random Number Generator"
>         select CRYPTO_RNG
>         help
>           The Jitterentropy RNG is a noise that is intended
> --
> 2.26.2
>
>
>
>

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Palmer Dabbelt]

On Sun, 04 Oct 2020 14:16:10 PDT (-0700), ardb@kernel.org wrote:
> On Sun, 4 Oct 2020 at 20:48, Stephan Müller <smueller@chronox.de> wrote:
>>
>> The RISC-V architecture is about to implement the callback
>> random_get_entropy with a function that is not exported to modules.
>
> Why is that? Wouldn't it be better to export the symbol instead?

It's static inline (in our timex.h), so I thought we didn't need to export the
symbol?  Did this just arise because clint_time_val wasn't exported?  That was
fixed before the random_get_entropy() change landed in Linus' tree, so as far
as I know we should be OK here.

If I broke something here it seem better to fix this in the RISC-V port than by
just banning modular compilation of jitterentropy, as that seems like a useful
feature to me.

>> Thus, the Jitter RNG is changed to be only bound statically into the
>> kernel removing the option to compile it as module.
>>
>> Reported-by: Christoph Hellwig <hch@infradead.org>
>> Signed-off-by: Stephan Mueller <smueller@chronox.de>
>> ---
>>  crypto/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/crypto/Kconfig b/crypto/Kconfig
>> index 094ef56ab7b4..5b20087b117f 100644
>> --- a/crypto/Kconfig
>> +++ b/crypto/Kconfig
>> @@ -1853,7 +1853,7 @@ config CRYPTO_DRBG
>>  endif  # if CRYPTO_DRBG_MENU
>>
>>  config CRYPTO_JITTERENTROPY
>> -       tristate "Jitterentropy Non-Deterministic Random Number Generator"
>> +       bool "Jitterentropy Non-Deterministic Random Number Generator"
>>         select CRYPTO_RNG
>>         help
>>           The Jitterentropy RNG is a noise that is intended
>> --
>> 2.26.2
>>
>>
>>
>>

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Christoph Hellwig]

On Sun, Oct 04, 2020 at 11:16:10PM +0200, Ard Biesheuvel wrote:
> On Sun, 4 Oct 2020 at 20:48, Stephan M??ller <smueller@chronox.de> wrote:
> >
> > The RISC-V architecture is about to implement the callback
> > random_get_entropy with a function that is not exported to modules.
> 
> Why is that? Wouldn't it be better to export the symbol instead?

get_cycles is a low-level time keeping detail that really should not
be exported, and at least for RISC-V this would be the only modular
user.  Once that is sorted out I'll audit other common architectures
to drop the export, as it isn't something that should be used in ramdom
driver code.

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Ard Biesheuvel]

On Mon, 5 Oct 2020 at 08:19, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Oct 04, 2020 at 11:16:10PM +0200, Ard Biesheuvel wrote:
> > On Sun, 4 Oct 2020 at 20:48, Stephan M??ller <smueller@chronox.de> wrote:
> > >
> > > The RISC-V architecture is about to implement the callback
> > > random_get_entropy with a function that is not exported to modules.
> >
> > Why is that? Wouldn't it be better to export the symbol instead?
>
> get_cycles is a low-level time keeping detail that really should not
> be exported, and at least for RISC-V this would be the only modular
> user.  Once that is sorted out I'll audit other common architectures
> to drop the export, as it isn't something that should be used in ramdom
> driver code.

Fair enough.

But this means we should fix the jitterentropy driver rather than
sidestepping the issue by only allowing it to be built in a way where
we don't happen to notice that the symbol in question is not meant for
general consumption.

If jitterentropy is a special case, we could put a alternate
non-'static inline' version of random_get_entropy() in the core
kernel, and only export it if JITTER_ENTROPY is built as a module in
the first place. But I'd prefer it if jitterentropy switches to an API
that is suitable for driver consumption.

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Stephan Mueller]

Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:

Hi Ard,

> If jitterentropy is a special case, we could put a alternate
> non-'static inline' version of random_get_entropy() in the core
> kernel, and only export it if JITTER_ENTROPY is built as a module in
> the first place. But I'd prefer it if jitterentropy switches to an API
> that is suitable for driver consumption.

Which API do you have in mind? In user space, I use 
clock_gettime(CLOCK_REALTIME) which also considers the clock source.

Thanks
Stephan

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Christoph Hellwig]

[adding Thomas]

On Mon, Oct 05, 2020 at 08:40:25AM +0200, Stephan Mueller wrote:
> > If jitterentropy is a special case, we could put a alternate
> > non-'static inline' version of random_get_entropy() in the core
> > kernel, and only export it if JITTER_ENTROPY is built as a module in
> > the first place. But I'd prefer it if jitterentropy switches to an API
> > that is suitable for driver consumption.
> 
> Which API do you have in mind? In user space, I use 
> clock_gettime(CLOCK_REALTIME) which also considers the clock source.

We could probably add a kernel_clock_gettime which contains the
clock_gettime syscal implementation minus the put_timespec64.  Thomas,
is this something that fits your timekeeping vision?

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Ard Biesheuvel]

On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > If jitterentropy is a special case, we could put a alternate
> > non-'static inline' version of random_get_entropy() in the core
> > kernel, and only export it if JITTER_ENTROPY is built as a module in
> > the first place. But I'd prefer it if jitterentropy switches to an API
> > that is suitable for driver consumption.
>
> Which API do you have in mind? In user space, I use
> clock_gettime(CLOCK_REALTIME) which also considers the clock source.
>

AFAICT, that call is backed by ktime_get_real_ts64(), which is already
being exported to modules.

Could you please check whether that works for your driver?

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Christoph Hellwig]

On Mon, Oct 05, 2020 at 08:44:39AM +0200, Ard Biesheuvel wrote:
> On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <smueller@chronox.de> wrote:
> >
> > Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:
> >
> > Hi Ard,
> >
> > > If jitterentropy is a special case, we could put a alternate
> > > non-'static inline' version of random_get_entropy() in the core
> > > kernel, and only export it if JITTER_ENTROPY is built as a module in
> > > the first place. But I'd prefer it if jitterentropy switches to an API
> > > that is suitable for driver consumption.
> >
> > Which API do you have in mind? In user space, I use
> > clock_gettime(CLOCK_REALTIME) which also considers the clock source.
> >
> 
> AFAICT, that call is backed by ktime_get_real_ts64(), which is already
> being exported to modules.

Indeed. No need for my earlier idea..

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Stephan Mueller]

Am Montag, 5. Oktober 2020, 08:44:39 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <smueller@chronox.de> wrote:
> > Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:
> > 
> > Hi Ard,
> > 
> > > If jitterentropy is a special case, we could put a alternate
> > > non-'static inline' version of random_get_entropy() in the core
> > > kernel, and only export it if JITTER_ENTROPY is built as a module in
> > > the first place. But I'd prefer it if jitterentropy switches to an API
> > > that is suitable for driver consumption.
> > 
> > Which API do you have in mind? In user space, I use
> > clock_gettime(CLOCK_REALTIME) which also considers the clock source.
> 
> AFAICT, that call is backed by ktime_get_real_ts64(), which is already
> being exported to modules.
> 
> Could you please check whether that works for your driver?

Yes, will do. Thanks.

Ciao
Stephan