On Thu, Oct 19, 2017 at 1:45 PM, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
> As this interface becomes more heavily used, it will be painful for
> callers to always need to check for -EALREADY.
>
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> ---
> drivers/char/random.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8ad92707e45f..e161acf35d4a 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes);
>
> /*
> * Add a callback function that will be invoked when the nonblocking
> - * pool is initialised.
> + * pool is initialised, or calls that function if it already is.
> *
> * returns: 0 if callback is successfully added
> - * -EALREADY if pool is already initialised (callback not called)
> * -ENOENT if module for callback is not alive
> */
> int add_random_ready_callback(struct random_ready_callback *rdy)
> {
> struct module *owner;
> unsigned long flags;
> - int err = -EALREADY;
>
> - if (crng_ready())
> - return err;
> + BUG_ON(!rdy->func);
Better to WARN_ON() and return a failure.
> +
> + if (crng_ready()) {
> + rdy->func(rdy);
> + rdy->func = NULL;
> + return 0;
> + }
>
> owner = rdy->owner;
> if (!try_module_get(owner))
> return -ENOENT;
>
> spin_lock_irqsave(&random_ready_list_lock, flags);
> - if (crng_ready())
> + if (crng_ready()) {
> + rdy->func(rdy);
> + rdy->func = NULL;
> goto out;
> + }
>
> owner = NULL;
>
> list_add(&rdy->list, &random_ready_list);
> - err = 0;
>
> out:
> spin_unlock_irqrestore(&random_ready_list_lock, flags);
>
> module_put(owner);
>
> - return err;
> + return 0;
> }
> EXPORT_SYMBOL(add_random_ready_callback);
>
> @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy)
> unsigned long flags;
> struct module *owner = NULL;
>
> + if (!rdy->func)
> + return;
Perhaps add a note here about what a NULL func means here? (e.g.
"Already run, not in the list.")
> +
> spin_lock_irqsave(&random_ready_list_lock, flags);
> if (!list_empty(&rdy->list)) {
> list_del_init(&rdy->list);
> --
> 2.14.2
>
Otherwise, yeah, looks sensible.
-Kees
--
Kees Cook
Pixel Security
