Re: [PATCH] crc-t10dif: Fix potential crypto notify dead-lock

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Jun 04, 2020 at 04:33:24PM +1000, Herbert Xu wrote:
> +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;
> +	return;
>  }

The last return statement is unnecessary.

>  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;
>  }

Wouldn't it make more sense to initialize crct10dif_tfm before registering the
notifier?  Then the mutex wouldn't be needed.

- Eric