Hi Jakub,
Thanks for taking a look and your review,
On 1/7/23 01:53, Jakub Kicinski wrote:
[..]
>> +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.
Sounds good
>> +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?
Ah, this is little historical left-over: in the beginning, I used
constant-sized area as "scratch" buffer, the way TCP-MD5 does it.
Later, while converting users to crypto_pool, I found that it would be
helpful to support simple resizing as users have different size
requirement to the temporary buffer, i.e. looking at xfrm_ipcomp, if
later it would be converted to use the same API, rather than its own:
IPCOMP_SCRATCH_SIZE is huge (which may help to save quite some memory if
shared with other crypto_pool users: as the buffer is as well protected
by bh-disabled section, the usage pattern is quite the same).
In patch 2 I rewrote it for crypto_pool_reserve_scratch(). The purpose
of patch 2 was to only add dynamic up-sizing of this buffer to make it
easier to review the change. So, here are 2 options:
- I can move scratch area allocation/resizing/freeing to patch2 for v3
- Or I can keep patch 2 for only adding the resizing functionality, but
in patch 1 make crypto_pool_scratch_alloc() non-static and to the header
API.
What would you prefer?
[..]
>> +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..
Good idea, will do
>
>> + 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
Agree, will do for v3
>> +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.
Yes, unsure why I haven't done it from the beginning
[..]
>> +/**
>> + * 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()?
Yeah, I presume I took not the best-fitting naming from
tcp_get_md5sig_pool()/tcp_put_md5sig_pool().
Will do the renaming.
Thanks,
Dmitry
