Re: [v3 PATCH] hwrng: core - Remove add_early_randomness

["Jarkko Sakkinen" <jarkko@kernel.org>]

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 <nfraprado@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 <nfraprado@collabora.com>
> Reported-by: Eric Biggers <ebiggers@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 <herbert@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 <jarkko@kernel.org>

BR, Jarkko