Re: [PATCH v4 01/21] net/tcp: Prepare tcp_md5sig_pool for TCP-AO

[Dmitry Safonov]

Hi Herbert,

On 2/20/23 09:41, Herbert Xu wrote:
> On Wed, Feb 15, 2023 at 06:33:15PM +0000, Dmitry Safonov wrote:
>> TCP-AO similarly to TCP-MD5 needs to allocate tfms on a slow-path, which
>> is setsockopt() and use crypto ahash requests on fast paths, which are
>> RX/TX softirqs. It as well needs a temporary/scratch buffer for
>> preparing the hashing request.
>>
>> Extend tcp_md5sig_pool to support other hashing algorithms than MD5.
>> Move it in a separate file.
>>
>> This patch was previously submitted as more generic crypto_pool [1],
>> but Herbert nacked making it generic crypto API. His view is that crypto
>> requests should be atomically allocated on fast-paths. So, in this
>> version I don't move this pool anywhere outside TCP, only extending it
>> for TCP-AO use-case. It can be converted once there will be per-request
>> hashing crypto keys.
>>
>> [1]: https://lore.kernel.org/all/20230118214111.394416-1-dima@xxxxxxxxxx/T/#u
>> Signed-off-by: Dmitry Safonov <dima@xxxxxxxxxx>
>> ---
>>  include/net/tcp.h        |  48 ++++--
>>  net/ipv4/Kconfig         |   4 +
>>  net/ipv4/Makefile        |   1 +
>>  net/ipv4/tcp.c           | 103 +++---------
>>  net/ipv4/tcp_ipv4.c      |  97 +++++++-----
>>  net/ipv4/tcp_minisocks.c |  21 ++-
>>  net/ipv4/tcp_sigpool.c   | 333 +++++++++++++++++++++++++++++++++++++++
>>  net/ipv6/tcp_ipv6.c      |  58 +++----
>>  8 files changed, 493 insertions(+), 172 deletions(-)
>>  create mode 100644 net/ipv4/tcp_sigpool.c
> 
> Please wait for my per-request hash work before you resubmit this.

Do you have a timeline for that work?
And if you don't mind I keep re-iterating, as I'm trying to address TCP
reviews and missed functionality/selftests.

> Once that's in place all you need is a single tfm for the whole
> system.

Unfortunately, not really: RFC5926 prescribes the mandatory-to-implement
MAC algorithms for TCP-AO: HMAC-SHA-1-96 and AES-128-CMAC-96. But since
the RFC was written sha1 is now more eligible for attacks as well as
RFC5925 has:
> The option should support algorithms other than the default, to
> allow agility over time.
> TCP-AO allows any desired algorithm, subject to TCP option
> space limitations, as noted in Section 2.2. The use of a set
> of MKTs allows separate connections to use different
> algorithms, both for the MAC and the KDF.

As well as from a customer's request we need to support more than two
required algorithms. So, this implementation let the user choose the
algorithm that is supported by crypto/ layer (more or less like xfrm does).

Which means, that it still has to support multiple tfms. I guess that
pool of tfms can be converted to use per-request keys quite easily.

> As to request pools what exactly is the point of that? Just kmalloc
> them on demand.

1) before your per-request key patches - it's not possible.
2) after your patches - my question would be: "is it better to
kmalloc(GFP_ATOMIC) in RX/TX for every signed TCP segment, rather than
pre-allocate it?"

The price of (2) may just well be negligible, but worth measuring before
switching.

Thanks,
          Dmitry