Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Nov 2022 15:16:49 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-efi@vger.kernel.org, linux-crypto@vger.kernel.org,
        Ard Biesheuvel <ardb@kernel.org>,
        Lennart Poettering <lennart@poettering.net>,
        Mike Galbraith <efault@gmx.de>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: Re: [PATCH RFC v1 2/6] vsprintf: initialize siphash key using
 notifier
Message-ID: <Y3eT0fd/t270Qeaj@alley>
References: <20221116161642.1670235-1-Jason@zx2c4.com>
 <20221116161642.1670235-3-Jason@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221116161642.1670235-3-Jason@zx2c4.com>
Precedence: bulk

On Wed 2022-11-16 17:16:38, Jason A. Donenfeld wrote:
> Rather than polling every second, use the new notifier to do this at
> exactly the right moment.

Great news!

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -752,26 +753,21 @@ early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
>  
>  static bool filled_random_ptr_key __read_mostly;
>  static siphash_key_t ptr_key __read_mostly;
> -static void fill_ptr_key_workfn(struct work_struct *work);
> -static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn);
>  
> -static void fill_ptr_key_workfn(struct work_struct *work)
> +static int fill_ptr_key(struct notifier_block *nb, unsigned long action, void *data)
>  {
> -	if (!rng_is_initialized()) {
> -		queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ  * 2);
> -		return;
> -	}
> -
>  	get_random_bytes(&ptr_key, sizeof(ptr_key));
>  
>  	/* Pairs with smp_rmb() before reading ptr_key. */
>  	smp_wmb();
>  	WRITE_ONCE(filled_random_ptr_key, true);
> +	return 0;

I believe that we should rather return NOTIFY_DONE here.
It is rather a formal change. The value is 0 as well.

That said, I have never really understood the difference between
NOTIFY_OK and NOTIFY_DONE.

>  }
>  
>  static int __init vsprintf_init_hashval(void)
>  {
> -	fill_ptr_key_workfn(NULL);
> +	static struct notifier_block fill_ptr_key_nb = { .notifier_call = fill_ptr_key };
> +	notify_on_rng_initialized(&fill_ptr_key_nb);
>  	return 0;
>  }
>  subsys_initcall(vsprintf_init_hashval)

Anyway, the code looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr