Thread (13 messages) 13 messages, 2 authors, 2023-05-30

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.h
b/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 parameters
diff --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(struct
work_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(struct
xprt_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

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help