netdev - Re: [PATCH v3 1/2] net/handshake: Create a NETLINK service for handling handshake requests

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:   Thu, 9 Feb 2023 16:34:58 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Paolo Abeni <pabeni@...hat.com>
CC:     Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        "hare@...e.com" <hare@...e.com>,
        David Howells <dhowells@...hat.com>,
        Benjamin Coddington <bcodding@...hat.com>,
        Olga Kornievskaia <kolga@...app.com>,
        "jmeneghi@...hat.com" <jmeneghi@...hat.com>
Subject: Re: [PATCH v3 1/2] net/handshake: Create a NETLINK service for
 handling handshake requests



> On Feb 9, 2023, at 11:02 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> 
> On Thu, 2023-02-09 at 15:43 +0000, Chuck Lever III wrote:
>>> On Feb 9, 2023, at 1:00 AM, Jakub Kicinski <kuba@...nel.org> wrote:
>>> 
>>> On Tue, 07 Feb 2023 16:41:13 -0500 Chuck Lever wrote:
>>>> diff --git a/tools/include/uapi/linux/netlink.h
>>>> b/tools/include/uapi/linux/netlink.h
>>>> index 0a4d73317759..a269d356f358 100644
>>>> --- a/tools/include/uapi/linux/netlink.h
>>>> +++ b/tools/include/uapi/linux/netlink.h
>>>> @@ -29,6 +29,7 @@
>>>> #define NETLINK_RDMA		20
>>>> #define NETLINK_CRYPTO		21	/* Crypto layer */
>>>> #define NETLINK_SMC		22	/* SMC monitoring */
>>>> +#define NETLINK_HANDSHAKE	23	/* transport layer sec
>>>> handshake requests */
>>> 
>>> The extra indirection of genetlink introduces some complications?
>> 
>> I don't think it does, necessarily. But neither does it seem
>> to add any value (for this use case). <shrug>
> 
> To me it introduces a good separation between the handshake mechanism
> itself and the current subject (sock).
> 
> IIRC the previous version allowed the user-space to create a socket of
> the HANDSHAKE family which in turn accept()ed tcp sockets. That kind of
> construct - assuming I interpreted it correctly - did not sound right
> to me.
> 
> Back to these patches, they looks sane to me, even if the whole
> architecture is a bit hard to follow, given the non trivial cross
> references between the patches - I can likely have missed some relevant
> point.

One of the original goals was to support other security protocols
besides TLS v1.3, which is why the code is split between two
patches. I know that is cumbersome for some review workflows.

Now is a good time to simplify, if we see a sensible opportunity
to do so.


> I'm wondering if this approach scales well enough with the number of
> concurrent handshakes: the single list looks like a potential bottle-
> neck.

It's not clear how much scaling is needed. I don't have a strong
sense of how frequently a busy storage server will need a handshake,
for instance, but it seems like it would be relatively less frequent
than, say, I/O. Network storage connections are typically long-lived,
unlike http.

In terms of scalability, I am a little more concerned about the
handshake_mutex. Maybe that isn't needed since the pending list is
spinlock protected?

All that said, the single pending list can be replaced easily. It
would be straightforward to move it into struct net, for example.


--
Chuck Lever