| Message ID | 12904468.qCSNRrkb6t@tachyon.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Wed, May 20, 2015 at 10:03:45PM +0200, Stephan Mueller wrote: >> @@ -1487,6 +1514,7 @@ unlock: > */ > static int drbg_uninstantiate(struct drbg_state *drbg) > { > + cancel_work_sync(&drbg->seed_work); This will just block until the work is done, i.e., until the pool is ready. It's no different to an uninterruptible sleep. So either just do an uninterruptible sleep, or allow the async seed to fail. Cheers,
Am Donnerstag, 21. Mai 2015, 05:44:08 schrieb Herbert Xu: Hi Herbert, > On Wed, May 20, 2015 at 10:03:45PM +0200, Stephan Mueller wrote: > >> @@ -1487,6 +1514,7 @@ unlock: > > */ > > > > static int drbg_uninstantiate(struct drbg_state *drbg) > > { > > > > + cancel_work_sync(&drbg->seed_work); > > This will just block until the work is done, i.e., until the > pool is ready. It's no different to an uninterruptible sleep. > > So either just do an uninterruptible sleep, or allow the async > seed to fail. > The cancel operation is needed as otherwise my drbg context handle will be removed by the crypto API during the sleep. That is the whole reason why wait_event_interruptible is used. So, even when using an uninterruptible sleep, the crypto API has no knowledge about it and I have to serialize the destruction code path with the async callback.
On Thu, May 21, 2015 at 08:10:13AM +0200, Stephan Mueller wrote: > > The cancel operation is needed as otherwise my drbg context handle will be > removed by the crypto API during the sleep. That is the whole reason why > wait_event_interruptible is used. > > So, even when using an uninterruptible sleep, the crypto API has no knowledge > about it and I have to serialize the destruction code path with the async > callback. I understand. But if you use the uniterruptible version at least you won't have to deal with bogus early returns. The cancel by itself will *not* cause wait_event_interruptible to return early. Somebody has to send it a signal for that to happen. Cheers,
Am Donnerstag, 21. Mai 2015, 14:36:18 schrieb Herbert Xu: Hi Herbert, >On Thu, May 21, 2015 at 08:10:13AM +0200, Stephan Mueller wrote: >> The cancel operation is needed as otherwise my drbg context handle will be >> removed by the crypto API during the sleep. That is the whole reason why >> wait_event_interruptible is used. >> >> So, even when using an uninterruptible sleep, the crypto API has no >> knowledge about it and I have to serialize the destruction code path with >> the async callback. > >I understand. But if you use the uniterruptible version at least >you won't have to deal with bogus early returns. Thank you for pointing that out - I have seen that too. But the crux is that when using wait_event, the cancel function to serialize the destruction code path will *not* return at all, even when the async callback function completed successfully. Hence the choice for wait_event_interruptible > >The cancel by itself will *not* cause wait_event_interruptible >to return early. Somebody has to send it a signal for that to >happen. Correct. But I can only wake it up with a handle on urandom_init_wait on which the wait sleeps. That handle lives in random.c. So, to wake it up, I need another piece of code in random.c. That piece of code used to be the get_blocking_random_bytes_cancel function I offered in previous patches. As Ted mentioned, he did not want to much code in random.c for this. In addition, if I understand you correctly in previous emails, it would be acceptable to wait until the callback was triggered (and thus causing the caller module to be not unloadable for that period of time. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 21, 2015 at 08:53:13AM +0200, Stephan Mueller wrote: > > Thank you for pointing that out - I have seen that too. But the crux is that > when using wait_event, the cancel function to serialize the destruction code > path will *not* return at all, even when the async callback function completed > successfully. Hence the choice for wait_event_interruptible Why is that? It should return when the pool is initialised and your work completes. If it does not then that means either the pool isn't initialising or somehow your code is racy and getting stuck. If it's racy then wait_event_interruptible is just hiding the real problem. Cheers,
Am Donnerstag, 21. Mai 2015, 05:44:08 schrieb Herbert Xu: Hi Herbert, > On Wed, May 20, 2015 at 10:03:45PM +0200, Stephan Mueller wrote: > >> @@ -1487,6 +1514,7 @@ unlock: > > */ > > > > static int drbg_uninstantiate(struct drbg_state *drbg) > > { > > > > + cancel_work_sync(&drbg->seed_work); > > This will just block until the work is done, i.e., until the > pool is ready. It's no different to an uninterruptible sleep. > > So either just do an uninterruptible sleep, or allow the async > seed to fail. I think I found the issue: we cannot put interruptible and noninterruptible sleeps onto the same wait queue. With my initial tests, I put the uninterruptible sleep onto urandom_init_wait where also interruptible sleeps are put onto. So, I will create a 2nd wait queue in random.c for uninterruptible waits, change the get_blocking_random_bytes back to void and use wait_event to wait for the initialization. > > Cheers,
diff --git a/crypto/drbg.c b/crypto/drbg.c index 36dfece..563e5e9 100644 --- a/crypto/drbg.c +++ b/crypto/drbg.c @@ -1056,6 +1056,27 @@ static inline int __drbg_seed(struct drbg_state *drbg, struct list_head *seed, return ret; } +static void drbg_async_seed(struct work_struct *work) +{ + struct drbg_string data; + LIST_HEAD(seedlist); + struct drbg_state *drbg = container_of(work, struct drbg_state, + seed_work); + int ret; + + do { + ret = get_blocking_random_bytes(drbg->seed_buf, + drbg->seed_buf_len); + } while (ret == -ERESTARTSYS); + + drbg_string_fill(&data, drbg->seed_buf, drbg->seed_buf_len); + list_add_tail(&data.list, &seedlist); + mutex_lock(&drbg->drbg_mutex); + __drbg_seed(drbg, &seedlist, true); + memzero_explicit(drbg->seed_buf, drbg->seed_buf_len); + mutex_unlock(&drbg->drbg_mutex); +} + /* * Seeding or reseeding of the DRBG * @@ -1125,6 +1146,10 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers, if (!reseed) drbg->seed_buf_len = drbg->seed_buf_len / 3 * 2; + /* Invoke asynchronous seeding unless DRBG is in test mode. */ + if (!list_empty(&drbg->test_data.list) && !reseed) + schedule_work(&drbg->seed_work); + out: return ret; } @@ -1231,6 +1256,8 @@ static inline int drbg_alloc_state(struct drbg_state *drbg) if (!drbg->seed_buf) goto err; + INIT_WORK(&drbg->seed_work, drbg_async_seed); + return 0; err: @@ -1487,6 +1514,7 @@ unlock: */ static int drbg_uninstantiate(struct drbg_state *drbg) { + cancel_work_sync(&drbg->seed_work); if (drbg->d_ops) drbg->d_ops->crypto_fini(drbg); drbg_dealloc_state(drbg); diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h index b052698..46994b2 100644 --- a/include/crypto/drbg.h +++ b/include/crypto/drbg.h @@ -51,6 +51,7 @@ #include <linux/fips.h> #include <linux/mutex.h> #include <linux/list.h> +#include <linux/workqueue.h> /* * Concatenation Helper and string operation helper @@ -119,6 +120,7 @@ struct drbg_state { bool fips_primed; /* Continuous test primed? */ unsigned char *prev; /* FIPS 140-2 continuous test value */ #endif + struct work_struct seed_work; /* asynchronous seeding support */ u8 *seed_buf; /* buffer holding the seed */ size_t seed_buf_len; const struct drbg_state_ops *d_ops;
The async seeding operation is triggered during initalization right after the first non-blocking seeding is completed. As required by the asynchronous operation of random.c, a callback function is provided that is triggered by random.c once entropy is available. That callback function performs the actual seeding of the DRBG. CC: Andreas Steffen <andreas.steffen@strongswan.org> CC: Theodore Ts'o <tytso@mit.edu> CC: Sandy Harris <sandyinchina@gmail.com> Signed-off-by: Stephan Mueller <smueller@chronox.de> --- crypto/drbg.c | 28 ++++++++++++++++++++++++++++ include/crypto/drbg.h | 2 ++ 2 files changed, 30 insertions(+)