Re: [v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers <ebiggers@kernel.org>]

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