Re: [PATCH v3 07/11] SUNRPC: Add a connect worker function for TLS
From: Trond Myklebust <hidden>
Date: 2023-05-30 15:10:44
On Tue, 2023-05-30 at 10:08 -0400, Chuck Lever wrote:
quoted hunk ↗ jump to hunk
From: Chuck Lever <redacted> Introduce a connect worker function that will handle the AUTH_TLS probe and TLS handshake, once a TCP connection is established. Signed-off-by: Chuck Lever <redacted> Reviewed-by: Jeff Layton <jlayton@kernel.org> --- include/linux/sunrpc/xprtsock.h | 1 + net/sunrpc/xprtsock.c | 70 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-)diff --git a/include/linux/sunrpc/xprtsock.hb/include/linux/sunrpc/xprtsock.h index daef030f4848..574a6a5391ba 100644--- a/include/linux/sunrpc/xprtsock.h +++ b/include/linux/sunrpc/xprtsock.h@@ -60,6 +60,7 @@ struct sock_xprt {struct sockaddr_storage srcaddr; unsigned short srcport; int xprt_err; + struct rpc_clnt *clnt; /* * UDP socket buffer size parametersdiff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 6f2fc863b47e..7ea5984a52a3 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c@@ -2411,6 +2411,62 @@ static void xs_tcp_setup_socket(structwork_struct *work) current_restore_flags(pflags, PF_MEMALLOC); } +/** + * xs_tls_connect - establish a TLS session on a socket + * @work: queued work item + * + */ +static void xs_tls_connect(struct work_struct *work) +{ + struct sock_xprt *transport = + container_of(work, struct sock_xprt, connect_worker.work); + struct rpc_clnt *clnt; + + clnt = transport->clnt; + transport->clnt = NULL; + if (IS_ERR(clnt)) + goto out_unlock; + + xs_tcp_setup_socket(work); + + rpc_shutdown_client(clnt); + +out_unlock: + return; +} + +static void xs_set_transport_clnt(struct rpc_clnt *clnt, struct rpc_xprt *xprt) +{ + struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); + struct rpc_create_args args = { + .net = xprt->xprt_net, + .protocol = xprt->prot, + .address = (struct sockaddr *)&xprt->addr, + .addrsize = xprt->addrlen, + .timeout = clnt->cl_timeout, + .servername = xprt->servername, + .nodename = clnt->cl_nodename, + .program = clnt->cl_program, + .prognumber = clnt->cl_prog, + .version = clnt->cl_vers, + .authflavor = RPC_AUTH_TLS, + .cred = clnt->cl_cred, + .xprtsec = { + .policy = RPC_XPRTSEC_NONE, + }, + .flags = RPC_CLNT_CREATE_NOPING, + }; + + switch (xprt->xprtsec.policy) { + case RPC_XPRTSEC_TLS_ANON: + case RPC_XPRTSEC_TLS_X509: + transport->clnt = rpc_create(&args);
NACK. rpciod should not be calling rpc_create(). Why can't you pre- create this client at setup time?
quoted hunk ↗ jump to hunk
+ break; + default: + transport->clnt = ERR_PTR(-ENOTCONN); + } +} + /** * xs_connect - connect a socket to a remote endpoint * @xprt: pointer to transport structure@@ -2442,6 +2498,8 @@ static void xs_connect(struct rpc_xprt *xprt,struct rpc_task *task) } else dprintk("RPC: xs_connect scheduled xprt %p\n", xprt); + xs_set_transport_clnt(task->tk_client, xprt); + queue_delayed_work(xprtiod_workqueue, &transport->connect_worker, delay);@@ -3057,7 +3115,17 @@ static struct rpc_xprt *xs_setup_tcp(structxprt_create *args) INIT_WORK(&transport->recv_worker, xs_stream_data_receive_workfn); INIT_WORK(&transport->error_worker, xs_error_handle); - INIT_DELAYED_WORK(&transport->connect_worker, xs_tcp_setup_socket); + + xprt->xprtsec = args->xprtsec; + switch (args->xprtsec.policy) { + case RPC_XPRTSEC_NONE: + INIT_DELAYED_WORK(&transport->connect_worker, xs_tcp_setup_socket); + break; + case RPC_XPRTSEC_TLS_ANON: + case RPC_XPRTSEC_TLS_X509: + INIT_DELAYED_WORK(&transport->connect_worker, xs_tls_connect); + break; + }
The patch series seems to be accumulating all these identical switch statements in the TCP callbacks. Instead of whittling away at the TCP transport, why shouldn't we just define TLS/TCP this as its own transport class with its own 'struct xprt_class' and its own set of struct rpc_xprt_ops?
switch (addr->sa_family) {
case AF_INET:
-- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com