linux-kernel - Re: [PATCH 3/3] random: use trylock in irq handler rather than spinning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgiraRIkuKK787YC@owl.dominikbrodowski.net>
Date:   Sun, 13 Feb 2022 07:55:37 +0100
From:   Dominik Brodowski <linux@...inikbrodowski.net>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH 3/3] random: use trylock in irq handler rather than
 spinning

Am Sun, Feb 13, 2022 at 12:10:22AM +0100 schrieb Jason A. Donenfeld:
> crng_pre_init_inject() (and prior crng_fast_load()) uses a trylock when
> in fast mode, so that it never contends. We should be doing the same
> when grabbing a spinlock for mixing into the entropy pool. So switch to
> doing that before calling the underscored _mix_pool_bytes().
> 
> Cc: Dominik Brodowski <linux@...inikbrodowski.net>
> Cc: Theodore Ts'o <tytso@....edu>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> ---
>  drivers/char/random.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 9a8e1bb9845d..ca224c3f2561 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1286,13 +1286,10 @@ void add_interrupt_randomness(int irq)
>  			atomic_set(&fast_pool->count, 0);
>  			fast_pool->last = now;
>  
> -			/*
> -			 * Technically this call means that we're using a spinlock_t
> -			 * in the IRQ handler, which isn't terrific for PREEMPT_RT.
> -			 * However, this only happens during boot, and then never
> -			 * again, so we live with it.
> -			 */
> -			mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
> +			if (spin_trylock(&input_pool.lock)) {
> +				_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
> +				spin_unlock(&input_pool.lock);
> +			}

You're still using a spinlock_t here, so I don't see a need to remove the
comment. Also, I'm not super happy that the count is re-set to 0 but the
input remains unused. Maybe the better approach here is, as discussed in the
other thread, to always use the workqueue mechanism, which would allow us to
streamline the code further.

Thanks,
	Dominik