Thread (3 messages) 3 messages, 1 author, 2023-08-02

Re: [PATCH v9 net-next 00/23] net/tcp: Add TCP-AO support

From: Dmitry Safonov <hidden>
Date: 2023-08-02 19:37:36
Also in: lkml, netdev

+Cc: Simon

I've realized that he wasn't in Cc list, albeit provided valuable
feedback in v8. Sorry about it, definitely going to Cc on next versions.

On 8/2/23 18:26, Dmitry Safonov wrote:
quoted hunk ↗ jump to hunk
Hi,

This is version 9 of TCP-AO support. It's based on net-next as
there's a trivial conflict with the commit dfa2f0483360 ("tcp: get rid
of sysctl_tcp_adv_win_scale").

Most of the changes in this version address Simon's reviews and polish
of patch set to please netdev/patchwork. I ran static analyzers over it,
there's currently only one warning introduced, which is Sparse's context
imbalance in tcp_sigpool_start(). I've spent some time trying to silence
it, here are my findings:
- __cond_acquires() is broken: refcount_dec_and_lock() produces Sparse warning
- tried __acquires() + __releases(), as in bpf_sk_storage_map_seq_find_next(),
  yet it doesn't silence Sparse
- I thought about moving rcu_read_unlock_bh() out of tcp_sigpool_start(),
  forcing the callers to call tcp_sigpool_end() even on error-paths, but:
  it feels wrong semantically and I'd have to initialize @c on error-case
  and check it in tcp_sigpool_end(). That feels even more wrong.
I've placed __cond_acquires() to tcp_sigpool_start() definition,
expecting that Sparse may be fixed in future to do proper thing.
Worth mentioning that it also complains about many other functions
including: sk_clone_lock(), sk_free_unlock_clone(), tcp_conn_request()
and etc.

Also, more checkpatch.pl warnings addressed, but yet I've left the ones
that are more personal preferences (i.e. 80 columns limit). Please, ping
me if you have a strong feeling about one of them.

Worth mentioning removing in-kernel wiring for TCP-AO key port matching:
it was restricted in uAPI and still it is. Removing from initial TCP-AO
implementation port matching as it can be added post-merge.

The following changes since commit 34093c9fa05df24558d1e2c5d32f7f93b2c97ee9:

  net: Remove duplicated include in mac.c (2023-08-02 11:42:47 +0100)

are available in the Git repository at:

  git@github.com:0x7f454c46/linux.git tcp-ao-v9

for you to fetch changes up to c1cf20fddf71a9ae9f07cb04a5a1efcce156c5ab:

  Documentation/tcp: Add TCP-AO documentation (2023-08-02 17:28:15 +0100)

----------------------------------------------------------------

And another branch with selftests, that will be sent later separately:

  git@github.com:0x7f454c46/linux.git tcp-ao-v9-with-selftests

Thanks for your time and reviews,
         Dmitry
--- Changelog ---
Changes from v8:
- Based on net-next
- Now doing git request-pull, rather than GitHub URLs
- Fix tmp_key buffer leak, introduced in v7 (Simon)
- More checkpatch.pl warning fixes (even to the code that existed but
  was touched)
- More reverse Xmas tree declarations (Simon)
- static code analysis fixes
- Removed TCP-AO key port matching code
- Removed `inline' for for static functions in .c files to make
  netdev/source_inline happy (I didn't know it's a thing)
- Moved tcp_ao_do_lookup() to a commit that uses it (Simon)
- __tcp_ao_key_cmp(): prefixlen is bits, but memcmp() uses bytes
- Added TCP port matching limitation to Documentation/networking/tcp_ao.rst

Version 8: https://lore.kernel.org/all/20230719202631.472019-1-dima@arista.com/T/#u (local)
[..]

Thanks,
          Dmitry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help