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

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Dec 04, 2019 at 05:19:10PM +0800, Herbert Xu wrote:
> I think this is a tad over-complicated.  All we really need to do
> is avoid changing larval->adult if we are not the best larval.
> Something like this (totally untested!):
> 
> ---8<---
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
> 
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
> 
> This patch fixes this by only fulfilling the original request if
> we are currently the best outstanding larval as judged by the
> priority.
>  
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba6..3e65653735f4 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -280,6 +280,18 @@ 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) {
> +		if (!crypto_is_larval(q))
> +			continue;
> +
> +		if (strcmp(alg->cra_name, q->cra_name))
> +			continue;
> +
> +		if (q->cra_priority > alg->cra_priority)
> +			goto complete;
> +	}
> +

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
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.

With my patch the user would get the lower priority implementation in this case,
since it would be the best one ready at the time cryptomgr_probe() finished.

- Eric