Re: [PATCH v3 07/11] SUNRPC: Add a connect worker function for TLS

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2023-05-30 at 10:08 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> 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 <chuck.lever@oracle.com>
> 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?

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