| Message ID | 1461449.htSjBpksos@tachyon.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Apr 09, 2015 at 09:36:03AM +0200, Stephan Mueller wrote: > > diff --git a/crypto/algapi.c b/crypto/algapi.c > index f1d0307..cfca1de 100644 > --- a/crypto/algapi.c > +++ b/crypto/algapi.c > @@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template *tmpl, > if (IS_ERR(larval)) > goto unlock; > > + err = -EAGAIN; > + if (unlikely(!crypto_mod_get(&inst->alg))) { > + up_write(&crypto_alg_sem); > + crypto_unregister_instance(inst); > + goto err; > + } Just grab the reference count as soon as you enter the function and then you can unconditionally drop the reference count at the end. If you fail to grab it then just return an error and the caller will free it for you. Cheers,
Am Donnerstag, 9. April 2015, 15:41:41 schrieb Herbert Xu: Hi Herbert, >On Thu, Apr 09, 2015 at 09:36:03AM +0200, Stephan Mueller wrote: >> diff --git a/crypto/algapi.c b/crypto/algapi.c >> index f1d0307..cfca1de 100644 >> --- a/crypto/algapi.c >> +++ b/crypto/algapi.c >> @@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template >> *tmpl,> >> if (IS_ERR(larval)) >> >> goto unlock; >> >> + err = -EAGAIN; >> + if (unlikely(!crypto_mod_get(&inst->alg))) { >> + up_write(&crypto_alg_sem); >> + crypto_unregister_instance(inst); >> + goto err; >> + } > >Just grab the reference count as soon as you enter the function >and then you can unconditionally drop the reference count at the >end. If you fail to grab it then just return an error and the >caller will free it for you. I tested it and this approach does not work. If I see that right, the reason for that is the following: The suggestion is to grab the ref count at the start of the function followed by a __crypto_register_alg. __crypto_register_alg however sets the refcount to 1 unconditionally. That means that the final put of the alg will most likely set the refcount to 0 that causes an issue with all other operations (at least I cannot allocate HMAC or CMAC any more -- the ones I currently test). So, the grabing of the alg must happen after the invocation of __crypto_register_alg. > >Cheers, 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
diff --git a/crypto/algapi.c b/crypto/algapi.c index f1d0307..cfca1de 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -533,6 +533,13 @@ int crypto_register_instance(struct crypto_template *tmpl, if (IS_ERR(larval)) goto unlock; + err = -EAGAIN; + if (unlikely(!crypto_mod_get(&inst->alg))) { + up_write(&crypto_alg_sem); + crypto_unregister_instance(inst); + goto err; + } + hlist_add_head(&inst->list, &tmpl->instances); inst->tmpl = tmpl; @@ -544,8 +551,13 @@ unlock: goto err; crypto_wait_for_test(larval); + + /* Remove instance if test failed */ + if (!(inst->alg.cra_flags & CRYPTO_ALG_TESTED)) + crypto_unregister_instance(inst); err = 0; + crypto_mod_put(&inst->alg); err: return err; }
A cipher instance is added to the list of instances unconditionally regardless of whether the associated test failed. However, a failed test implies that during another lookup, the cipher instance will be added to the list again as it will not be found by the lookup code. That means that the list can be filled up with instances whose tests failed. Note: tests only fail in reality in FIPS mode when a cipher is not marked as fips_allowed=1. This can be seen with cmac(des3_ede) that does not have a fips_allowed=1. When allocating the cipher, the allocation fails with -ENOENT due to the missing fips_allowed=1 flag (which causes the testmgr to return EINVAL). Yet, the instance of cmac(des3_ede) is shown in /proc/crypto. Allocating the cipher again fails again, but a 2nd instance is listed in /proc/crypto. The patch simply de-registers the instance when the testing failed. Signed-off-by: Stephan Mueller <smueller@chronox.de> --- crypto/algapi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)