Date:   Mon, 26 Sep 2022 19:57:12 -0600
From:   David Ahern <dsahern@...nel.org>
To:     Dmitry Safonov <dima@...sta.com>, linux-kernel@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Leonard Crestez <cdleonard@...il.com>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Ard Biesheuvel <ardb@...nel.org>,
        Bob Gilligan <gilligan@...sta.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Ivan Delalande <colona@...sta.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Salam Noureddine <noureddine@...sta.com>,
        Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org
Subject: Re: [PATCH v2 00/35] net/tcp: Add TCP-AO support

On 9/23/22 2:25 PM, Dmitry Safonov wrote:
> On 9/23/22 21:12, Dmitry Safonov wrote:
>> Changes from v1:
>> - Building now with CONFIG_IPV6=n (kernel test robot <lkp@...el.com>)
>> - Added missing static declarations for local functions
>>   (kernel test robot <lkp@...el.com>)
>> - Addressed static analyzer and review comments by Dan Carpenter
>>   (thanks, they were very useful!)
>> - Fix elif without defined() for !CONFIG_TCP_AO
>> - Recursively build selftests/net/tcp_ao (Shuah Khan), patches in:
>>   https://lore.kernel.org/all/20220919201958.279545-1-dima@arista.com/T/#u
>> - Don't leak crypto_pool reference when TCP-MD5 key is modified/changed
>> - Add TCP-AO support for nettest.c and fcnal-test.sh
>>   (will be used for VRF testing in later versions)

The patchset is large enough, so deferring feature addons like VRF to a
follow on set is fine. That said, it needs to be clear that the UAPI
will support VRF from the onset, so please state how VRF support will be
added.

>>
>> Version 1: https://lore.kernel.org/all/20220818170005.747015-1-dima@arista.com/T/#u
> 
> I think it's worth answering the question: why am I continuing sending
> patches for TCP-AO support when there's already another proposal? [1]
> Pre-history how we end up with the second approach is here: [2]
> TLDR; we had a customer and a deadline to deliver, I've given reviews to
> Leonard, but in the end it seems to me what we got is worth submitting
> as it's better in my view in many aspects.
> 
> The biggest differences between two proposals, that I care about
> (design-decisions, not implementation details):

Thanks for the adding the comparison on the 2 implementations.

> 
> 1. Per-netns TCP-AO keys vs per-socket TCP-AO keys. The reasons why this
> proposal is using per-socket keys (that are added like TCP-MD5 keys with
> setsockopt()) are:
> - They scale better: you don't have to lookup in netns database for a
> key. This is a major thing for Arista: we have to support customers that
> want more than 1000 peers with possible multiple keys per-peer. This
> scales well when the keys are split by design for each socket on every
> established connection.
> - TCP-AO doesn't require CAP_NET_ADMIN for usage.
> - TCP-AO is not meant to be transparent (like ipsec tunnels) for
> applications. The users are BGP applications which already know what
> they need.
> - Leonard's proposal has weird semantics when setsockopt() on some
> socket will change keys on other sockets in that network namespace. It
> should have been rather netlink-managed API or something of the kind if
> the keys are per-netns.
> 
> 2. This proposal seeks to do less in kernel space and leave more
> decision-making to the userspace. It is another major disagreement with

I do agree that complexity should be in userspace.


> Leonard's proposal, which seeks to add a key lifetime, key rotation
> logic and all other business-logic details into the kernel, while here
> those decisions are left for the userspace.
> If I understood Leonard correctly, he is placing more things in kernel
> to simplify migration for user applications from TCP-MD5 to TCP-AO. I
> rather think that would be a job for a shared library if that's needed.
> As per my perception (1) was also done to relieve userspace from the job
> of removing an outdated key simultaneously from all users in netns,
> while in this proposal this job is left for userspace to use available
> IPC methods. Essentially, I think TCP-AO in kernel should do only
> minimum that can't be done "reasonably" in userspace. By "reasonably" I
> mean without moving the TCP-IP stack into userspace.
> 
> 3. Re-using existing kernel code vs copy'n'pasting it, leaving
> refactoring for later. I'm a big fan of reusing existing functions. I
> think lesser amount of code in the end reduces the burden of maintenance
> as well as simplifies the code (both reading and changing). I can see
> Leonard's point of simplifying backports to stable releases that he
> ships to customers, but I think any upstream proposal should add less
> code and try reusing more.
> 
> 4. Following RFC5925 closer to text. RFC says that rnext_key from the
> peer MUST be respected, as well as that current_key MUST not be removed
> on an established connection. In this proposal if the requirements of
> RFC can be met, they are followed, rather than broken.
> 
> 5. Using ahash instead of shash. If there's a hardware accelerator - why
> not using it? This proposal uses crypto_ahash through per-CPU pool of
> crypto requests (crypto_pool).
> 
> 6. Hash algorithm UAPI: magic constants vs hash name as char *. This is
> a thing I've asked Leonard multiple times and what he refuses to change
> in his patches: let the UAPI have `char tcpa_alg_name[64]' and just pass
> it to crypto_* layer. There is no need for #define MY_HASHING_ALGO 0x2
> and another in-kernel array to convert the magic number to algorithm
> string in order to pass it to crypto.
> The algorithm names are flexible: we already have customer's request to
> use other than RFC5926 required hashing algorithms. And I don't see any
> value in this middle-layer. This is already what kernel does, see for
> example, include/uapi/linux/xfrm.h, grep for alg_name.
> 
> 7. Adding traffic keys from the beginning. The proposal would be
> incomplete without having traffic keys: they are pre-calculated in this
> proposal, so the TCP stack doesn't have to do hashing twice (first for
> calculation of the traffic key) for every segment on established
> connections. This proposal has them naturally per-socket.
> 
> I think those are the biggest differences in the approaches and they are
> enough to submit a concurrent proposal. Salam, Francesco, please add if
> I've missed any other disagreement or major architectural/design
> difference in the proposals.
> 
>> In TODO (expect in next versions):
>> - selftests on older kernels (or with CONFIG_TCP_AO=n) should exit with
>>   SKIP, not FAIL
>> - Support VRFs in setsockopt()
>> - setsockopt() UAPI padding + a test that structures are of the same
>>   size on 32-bit as on 64-bit platforms
>> - IPv4-mapped-IPv6 addresses (might be working, but no selftest now)
>> - Remove CONFIG_TCP_AO dependency on CONFIG_TCP_MD5SIG
>> - Add TCP-AO static key
>> - Measure/benchmark TCP-AO and regular TCP connections
>> - setsockopt(TCP_REPAIR) with TCP-AO
> [..]
> [1]:
> https://lore.kernel.org/linux-crypto/cover.1662361354.git.cdleonard@gmail.com/
> [2]:
> https://lore.kernel.org/all/8097c38e-e88e-66ad-74d3-2f4a9e3734f4@arista.com/T/#u
> 
> Thanks,
>           Dmitry

Powered by blists - more mailing lists