On Fri, Jun 05, 2020 at 04:59:18PM +1000, Herbert Xu wrote:
> The crypto notify call occurs with a read mutex held so you must
> not do any substantial work directly. In particular, you cannot
> call crypto_alloc_* as they may trigger further notifications
> which may dead-lock in the presence of another writer.
>
> This patch fixes this by postponing the work into a work queue and
> taking the same lock in the module init function.
>
> While we're at it this patch also ensures that all RCU accesses are
> marked appropriately (tested with sparse).
>
> Finally this also reveals a race condition in module param show
> function as it may be called prior to the module init function.
> It's fixed by testing whether crct10dif_tfm is NULL (this is true
> iff the init function has not completed assuming fallback is false).
>
> Fixes: 11dcb1037f40 ("crc-t10dif: Allow current transform to be...")
> Fixes: b76377543b73 ("crc-t10dif: Pick better transform if one...")
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
>
> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
> index 8cc01a603416..c9acf1c12cfc 100644
> --- a/lib/crc-t10dif.c
> +++ b/lib/crc-t10dif.c
> @@ -19,39 +19,46 @@
> static struct crypto_shash __rcu *crct10dif_tfm;
> static struct static_key crct10dif_fallback __read_mostly;
> static DEFINE_MUTEX(crc_t10dif_mutex);
> +static struct work_struct crct10dif_rehash_work;
>
> -static int crc_t10dif_rehash(struct notifier_block *self, unsigned long val,
> void *data)
> +static int crc_t10dif_notify(struct notifier_block *self, unsigned long val,
> void *data)
> {
> struct crypto_alg *alg = data;
> - struct crypto_shash *new, *old;
>
> if (val != CRYPTO_MSG_ALG_LOADED ||
> static_key_false(&crct10dif_fallback) ||
> strncmp(alg->cra_name, CRC_T10DIF_STRING,
> strlen(CRC_T10DIF_STRING)))
> return 0;
>
> + schedule_work(&crct10dif_rehash_work);
> + return 0;
> +}
> +
> +static void crc_t10dif_rehash(struct work_struct *work)
> +{
> + struct crypto_shash *new, *old;
> +
> mutex_lock(&crc_t10dif_mutex);
> old = rcu_dereference_protected(crct10dif_tfm,
> lockdep_is_held(&crc_t10dif_mutex));
> if (!old) {
> mutex_unlock(&crc_t10dif_mutex);
> - return 0;
> + return;
> }
> new = crypto_alloc_shash("crct10dif", 0, 0);
> if (IS_ERR(new)) {
> mutex_unlock(&crc_t10dif_mutex);
> - return 0;
> + return;
> }
> rcu_assign_pointer(crct10dif_tfm, new);
> mutex_unlock(&crc_t10dif_mutex);
>
> synchronize_rcu();
> crypto_free_shash(old);
> - return 0;
> }
>
> static struct notifier_block crc_t10dif_nb = {
> - .notifier_call = crc_t10dif_rehash,
> + .notifier_call = crc_t10dif_notify,
> };
>
> __u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
> @@ -86,19 +93,26 @@ EXPORT_SYMBOL(crc_t10dif);
>
> static int __init crc_t10dif_mod_init(void)
> {
> + struct crypto_shash *tfm;
> +
> + INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash);
> crypto_register_notifier(&crc_t10dif_nb);
> - crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0);
> - if (IS_ERR(crct10dif_tfm)) {
> + mutex_lock(&crc_t10dif_mutex);
> + tfm = crypto_alloc_shash("crct10dif", 0, 0);
> + if (IS_ERR(tfm)) {
> static_key_slow_inc(&crct10dif_fallback);
> - crct10dif_tfm = NULL;
> + tfm = NULL;
> }
> + RCU_INIT_POINTER(crct10dif_tfm, tfm);
> + mutex_unlock(&crc_t10dif_mutex);
> return 0;
> }
>
> static void __exit crc_t10dif_mod_fini(void)
> {
> crypto_unregister_notifier(&crc_t10dif_nb);
> - crypto_free_shash(crct10dif_tfm);
> + cancel_work_sync(&crct10dif_rehash_work);
> + crypto_free_shash(rcu_dereference_protected(crct10dif_tfm, 1));
> }
>
> module_init(crc_t10dif_mod_init);
> @@ -106,11 +120,27 @@ module_exit(crc_t10dif_mod_fini);
>
> static int crc_t10dif_transform_show(char *buffer, const struct kernel_param
> *kp)
> {
> + struct crypto_shash *tfm;
> + const char *name;
> + int len;
> +
> if (static_key_false(&crct10dif_fallback))
> return sprintf(buffer, "fallback\n");
>
> - return sprintf(buffer, "%s\n",
> - crypto_tfm_alg_driver_name(crypto_shash_tfm(crct10dif_tfm)));
> + rcu_read_lock();
> + tfm = rcu_dereference(crct10dif_tfm);
> + if (!tfm) {
> + len = sprintf(buffer, "init\n");
> + goto unlock;
> + }
Wouldn't it be better to have crct10dif_fallback enabled by default, and then
disable it once the tfm is allocated?
That would make the checks for a NULL tfm in crc_t10dif_transform_show() and
crc_t10dif_notify() unnecessary. Also, it would make it so that
crc_t10dif_update() no longer crashes if called before module_init().
- Eric
