On Tue, 3 Jan 2023 18:42:53 +0000 Dmitry Safonov wrote:
> Introduce a per-CPU pool of async crypto requests that can be used
> in bh-disabled contexts (designed with net RX/TX softirqs as users in
> mind). Allocation can sleep and is a slow-path.
> Initial implementation has only ahash as a backend and a fix-sized array
> of possible algorithms used in parallel.
>
> Signed-off-by: Dmitry Safonov <dima@xxxxxxxxxx>
> +config CRYPTO_POOL
> + tristate "Per-CPU crypto pool"
> + default n
> + help
> + Per-CPU pool of crypto requests ready for usage in atomic contexts.
Let's make it a hidden symbol? It seems like a low-level library
which gets select'ed, so no point bothering users with questions.
config CRYPTO_POOL
tristate
that's it.
> +static int crypto_pool_scratch_alloc(void)
This isn't called by anything in this patch..
crypto_pool_alloc_ahash() should call it I'm guessing?
> +{
> + int cpu;
> +
> + lockdep_assert_held(&cpool_mutex);
> +
> + for_each_possible_cpu(cpu) {
> + void *scratch = per_cpu(crypto_pool_scratch, cpu);
> +
> + if (scratch)
> + continue;
> +
> + scratch = kmalloc_node(scratch_size, GFP_KERNEL,
> + cpu_to_node(cpu));
> + if (!scratch)
> + return -ENOMEM;
> + per_cpu(crypto_pool_scratch, cpu) = scratch;
> + }
> + return 0;
> +}
> +out_free:
> + if (!IS_ERR_OR_NULL(hash) && e->needs_key)
> + crypto_free_ahash(hash);
> +
> + for_each_possible_cpu(cpu) {
> + if (*per_cpu_ptr(e->req, cpu) == NULL)
> + break;
> + hash = crypto_ahash_reqtfm(*per_cpu_ptr(e->req, cpu));
Could you use a local variable here instead of @hash?
That way you won't need the two separate free_ahash()
one before and one after the loop..
> + ahash_request_free(*per_cpu_ptr(e->req, cpu));
I think using @req here would be beneficial as well :S
> + if (e->needs_key) {
> + crypto_free_ahash(hash);
> + hash = NULL;
> + }
> + }
> +
> + if (hash)
> + crypto_free_ahash(hash);
This error handling is tricky as hell, please just add a separate
variable to hold the
> +out_free_req:
> + free_percpu(e->req);
> +out_free_alg:
> + kfree(e->alg);
> + e->alg = NULL;
> + return ret;
> +}
> +
> +/**
> + * crypto_pool_alloc_ahash - allocates pool for ahash requests
> + * @alg: name of async hash algorithm
> + */
> +int crypto_pool_alloc_ahash(const char *alg)
> +{
> + int i, ret;
> +
> + /* slow-path */
> + mutex_lock(&cpool_mutex);
> +
> + for (i = 0; i < cpool_populated; i++) {
> + if (cpool[i].alg && !strcmp(cpool[i].alg, alg)) {
> + if (kref_read(&cpool[i].kref) > 0) {
In the current design we can as well resurrect a pool waiting to
be destroyed, right? Just reinit the ref and we're good.
Otherwise the read() + get() looks quite suspicious to a reader.
> + kref_get(&cpool[i].kref);
> + ret = i;
> + goto out;
> + } else {
> + break;
> + }
> + }
> + }
> +
> + for (i = 0; i < cpool_populated; i++) {
> + if (!cpool[i].alg)
> + break;
> + }
> + if (i >= CPOOL_SIZE) {
> + ret = -ENOSPC;
> + goto out;
> + }
> +
> + ret = __cpool_alloc_ahash(&cpool[i], alg);
> + if (!ret) {
> + ret = i;
> + if (i == cpool_populated)
> + cpool_populated++;
> + }
> +out:
> + mutex_unlock(&cpool_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(crypto_pool_alloc_ahash);
> +/**
> + * crypto_pool_add - increases number of users (refcounter) for a pool
> + * @id: crypto_pool that was previously allocated by crypto_pool_alloc_ahash()
> + */
> +void crypto_pool_add(unsigned int id)
> +{
> + if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg))
> + return;
> + kref_get(&cpool[id].kref);
> +}
> +EXPORT_SYMBOL_GPL(crypto_pool_add);
> +
> +/**
> + * crypto_pool_get - disable bh and start using crypto_pool
> + * @id: crypto_pool that was previously allocated by crypto_pool_alloc_ahash()
> + * @c: returned crypto_pool for usage (uninitialized on failure)
> + */
> +int crypto_pool_get(unsigned int id, struct crypto_pool *c)
Is there a precedent somewhere for the _add() and _get() semantics
you're using here? I don't think I've seen _add() for taking a
reference, maybe _get() -> start(), _add() -> _get()?
> +{
> + struct crypto_pool_ahash *ret = (struct crypto_pool_ahash *)c;
> +
> + local_bh_disable();
> + if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) {
> + local_bh_enable();
> + return -EINVAL;
> + }
> + ret->req = *this_cpu_ptr(cpool[id].req);
> + ret->base.scratch = this_cpu_read(crypto_pool_scratch);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(crypto_pool_get);
