[v4,0/4] net/crypto: Introduce crypto_pool

Changes since v3 [6]:
- Cleanup seg6_hmac_init() and seg6_hmac_exit() declaration/usage
  left-overs (reported by Jakub)
- Remove max(size, __scratch_size) from crypto_pool_reserve_scratch()

Changes since v2 [5]:
- Fix incorrect rebase of v2: tcp_md5_add_crypto_pool() was
  called on twsk creation even for sockets without TCP-MD5 key
- Documentation title underline length
  (Reported-by: kernel test robot <lkp@intel.com>)
- Migrate crypto_pool_scratch to __rcu, using rcu_dereference*()
  and rcu_replace_pointer(). As well, I changed local_bh_{en,dis}able()
  to rcu_read_{,un}lock_bh().
  (Addressing Jakub's review)
- Correct Documentation/ to use proper kerneldoc style, include it in
  toc/tree and editor notes (from Jakub's comments)
- Avoid cast in crypto_pool_get() (Jakub's review)
- Select CRYPTO in Kconfig, not only CRYPTO_POOL (Jakub's reivew)
- Remove free_batch[] with synchronize_rcu() in favor of a struct
  with a flexible array inside + call_rcu() (suggested by Jakub)
- Change scratch `size` argument type from (unsigned long) to (size_t)
  for consistency
- Combined crypto_pool_alloc_ahash() and crypto_pool_reserve_scratch(),
  now the scratch area size is supplied on crypto_pool allocation
  (suggested by Jakub)
- Removed CONFIG_CRYPTO_POOL_DEFAULT_SCRATCH_SIZE
- CRYPTO_POOL now is a hidden symbol (Jakub's review)
- Simplified __cpool_alloc_ahash() error-paths, adding local variables
  (suggested by Jakub)
- Resurrect a pool waiting to be destroyed if possible (Jakub's review)
- Rename _get() => _start(), _put() => _end(), _add() => _get()
  (suggested by Jakub)

Changes since v1 [1]:
- Patches went through 3 iterations inside bigger TCP-AO patch set [2],
  now I'm splitting it apart and sending it once again as a stand-alone
  patch set to help reviewing it and make it easier to merge.
  It is second part of that big series, once it merges the next part
  will be TCP changes to add Authentication Option support (RFC5925),
  that use API provided by these patches.
- Corrected kerneldoc-style comment near crypto_pool_reserve_scratch()
  (Reported-By: kernel test robot <lkp@intel.com>)
- Added short Documentation/ page for crypto_pool API

Add crypto_pool - an API for allocating per-CPU array of crypto requests
on slow-path (in sleep'able contexts) and for using them on a fast-path,
which is RX/TX for net/* users.

The design is based on the current implementations of md5sig_pool, which
this patch set makes generic by separating it from TCP core, moving it
to crypto/ and adding support for other hashing algorithms than MD5.
It makes a generic implementation for a common net/ pattern.

The initial motivation to have this API is TCP-AO, that's going to use
the very same pattern as TCP-MD5, but for multiple hashing algorithms.
Previously, I've suggested to add such API on TCP-AO patch submission [3],
where Herbert kindly suggested to help with introducing new crypto API.
See also discussion and motivation in crypto_pool-v1 [4].

The API will allow:
- to reuse per-CPU ahash_request(s) for different users
- to allocate only one per-CPU scratch buffer rather than a new one for
  each user
- to have a common API for net/ users that need ahash on RX/TX fast path

In this version I've wired up TCP-MD5 and IPv6-SR-HMAC as users.
Potentially, xfrm_ipcomp and xfrm_ah can be converted as well.
The initial reason for patches would be to have TCP-AO as a user, which
would let it share per-CPU crypto_request for any supported hashing
algorithm.

[1]: https://lore.kernel.org/all/20220726201600.1715505-1-dima@arista.com/ 
[2]: https://lore.kernel.org/all/20221027204347.529913-1-dima@arista.com/T/#u
[3]: http://lkml.kernel.org/r/20211106034334.GA18577@gondor.apana.org.au
[4]: https://lore.kernel.org/all/26d5955b-3807-a015-d259-ccc262f665c2@arista.com/T/#u
[5]: https://lore.kernel.org/all/20230103184257.118069-1-dima@arista.com/
[6]: https://lore.kernel.org/all/20230116201458.104260-1-dima@arista.com/T/#u

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Bob Gilligan <gilligan@arista.com>
Cc: David Ahern <dsahern@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Leonard Crestez <cdleonard@gmail.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Salam Noureddine <noureddine@arista.com>
Cc: netdev@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Dmitry Safonov (4):
  crypto: Introduce crypto_pool
  crypto/net/tcp: Use crypto_pool for TCP-MD5
  crypto/net/ipv6: sr: Switch to using crypto_pool
  crypto/Documentation: Add crypto_pool kernel API

 Documentation/crypto/crypto_pool.rst |  36 +++
 Documentation/crypto/index.rst       |   1 +
 crypto/Kconfig                       |   3 +
 crypto/Makefile                      |   1 +
 crypto/crypto_pool.c                 | 333 +++++++++++++++++++++++++++
 include/crypto/pool.h                |  46 ++++
 include/net/seg6_hmac.h              |   9 -
 include/net/tcp.h                    |  24 +-
 net/ipv4/Kconfig                     |   1 +
 net/ipv4/tcp.c                       | 104 ++-------
 net/ipv4/tcp_ipv4.c                  | 100 ++++----
 net/ipv4/tcp_minisocks.c             |  21 +-
 net/ipv6/Kconfig                     |   1 +
 net/ipv6/seg6.c                      |  14 +-
 net/ipv6/seg6_hmac.c                 | 207 +++++++----------
 net/ipv6/tcp_ipv6.c                  |  61 +++--
 16 files changed, 636 insertions(+), 326 deletions(-)
 create mode 100644 Documentation/crypto/crypto_pool.rst
 create mode 100644 crypto/crypto_pool.c
 create mode 100644 include/crypto/pool.h


base-commit: c1649ec55708ae42091a2f1bca1ab49ecd722d55

Hi Herbert,

On 1/19/23 09:51, Herbert Xu wrote:
> On Wed, Jan 18, 2023 at 09:41:08PM +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@arista.com>
>> ---
>>  crypto/Kconfig        |   3 +
>>  crypto/Makefile       |   1 +
>>  crypto/crypto_pool.c  | 333 ++++++++++++++++++++++++++++++++++++++++++
>>  include/crypto/pool.h |  46 ++++++
>>  4 files changed, 383 insertions(+)
>>  create mode 100644 crypto/crypto_pool.c
>>  create mode 100644 include/crypto/pool.h
> 
> I'm still nacking this.
> 
> I'm currently working on per-request keys which should render
> this unnecessary.  With per-request keys you can simply do an
> atomic kmalloc when you compute the hash.

Adding per-request keys sounds like a real improvement to me.
But that is not the same issue I'm addressing here. I'm maybe bad at
describing or maybe I just don't see how per-request keys would help.
Let me describe the problem I'm solving again and please feel free to
correct inline or suggest alternatives.

The initial need for crypto_pool comes from TCP-AO implementation that
I'm pusing upstream, see RFC5925 that describes the option and the
latest version of patch set is in [1]. In that patch set hashing is used
in a similar way to TCP-MD5: crypto_alloc_ahash() is a slow-path in
setsockopt() and the use of pre-allocated requests in fast path, TX/RX
softirqs.

For TCP-AO 2 algorithms are "must have" in any compliant implementation,
according to RFC5926: HMAC-SHA-1-96 and AES-128-CMAC-96, other
algorithms are optional. But having in mind that sha1, as you know, is
not secure to collision attacks, some customers prefer to have/use
stronger hashes. In other words, TCP-AO implementation needs 2+ hashing
algorithms to be used in a similar manner as TCP-MD5 uses MD5 hashing.

And than, I look around and I see that the same pattern (slow allocation
of crypto request and usage on a fast-path with bh disabled) is used in
other places over kernel:
- here I convert to crypto_pool seg6_hmac & tcp-md5
- net/ipv4/ah4.c could benefit from it: currently it allocates
crypto_alloc_ahash() per every connection, allocating user-specified
hash algorithm with ahash = crypto_alloc_ahash(x->aalg->alg_name, 0, 0),
which are not shared between each other and it doesn't provide
pre-allocated temporary/scratch buffer to calculate hash, so it uses
GFP_ATOMIC in ah_alloc_tmp()
- net/ipv6/ah6.c is copy'n'paste of the above
- net/ipv4/esp4.c and net/ipv6/esp6.c are more-or-less also copy'n'paste
with crypto_alloc_aead() instead of crypto_alloc_ahash()
- net/mac80211/ - another example of the same pattern, see even the
comment in ieee80211_key_alloc() where the keys are allocated and the
usage in net/mac80211/{rx,tx}.c with bh-disabled
- net/xfrm/xfrm_ipcomp.c has its own manager for different compression
algorithms that are used in quite the same fashion. The significant
exception is scratch area: it's IPCOMP_SCRATCH_SIZE=65400. So, if it
could be shared with other crypto users that do the same pattern
(bh-disabled usage), it would save some memory.

And those are just fast-grep examples from net/, looking closer it may
be possible to find more potential users.
So, in crypto_pool.c it's 333 lines where is a manager that let a user
share pre-allocated ahash requests [comp, aead, may be added on top]
inside bh-disabled section as well as share a temporary/scratch buffer.
It will make it possible to remove some if not all custom managers of
the very same code pattern, some of which don't even try to share
pre-allocated tfms.

That's why I see some value in this crypto-pool thing.
If you NACK it, the alternative for TCP-AO patches would be to add just
another pool into net/ipv4/tcp.c that either copies TCP-MD5 code or
re-uses it.

I fail to see how your per-request keys patches would provide an API
alternative to this patch set. Still, users will have to manage
pre-allocated tfms and buffers.
I can actually see how your per-request keys would benefit *from* this
patch set: it will be much easier to wire per-req keys up to crypto_pool
to avoid per-CPU tfm allocation for algorithms you'll add support for.
In that case you won't have to patch crypto-pool users.

[1]:
https://lore.kernel.org/all/20221027204347.529913-1-dima@arista.com/T/#u

Thanks, waiting for your input,
          Dmitry

On 1/20/23 08:49, Herbert Xu wrote:
> On Thu, Jan 19, 2023 at 06:03:40PM +0000, Dmitry Safonov wrote:
>>
>> - net/ipv4/ah4.c could benefit from it: currently it allocates
>> crypto_alloc_ahash() per every connection, allocating user-specified
>> hash algorithm with ahash = crypto_alloc_ahash(x->aalg->alg_name, 0, 0),
>> which are not shared between each other and it doesn't provide
>> pre-allocated temporary/scratch buffer to calculate hash, so it uses
>> GFP_ATOMIC in ah_alloc_tmp()
>> - net/ipv6/ah6.c is copy'n'paste of the above
>> - net/ipv4/esp4.c and net/ipv6/esp6.c are more-or-less also copy'n'paste
>> with crypto_alloc_aead() instead of crypto_alloc_ahash()
> 
> No they should definitely not switch over to the pool model.  In
> fact, these provide the correct model that you should follow.
> 
> The correct model is to allocate the tfm on the control/slow path,
> and allocate requests on the fast path (or reuse existing memory,
> e.g., from the skb).

Ok, I see. Do you think, it's worth having a pool of tfms?

If not, I can proceed with TCP-AO patches set and implement pool of
ahash tfms that will be used only for TCP-MD5 and TCP-AO, does that
sound good to you?

I see that ahash tfm allocation doesn't eat a lot of memory, rather
little more than 100 bytes, but even so, I don't see why not saving some
memory "for free", especially if one can have thousands of keys over
different sockets. Where there's not much complexity in sharing tfms &
scratch buffers?

Thanks,
          Dmitry