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

[Dmitry Safonov]

+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:
> 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@xxxxxxxxxx: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@xxxxxxxxxx: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@xxxxxxxxxx/T/#u

[..]

Thanks,
          Dmitry