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@xxxxxxxxx>) >> - Added missing static declarations for local functions >> (kernel test robot <lkp@xxxxxxxxx>) >> - 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@xxxxxxxxxx/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@xxxxxxxxxx/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@xxxxxxxxx/ > [2]: > https://lore.kernel.org/all/8097c38e-e88e-66ad-74d3-2f4a9e3734f4@xxxxxxxxxx/T/#u > > Thanks, > Dmitry
