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

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2023-02-09 at 16:34 +0000, Chuck Lever III wrote:
> 
> > On Feb 9, 2023, at 11:02 AM, Paolo Abeni <pabeni@redhat.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@kernel.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 think that adding a 'hi_free'/'hi_release' op inside the
handshake_info struct - and moving the handshake info deallocation
inside the 'core' could possibly simplify a bit the architecture.

Since it looks like there is a reasonable agreement on this path
(@Dave, @Eric, @Jakub: please educate me otherwise!), and no
clear/immediate show stoppers, I suggested start hammering some
documentation with an high level overview that will help also
understanding/reviewing the code.

> > 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?

Good point. Indeed it looks like that is not needed.

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

In the end I don't see a operations needing a full list traversal.
handshake_nl_msg_accept walk that, but it stops at netns/proto matching
which should be ~always /~very soon in the typical use-case. And as you
said it should be easy to avoid even that.

I think it could be useful limiting the number of pending handshake to
some maximum, to avoid problems in pathological/malicious scenarios.

Cheers,

Paolo