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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
