On Thu, Jun 04, 2020 at 10:40:49PM -0700, Eric Biggers wrote:
> 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.
Thanks I'll change this.
> > 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.
Right the mutex wouldn't be needed, but you open up a race window
where a better algorithm could be registered in between the first
crypto_alloc call and the notifier registration.
Cheers,
--
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
