Re: [PATCH v3 4/6] crypto: drbg - add async seeding operation

[Stephan Mueller <smueller@chronox.de>]

Am Freitag, 1. Mai 2015, 11:13:31 schrieb Herbert Xu:

Hi Herbert,

>On Tue, Apr 28, 2015 at 05:00:03AM +0200, Stephan Mueller wrote:
>> @@ -1081,6 +1115,11 @@ static int drbg_seed(struct drbg_state *drbg, struct
>> drbg_string *pers,> 
>>  		return -EINVAL;
>>  	
>>  	}
>> 
>> +	/* cancel any previously invoked seeding */
>> +	mutex_unlock(&drbg->drbg_mutex);
>> +	drbg_async_work_cancel(&drbg->seed_work);
>> +	mutex_lock(&drbg->drbg_mutex);
>
>This seems dangerous and unnecessary.  Releasing and reacquiring
>the locks may invalidate previous checks.  Even if it doesn't
>matter today if somebody modifies the callers later on this could
>explode.

Agreed.
>
>You can easily remove this by making get_blocking_random_bytes_cb
>idempotent, i.e., do nothing if the work is already queued, which
>is what it would do anyway if you simply move the INIT_WORK out of
>it.

As the get_blocking_random_bytes_cb fully sets up the random_work data 
structure, I think INIT_WORK should be left in there to have a nice and easy 
API. Otherwise either a new call would need to be added to random.c. The 
caller is not able to invoke INIT_WORK himself as the worker function is 
static.

However, what about simply checking if rw->work is NULL and only then 
performing the INIT_WORK? In that case then, I guess that all the members of 
random_walk in get_blocking_random_bytes_cb should only be filled in if 
INIT_WORK is to be called as otherwise a race may occur: 
get_blocking_random_bytes_work already performs its operation on the data in 
the supplied random_work and in the middle of that work, and then we would 
change it with a new call to get_blocking_random_bytes_cb.

Ciao
Stephan