2020-10-02 06:49:52

by Christoph Hellwig

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

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 <[email protected]>
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
---end quoted text---


2020-10-02 06:57:22

by Stephan Müller

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

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


2020-10-04 18:49:15

by Stephan Müller

[permalink] [raw]
Subject: [PATCH] crypto: jitterentropy - bind statically into kernel

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 <[email protected]>
Signed-off-by: Stephan Mueller <[email protected]>
---
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




2020-10-04 21:17:55

by Ard Biesheuvel

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

On Sun, 4 Oct 2020 at 20:48, Stephan Müller <[email protected]> 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 <[email protected]>
> Signed-off-by: Stephan Mueller <[email protected]>
> ---
> 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
>
>
>
>

2020-10-04 22:05:50

by Palmer Dabbelt

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

On Sun, 04 Oct 2020 14:16:10 PDT (-0700), [email protected] wrote:
> On Sun, 4 Oct 2020 at 20:48, Stephan Müller <[email protected]> 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 <[email protected]>
>> Signed-off-by: Stephan Mueller <[email protected]>
>> ---
>> 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
>>
>>
>>
>>

2020-10-05 06:20:07

by Christoph Hellwig

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

On Sun, Oct 04, 2020 at 11:16:10PM +0200, Ard Biesheuvel wrote:
> On Sun, 4 Oct 2020 at 20:48, Stephan M??ller <[email protected]> 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.

2020-10-05 06:25:35

by Ard Biesheuvel

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

On Mon, 5 Oct 2020 at 08:19, Christoph Hellwig <[email protected]> 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 <[email protected]> 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.

2020-10-05 06:41:12

by Stephan Müller

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

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


2020-10-05 06:44:38

by Christoph Hellwig

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

[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?

2020-10-05 06:47:47

by Ard Biesheuvel

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

On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <[email protected]> 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?

2020-10-05 06:57:17

by Christoph Hellwig

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

On Mon, Oct 05, 2020 at 08:44:39AM +0200, Ard Biesheuvel wrote:
> On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <[email protected]> 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..

2020-10-05 07:46:17

by Stephan Müller

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

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

Hi Ard,

> On Mon, 5 Oct 2020 at 08:40, Stephan Mueller <[email protected]> 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