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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 2 Aug 2023 20:37:19 +0100
From: Dmitry Safonov <dima@...sta.com>
To: David Ahern <dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
 "David S. Miller" <davem@...emloft.net>,
 Simon Horman <simon.horman@...igine.com>
Cc: linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
 Ard Biesheuvel <ardb@...nel.org>, Bob Gilligan <gilligan@...sta.com>,
 Dan Carpenter <error27@...il.com>, David Laight <David.Laight@...lab.com>,
 Dmitry Safonov <0x7f454c46@...il.com>, Donald Cassidy <dcassidy@...hat.com>,
 Eric Biggers <ebiggers@...nel.org>, "Eric W. Biederman"
 <ebiederm@...ssion.com>, Francesco Ruggeri <fruggeri05@...il.com>,
 "Gaillardetz, Dominik" <dgaillar@...na.com>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
 Ivan Delalande <colona@...sta.com>, Leonard Crestez <cdleonard@...il.com>,
 Salam Noureddine <noureddine@...sta.com>,
 "Tetreault, Francois" <ftetreau@...na.com>, netdev@...r.kernel.org,
 Steen Hegelund <Steen.Hegelund@...rochip.com>,
 Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org
Subject: Re: [PATCH v9 net-next 00/23] net/tcp: Add TCP-AO support

+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@...hub.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@...hub.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

[..]

Thanks,
          Dmitry