On Wed May 22, 2024 at 8:37 AM EEST, Herbert Xu wrote:
> On Tue, May 21, 2024 at 03:37:16PM -0400, Nícolas F. R. A. Prado wrote:
> >
> > FWIW this patch fixes the warning. So feel free to add
> >
> > Tested-by: Nícolas F. R. A. Prado <nfrapr...@collabora.com>
>
> Could you please test this patch instead?
>
> ---8<---
> A potential deadlock was reported with the config file at
>
> https://web.archive.org/web/20240522052129/https://0x0.st/XPN_.txt
>
> In this particular configuration, the deadlock doesn't exist because
> the warning triggered at a point before modules were even available.
> However, the deadlock can be real because any module loaded would
> invoke async_synchronize_full.
>
> The issue is spurious for software crypto algorithms which aren't
> themselves involved in async probing. However, it would be hard to
> avoid for a PCI crypto driver using async probing.
>
> In this particular call trace, the problem is easily avoided because
> the only reason the module is being requested during probing is the
> add_early_randomness call in the hwrng core. This feature is
> vestigial since there is now a kernel thread dedicated to doing
> exactly this.
>
> So remove add_early_randomness as it is no longer needed.
"vestigial" did not know that word before ;-) Something learned.
What is the kthread doing this currently?
>
> Reported-by: Nícolas F. R. A. Prado <nfrapr...@collabora.com>
> Reported-by: Eric Biggers <ebigg...@kernel.org>
> Fixes: 1b6d7f9eb150 ("tpm: add session encryption protection to
> tpm2_get_random()")
> Link:
> https://lore.kernel.org/r/119dc5ed-f159-41be-9dda-1a056f29888d@notapiano/
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index f5c71a617a99..4084df65c9fa 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -64,19 +64,6 @@ static size_t rng_buffer_size(void)
> return RNG_BUFFER_SIZE;
> }
>
> -static void add_early_randomness(struct hwrng *rng)
> -{
> - int bytes_read;
> -
> - mutex_lock(&reading_mutex);
> - bytes_read = rng_get_data(rng, rng_fillbuf, 32, 0);
> - mutex_unlock(&reading_mutex);
> - if (bytes_read > 0) {
> - size_t entropy = bytes_read * 8 * rng->quality / 1024;
> - add_hwgenerator_randomness(rng_fillbuf, bytes_read, entropy,
> false);
> - }
> -}
> -
> static inline void cleanup_rng(struct kref *kref)
> {
> struct hwrng *rng = container_of(kref, struct hwrng, ref);
> @@ -340,13 +327,12 @@ static ssize_t rng_current_store(struct device *dev,
> const char *buf, size_t len)
> {
> int err;
> - struct hwrng *rng, *old_rng, *new_rng;
> + struct hwrng *rng, *new_rng;
>
> err = mutex_lock_interruptible(&rng_mutex);
> if (err)
> return -ERESTARTSYS;
>
> - old_rng = current_rng;
> if (sysfs_streq(buf, "")) {
> err = enable_best_rng();
> } else {
> @@ -362,11 +348,8 @@ static ssize_t rng_current_store(struct device *dev,
> new_rng = get_current_rng_nolock();
> mutex_unlock(&rng_mutex);
>
> - if (new_rng) {
> - if (new_rng != old_rng)
> - add_early_randomness(new_rng);
> + if (new_rng)
> put_rng(new_rng);
> - }
>
> return err ? : len;
> }
> @@ -544,7 +527,6 @@ int hwrng_register(struct hwrng *rng)
> {
> int err = -EINVAL;
> struct hwrng *tmp;
> - bool is_new_current = false;
>
> if (!rng->name || (!rng->data_read && !rng->read))
> goto out;
> @@ -573,25 +555,8 @@ int hwrng_register(struct hwrng *rng)
> err = set_current_rng(rng);
> if (err)
> goto out_unlock;
> - /* to use current_rng in add_early_randomness() we need
> - * to take a ref
> - */
> - is_new_current = true;
> - kref_get(&rng->ref);
> }
> mutex_unlock(&rng_mutex);
> - if (is_new_current || !rng->init) {
> - /*
> - * Use a new device's input to add some randomness to
> - * the system. If this rng device isn't going to be
> - * used right away, its init function hasn't been
> - * called yet by set_current_rng(); so only use the
> - * randomness from devices that don't need an init callback
> - */
> - add_early_randomness(rng);
> - }
> - if (is_new_current)
> - put_rng(rng);
> return 0;
> out_unlock:
> mutex_unlock(&rng_mutex);
> @@ -602,12 +567,11 @@ EXPORT_SYMBOL_GPL(hwrng_register);
>
> void hwrng_unregister(struct hwrng *rng)
> {
> - struct hwrng *old_rng, *new_rng;
> + struct hwrng *new_rng;
> int err;
>
> mutex_lock(&rng_mutex);
>
> - old_rng = current_rng;
> list_del(&rng->list);
> complete_all(&rng->dying);
> if (current_rng == rng) {
> @@ -626,11 +590,8 @@ void hwrng_unregister(struct hwrng *rng)
> } else
> mutex_unlock(&rng_mutex);
>
> - if (new_rng) {
> - if (old_rng != new_rng)
> - add_early_randomness(new_rng);
> + if (new_rng)
> put_rng(new_rng);
> - }
>
> wait_for_completion(&rng->cleanup_done);
> }
I have no doubts that such thread would not exist, so:
Reviewed-by: Jarkko Sakkinen <jar...@kernel.org>
BR, Jarkko
