| Message ID | 20191205015811.mg6r3qnv7uj3fgpz@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v3] crypto: api - fix unexpectedly getting generic implementation | expand |
On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote: > On Wed, Dec 04, 2019 at 09:22:44AM -0800, Eric Biggers wrote: > > > > I was going to do something like this originally (but also checking that 'q' is > > not "moribund", is a test larval, and has compatible cra_flags). But I don't > > You are right. I'll add these tests to the patch. > > > think it will work because a higher priority implementation could be registered > > while a lower priority one is being instantiated and tested. Based on this > > logic, when the lower priority implementation finishes being tested, > > larval->adult wouldn't be set since a higher priority implementation is still > > being tested. But then cryptomgr_probe() will complete() the larval anyway and > > for the user crypto_alloc_foo() will fail with ENOENT. > > I think this is a different problem, one which we probably should > address but it already exists regardless of what we do here. For > example, assuming that tmpl(X) does not currently exist, and I > request tmpl(X-generic) then tmpl(X-generic) along with X-generic > will be created in the system. If someone then comes along and > asks for tmpl(X) then we'll simply give them tmpl(X-generic) even > if there exists an accelerated version of X. > > The problem you describe is simply a racy version of the above > scenario where the requests for tmpl(X) and tmpl(X-generic) occur > about the same time. > No, the problem I'm talking about is different and is new to your patch. If tmpl(X-accelerated) is registered while someone is doing crypto_alg_mod_lookup() that triggered instantiation of tmpl(X-generic), then crypto_alg_mod_lookup() could fail with ENOENT, instead of returning tmpl(X-generic) as it does currently. This is because the proposed new logic will not fulfill the request larval if a better implementation of tmpl(X) is still being tested. But there's no guarantee that tmpl(X) will finish being tested by the time cryptomgr_probe() thinks it is done and complete()s the request larval with 'adult' still NULL. (I think; I haven't actually tested this, this analysis is just based on my reading of the code...) - Eric
On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote: > + /* Only satisfy larval waiters if we are the best. */ > + list_for_each_entry(q, &crypto_alg_list, cra_list) { > + struct crypto_larval *larval; > + > + if (crypto_is_moribund(q) || !crypto_is_larval(q)) > + continue; > + > + if (strcmp(alg->cra_name, q->cra_name)) > + continue; > + > + larval = (void *)q; > + if ((q->cra_flags ^ alg->cra_flags) & larval->mask) > + continue; > + > + if (q->cra_priority > alg->cra_priority) > + goto complete; > + } > + This logic doesn't make sense to me either. It's supposed to be looking for a "test larval", not a "request larval", right? But it seems that larval->mask is always 0 for "test larvals", so the flags check will never do anything... Also, different "request larvals" can use different flags and masks. So I don't think it's possible to know whether 'q' can fulfill every outstanding request that 'alg' can without actually going through and looking at the requests. So that's another case where users can start incorrectly getting ENOENT. If we don't try to skip setting larval->adult, but rather override the existing value (as my patch does), the incorrect ENOENTs are prevented. - Eric
On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote: > > This logic doesn't make sense to me either. It's supposed to be looking for a > "test larval", not a "request larval", right? But it seems that larval->mask is > always 0 for "test larvals", so the flags check will never do anything... No we only care about request larvals. Test larvals always have a non-null adult set so they are irrelevant. > Also, different "request larvals" can use different flags and masks. So I don't > think it's possible to know whether 'q' can fulfill every outstanding request > that 'alg' can without actually going through and looking at the requests. So > that's another case where users can start incorrectly getting ENOENT. That's a good point. Let me think about this a bit more. Thanks,
On Thu, Dec 05, 2019 at 12:23:57PM +0800, Herbert Xu wrote: > On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote: > > > > This logic doesn't make sense to me either. It's supposed to be looking for a > > "test larval", not a "request larval", right? But it seems that larval->mask is > > always 0 for "test larvals", so the flags check will never do anything... > > No we only care about request larvals. Test larvals always have > a non-null adult set so they are irrelevant. Scratch that. You're right we only care about test larvals and if we wanted to check the flags we need to drill down to the actual tested algorithm using larval->adult. Cheers,
diff --git a/crypto/algapi.c b/crypto/algapi.c index b052f38edba6..390bdc874b61 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -280,6 +280,24 @@ void crypto_alg_tested(const char *name, int err) alg->cra_flags |= CRYPTO_ALG_TESTED; + /* Only satisfy larval waiters if we are the best. */ + list_for_each_entry(q, &crypto_alg_list, cra_list) { + struct crypto_larval *larval; + + if (crypto_is_moribund(q) || !crypto_is_larval(q)) + continue; + + if (strcmp(alg->cra_name, q->cra_name)) + continue; + + larval = (void *)q; + if ((q->cra_flags ^ alg->cra_flags) & larval->mask) + continue; + + if (q->cra_priority > alg->cra_priority) + goto complete; + } + list_for_each_entry(q, &crypto_alg_list, cra_list) { if (q == alg) continue;