[PATCH v2 00/15] RPC-with-TLS client side

[PATCH v2 00/15] RPC-with-TLS client side

[Chuck Lever]

Now that the initial v5.19 merge window has closed, it's time for
another round of review for RPC-with-TLS support in the Linux NFS
client. This is just the RPC-specific portions. The full series is
available in the "topic-rpc-with-tls-upcall" branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

I've taken two or three steps towards implementing the architecture
Trond requested during the last review. There is now a two-stage
connection establishment process so that the upper level can use
XPRT_CONNECTED to determine when a TLS session is ready to use.
There are probably additional changes and simplifications that can
be made. Please review and provide feedback.

I wanted to make more progress on client-side authentication (ie,
passing an x.509 cert from the client to the server) but NFSD bugs
have taken all my time for the past few weeks.


Changes since v1:
- Rebased on v5.18
- Re-ordered so generic fixes come first
- Addressed some of Trond's review comments

---

Chuck Lever (15):
      SUNRPC: Fail faster on bad verifier
      SUNRPC: Widen rpc_task::tk_flags
      SUNRPC: Replace dprintk() call site in xs_data_ready
      NFS: Replace fs_context-related dprintk() call sites with tracepoints
      SUNRPC: Plumb an API for setting transport layer security
      SUNRPC: Trace the rpc_create_args
      SUNRPC: Refactor rpc_call_null_helper()
      SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
      SUNRPC: Ignore data_ready callbacks during TLS handshakes
      SUNRPC: Capture cmsg metadata on client-side receive
      SUNRPC: Add a connect worker function for TLS
      SUNRPC: Add RPC-with-TLS support to xprtsock.c
      SUNRPC: Add RPC-with-TLS tracepoints
      NFS: Have struct nfs_client carry a TLS policy field
      NFS: Add an "xprtsec=" NFS mount option


 fs/nfs/client.c                 |  14 ++
 fs/nfs/fs_context.c             |  65 +++++--
 fs/nfs/internal.h               |   2 +
 fs/nfs/nfs3client.c             |   1 +
 fs/nfs/nfs4client.c             |  16 +-
 fs/nfs/nfstrace.h               |  77 ++++++++
 fs/nfs/super.c                  |   7 +
 include/linux/nfs_fs_sb.h       |   5 +-
 include/linux/sunrpc/auth.h     |   1 +
 include/linux/sunrpc/clnt.h     |  15 +-
 include/linux/sunrpc/sched.h    |  32 ++--
 include/linux/sunrpc/xprt.h     |   2 +
 include/linux/sunrpc/xprtsock.h |   4 +
 include/net/tls.h               |   2 +
 include/trace/events/sunrpc.h   | 157 ++++++++++++++--
 net/sunrpc/Makefile             |   2 +-
 net/sunrpc/auth.c               |   2 +-
 net/sunrpc/auth_tls.c           | 120 +++++++++++++
 net/sunrpc/clnt.c               |  34 ++--
 net/sunrpc/debugfs.c            |   2 +-
 net/sunrpc/xprtsock.c           | 310 +++++++++++++++++++++++++++++++-
 21 files changed, 805 insertions(+), 65 deletions(-)
 create mode 100644 net/sunrpc/auth_tls.c

--
Chuck Lever

[PATCH v2 01/15] SUNRPC: Fail faster on bad verifier

[Chuck Lever]

A bad verifier is not a garbage argument, it's an authentication
failure. Retrying it doesn't make the problem go away, and delays
upper layer recovery steps.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/clnt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e2c6eca0271b..ed13d55df720 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2649,7 +2649,7 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
 
 out_verifier:
 	trace_rpc_bad_verifier(task);
-	goto out_garbage;
+	goto out_err;
 
 out_msg_denied:
 	error = -EACCES;

[PATCH v2 02/15] SUNRPC: Widen rpc_task::tk_flags

[Chuck Lever]

There is just one unused bit left in rpc_task::tk_flags, and I will
need two in subsequent patches. Double the width of the field to
accommodate more flag bits.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/clnt.h  |    6 ++++--
 include/linux/sunrpc/sched.h |   32 ++++++++++++++++----------------
 net/sunrpc/clnt.c            |   11 ++++++-----
 net/sunrpc/debugfs.c         |    2 +-
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 90501404fa49..cbdd20dc84b7 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -193,11 +193,13 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
 			     unsigned int hdrsize);
 void		rpc_call_start(struct rpc_task *);
 int		rpc_call_async(struct rpc_clnt *clnt,
-			       const struct rpc_message *msg, int flags,
+			       const struct rpc_message *msg,
+			       unsigned int flags,
 			       const struct rpc_call_ops *tk_ops,
 			       void *calldata);
 int		rpc_call_sync(struct rpc_clnt *clnt,
-			      const struct rpc_message *msg, int flags);
+			      const struct rpc_message *msg,
+			      unsigned int flags);
 struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred,
 			       int flags);
 int		rpc_restart_call_prepare(struct rpc_task *);
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 1d7a3e51b795..d4b7ebd0a99c 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -82,7 +82,7 @@ struct rpc_task {
 	ktime_t			tk_start;	/* RPC task init timestamp */
 
 	pid_t			tk_owner;	/* Process id for batching tasks */
-	unsigned short		tk_flags;	/* misc flags */
+	unsigned int		tk_flags;	/* misc flags */
 	unsigned short		tk_timeouts;	/* maj timeouts */
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) || IS_ENABLED(CONFIG_TRACEPOINTS)
@@ -112,27 +112,27 @@ struct rpc_task_setup {
 	const struct rpc_call_ops *callback_ops;
 	void *callback_data;
 	struct workqueue_struct *workqueue;
-	unsigned short flags;
+	unsigned int flags;
 	signed char priority;
 };
 
 /*
  * RPC task flags
  */
-#define RPC_TASK_ASYNC		0x0001		/* is an async task */
-#define RPC_TASK_SWAPPER	0x0002		/* is swapping in/out */
-#define RPC_TASK_MOVEABLE	0x0004		/* nfs4.1+ rpc tasks */
-#define RPC_TASK_NULLCREDS	0x0010		/* Use AUTH_NULL credential */
-#define RPC_CALL_MAJORSEEN	0x0020		/* major timeout seen */
-#define RPC_TASK_DYNAMIC	0x0080		/* task was kmalloc'ed */
-#define	RPC_TASK_NO_ROUND_ROBIN	0x0100		/* send requests on "main" xprt */
-#define RPC_TASK_SOFT		0x0200		/* Use soft timeouts */
-#define RPC_TASK_SOFTCONN	0x0400		/* Fail if can't connect */
-#define RPC_TASK_SENT		0x0800		/* message was sent */
-#define RPC_TASK_TIMEOUT	0x1000		/* fail with ETIMEDOUT on timeout */
-#define RPC_TASK_NOCONNECT	0x2000		/* return ENOTCONN if not connected */
-#define RPC_TASK_NO_RETRANS_TIMEOUT	0x4000		/* wait forever for a reply */
-#define RPC_TASK_CRED_NOREF	0x8000		/* No refcount on the credential */
+#define RPC_TASK_ASYNC			0x00000001	/* is an async task */
+#define RPC_TASK_SWAPPER		0x00000002	/* is swapping in/out */
+#define RPC_TASK_MOVEABLE		0x00000004	/* nfs4.1+ rpc tasks */
+#define RPC_TASK_NULLCREDS		0x00000010	/* Use AUTH_NULL credential */
+#define RPC_CALL_MAJORSEEN		0x00000020	/* major timeout seen */
+#define RPC_TASK_DYNAMIC		0x00000080	/* task was kmalloc'ed */
+#define	RPC_TASK_NO_ROUND_ROBIN		0x00000100	/* send requests on "main" xprt */
+#define RPC_TASK_SOFT			0x00000200	/* Use soft timeouts */
+#define RPC_TASK_SOFTCONN		0x00000400	/* Fail if can't connect */
+#define RPC_TASK_SENT			0x00000800	/* message was sent */
+#define RPC_TASK_TIMEOUT		0x00001000	/* fail with ETIMEDOUT on timeout */
+#define RPC_TASK_NOCONNECT		0x00002000	/* return ENOTCONN if not connected */
+#define RPC_TASK_NO_RETRANS_TIMEOUT	0x00004000	/* wait forever for a reply */
+#define RPC_TASK_CRED_NOREF		0x00008000	/* No refcount on the credential */
 
 #define RPC_IS_ASYNC(t)		((t)->tk_flags & RPC_TASK_ASYNC)
 #define RPC_IS_SWAPPER(t)	((t)->tk_flags & RPC_TASK_SWAPPER)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index ed13d55df720..8fd45de66882 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1161,7 +1161,8 @@ EXPORT_SYMBOL_GPL(rpc_run_task);
  * @msg: RPC call parameters
  * @flags: RPC call flags
  */
-int rpc_call_sync(struct rpc_clnt *clnt, const struct rpc_message *msg, int flags)
+int rpc_call_sync(struct rpc_clnt *clnt, const struct rpc_message *msg,
+		  unsigned int flags)
 {
 	struct rpc_task	*task;
 	struct rpc_task_setup task_setup_data = {
@@ -1196,9 +1197,9 @@ EXPORT_SYMBOL_GPL(rpc_call_sync);
  * @tk_ops: RPC call ops
  * @data: user call data
  */
-int
-rpc_call_async(struct rpc_clnt *clnt, const struct rpc_message *msg, int flags,
-	       const struct rpc_call_ops *tk_ops, void *data)
+int rpc_call_async(struct rpc_clnt *clnt, const struct rpc_message *msg,
+		   unsigned int flags, const struct rpc_call_ops *tk_ops,
+		   void *data)
 {
 	struct rpc_task	*task;
 	struct rpc_task_setup task_setup_data = {
@@ -3079,7 +3080,7 @@ static void rpc_show_task(const struct rpc_clnt *clnt,
 	if (RPC_IS_QUEUED(task))
 		rpc_waitq = rpc_qname(task->tk_waitqueue);
 
-	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
+	printk(KERN_INFO "%5u %08x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
 		task->tk_pid, task->tk_flags, task->tk_status,
 		clnt, task->tk_rqstp, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index 7dc9cc929bfd..8b66235a3a49 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -30,7 +30,7 @@ tasks_show(struct seq_file *f, void *v)
 	if (task->tk_rqstp)
 		xid = be32_to_cpu(task->tk_rqstp->rq_xid);
 
-	seq_printf(f, "%5u %04x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
+	seq_printf(f, "%5u %08x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
 		task->tk_pid, task->tk_flags, task->tk_status,
 		clnt->cl_clid, xid, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),

[PATCH v2 03/15] SUNRPC: Replace dprintk() call site in xs_data_ready

[Chuck Lever]

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/sunrpc.h |   20 ++++++++++++++++++++
 net/sunrpc/xprtsock.c         |    6 ++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 3995c58a1c51..a66aa1f59ed8 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -1266,6 +1266,26 @@ TRACE_EVENT(xprt_reserve,
 	)
 );
 
+TRACE_EVENT(xs_data_ready,
+	TP_PROTO(
+		const struct rpc_xprt *xprt
+	),
+
+	TP_ARGS(xprt),
+
+	TP_STRUCT__entry(
+		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
+		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
+	),
+
+	TP_fast_assign(
+		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
+		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
+	),
+
+	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
+);
+
 TRACE_EVENT(xs_stream_read_data,
 	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
 
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 650102a9c86a..73fab802996d 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1378,7 +1378,7 @@ static void xs_udp_data_receive_workfn(struct work_struct *work)
 }
 
 /**
- * xs_data_ready - "data ready" callback for UDP sockets
+ * xs_data_ready - "data ready" callback for sockets
  * @sk: socket with data to read
  *
  */
@@ -1386,11 +1386,13 @@ static void xs_data_ready(struct sock *sk)
 {
 	struct rpc_xprt *xprt;
 
-	dprintk("RPC:       xs_data_ready...\n");
 	xprt = xprt_from_sock(sk);
 	if (xprt != NULL) {
 		struct sock_xprt *transport = container_of(xprt,
 				struct sock_xprt, xprt);
+
+		trace_xs_data_ready(xprt);
+
 		transport->old_data_ready(sk);
 		/* Any data means we had a useful conversation, so
 		 * then we don't need to delay the next reconnect

[PATCH v2 04/15] NFS: Replace fs_context-related dprintk() call sites with tracepoints

[Chuck Lever]

Contributed as part of the long patch series that converts NFS from
using dprintk to tracepoints for observability.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/fs_context.c |   25 ++++++++++-------
 fs/nfs/nfstrace.h   |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 9a16897e8dc6..35e400a557b9 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -21,6 +21,8 @@
 #include "nfs.h"
 #include "internal.h"
 
+#include "nfstrace.h"
+
 #define NFSDBG_FACILITY		NFSDBG_MOUNT
 
 #if IS_ENABLED(CONFIG_NFS_V3)
@@ -284,7 +286,7 @@ static int nfs_verify_server_address(struct sockaddr *addr)
 	}
 	}
 
-	dfprintk(MOUNT, "NFS: Invalid IP address specified\n");
+	trace_nfs_mount_addr_err(addr);
 	return 0;
 }
 
@@ -378,7 +380,7 @@ static int nfs_parse_security_flavors(struct fs_context *fc,
 	char *string = param->string, *p;
 	int ret;
 
-	dfprintk(MOUNT, "NFS: parsing %s=%s option\n", param->key, param->string);
+	trace_nfs_mount_assign(param->key, string);
 
 	while ((p = strsep(&string, ":")) != NULL) {
 		if (!*p)
@@ -480,7 +482,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 	unsigned int len;
 	int ret, opt;
 
-	dfprintk(MOUNT, "NFS:   parsing nfs mount option '%s'\n", param->key);
+	trace_nfs_mount_option(param);
 
 	opt = fs_parse(fc, nfs_fs_parameters, param, &result);
 	if (opt < 0)
@@ -683,6 +685,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 			return ret;
 		break;
 	case Opt_vers:
+		trace_nfs_mount_assign(param->key, param->string);
 		ret = nfs_parse_version_string(fc, param->string);
 		if (ret < 0)
 			return ret;
@@ -694,6 +697,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		break;
 
 	case Opt_proto:
+		trace_nfs_mount_assign(param->key, param->string);
 		protofamily = AF_INET;
 		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
 		case Opt_xprt_udp6:
@@ -729,6 +733,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		break;
 
 	case Opt_mountproto:
+		trace_nfs_mount_assign(param->key, param->string);
 		mountfamily = AF_INET;
 		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
 		case Opt_xprt_udp6:
@@ -751,6 +756,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		break;
 
 	case Opt_addr:
+		trace_nfs_mount_assign(param->key, param->string);
 		len = rpc_pton(fc->net_ns, param->string, param->size,
 			       &ctx->nfs_server.address,
 			       sizeof(ctx->nfs_server._address));
@@ -759,16 +765,19 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		ctx->nfs_server.addrlen = len;
 		break;
 	case Opt_clientaddr:
+		trace_nfs_mount_assign(param->key, param->string);
 		kfree(ctx->client_address);
 		ctx->client_address = param->string;
 		param->string = NULL;
 		break;
 	case Opt_mounthost:
+		trace_nfs_mount_assign(param->key, param->string);
 		kfree(ctx->mount_server.hostname);
 		ctx->mount_server.hostname = param->string;
 		param->string = NULL;
 		break;
 	case Opt_mountaddr:
+		trace_nfs_mount_assign(param->key, param->string);
 		len = rpc_pton(fc->net_ns, param->string, param->size,
 			       &ctx->mount_server.address,
 			       sizeof(ctx->mount_server._address));
@@ -846,7 +855,6 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		 */
 	case Opt_sloppy:
 		ctx->sloppy = true;
-		dfprintk(MOUNT, "NFS:   relaxing parsing rules\n");
 		break;
 	}
 
@@ -879,10 +887,8 @@ static int nfs_parse_source(struct fs_context *fc,
 	size_t len;
 	const char *end;
 
-	if (unlikely(!dev_name || !*dev_name)) {
-		dfprintk(MOUNT, "NFS: device name not specified\n");
+	if (unlikely(!dev_name || !*dev_name))
 		return -EINVAL;
-	}
 
 	/* Is the host name protected with square brakcets? */
 	if (*dev_name == '[') {
@@ -922,7 +928,7 @@ static int nfs_parse_source(struct fs_context *fc,
 	if (!ctx->nfs_server.export_path)
 		goto out_nomem;
 
-	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", ctx->nfs_server.export_path);
+	trace_nfs_mount_path(ctx->nfs_server.export_path);
 	return 0;
 
 out_bad_devname:
@@ -1116,7 +1122,6 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
 	return nfs_invalf(fc, "NFS: nfs_mount_data version supports only AUTH_SYS");
 
 out_nomem:
-	dfprintk(MOUNT, "NFS: not enough memory to handle mount options");
 	return -ENOMEM;
 
 out_no_address:
@@ -1248,7 +1253,7 @@ static int nfs4_parse_monolithic(struct fs_context *fc,
 	if (IS_ERR(c))
 		return PTR_ERR(c);
 	ctx->nfs_server.export_path = c;
-	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", c);
+	trace_nfs_mount_path(c);
 
 	c = strndup_user(data->client_addr.data, 16);
 	if (IS_ERR(c))
diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 012bd7339862..ccaeae42ee77 100644
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -1609,6 +1609,83 @@ TRACE_EVENT(nfs_fh_to_dentry,
 		)
 );
 
+TRACE_EVENT(nfs_mount_addr_err,
+	TP_PROTO(
+		const struct sockaddr *sap
+	),
+
+	TP_ARGS(sap),
+
+	TP_STRUCT__entry(
+		__array(unsigned char, addr, sizeof(struct sockaddr_in6))
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->addr, sap, sizeof(__entry->addr));
+	),
+
+	TP_printk("addr=%pISpc", __entry->addr)
+);
+
+TRACE_EVENT(nfs_mount_assign,
+	TP_PROTO(
+		const char *option,
+		const char *value
+	),
+
+	TP_ARGS(option, value),
+
+	TP_STRUCT__entry(
+		__string(option, option)
+		__string(value, value)
+	),
+
+	TP_fast_assign(
+		__assign_str(option, option);
+		__assign_str(value, value);
+	),
+
+	TP_printk("option %s=%s",
+		__get_str(option), __get_str(value)
+	)
+);
+
+TRACE_EVENT(nfs_mount_option,
+	TP_PROTO(
+		const struct fs_parameter *param
+	),
+
+	TP_ARGS(param),
+
+	TP_STRUCT__entry(
+		__string(option, param->key)
+	),
+
+	TP_fast_assign(
+		__assign_str(option, param->key);
+	),
+
+	TP_printk("option %s", __get_str(option))
+);
+
+TRACE_EVENT(nfs_mount_path,
+	TP_PROTO(
+		const char *path
+	),
+
+	TP_ARGS(path),
+
+	TP_STRUCT__entry(
+		__string(path, path)
+	),
+
+	TP_fast_assign(
+		__assign_str(path, path);
+	),
+
+	TP_printk("path='%s'", __get_str(path))
+);
+
 DECLARE_EVENT_CLASS(nfs_xdr_event,
 		TP_PROTO(
 			const struct xdr_stream *xdr,

[PATCH v2 05/15] SUNRPC: Plumb an API for setting transport layer security

[Chuck Lever]

Add an initial set of policies along with fields for upper layers to
pass the requested policy down to the transport layer.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/clnt.h |    9 +++++++++
 include/linux/sunrpc/xprt.h |    2 ++
 net/sunrpc/clnt.c           |    4 ++++
 3 files changed, 15 insertions(+)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index cbdd20dc84b7..85c2f810d4bb 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -58,6 +58,7 @@ struct rpc_clnt {
 				cl_noretranstimeo: 1,/* No retransmit timeouts */
 				cl_autobind : 1,/* use getport() */
 				cl_chatty   : 1;/* be verbose */
+	unsigned int		cl_xprtsec;	/* transport security policy */
 
 	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
 	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
@@ -139,6 +140,7 @@ struct rpc_create_args {
 	struct svc_xprt		*bc_xprt;	/* NFSv4.1 backchannel */
 	const struct cred	*cred;
 	unsigned int		max_connect;
+	unsigned int		xprtsec;
 };
 
 struct rpc_add_xprt_test {
@@ -162,6 +164,13 @@ struct rpc_add_xprt_test {
 #define RPC_CLNT_CREATE_REUSEPORT	(1UL << 11)
 #define RPC_CLNT_CREATE_CONNECTED	(1UL << 12)
 
+/* RPC transport layer security policies */
+enum {
+	RPC_XPRTSEC_NONE = 0,
+	RPC_XPRTSEC_TLS_X509,
+	RPC_XPRTSEC_TLS_PSK,
+};
+
 struct rpc_clnt *rpc_create(struct rpc_create_args *args);
 struct rpc_clnt	*rpc_bind_new_program(struct rpc_clnt *,
 				const struct rpc_program *, u32);
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 522bbf937957..d091ad2b7340 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -228,6 +228,7 @@ struct rpc_xprt {
 	 */
 	unsigned long		bind_timeout,
 				reestablish_timeout;
+	unsigned int		xprtsec;
 	unsigned int		connect_cookie;	/* A cookie that gets bumped
 						   every time the transport
 						   is reconnected */
@@ -332,6 +333,7 @@ struct xprt_create {
 	struct svc_xprt		*bc_xprt;	/* NFSv4.1 backchannel */
 	struct rpc_xprt_switch	*bc_xps;
 	unsigned int		flags;
+	unsigned int		xprtsec;
 };
 
 struct xprt_class {
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8fd45de66882..6dcc88d45f5d 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -385,6 +385,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
 	if (!clnt)
 		goto out_err;
 	clnt->cl_parent = parent ? : clnt;
+	clnt->cl_xprtsec = args->xprtsec;
 
 	err = rpc_alloc_clid(clnt);
 	if (err)
@@ -532,6 +533,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
 		.addrlen = args->addrsize,
 		.servername = args->servername,
 		.bc_xprt = args->bc_xprt,
+		.xprtsec = args->xprtsec,
 	};
 	char servername[48];
 	struct rpc_clnt *clnt;
@@ -726,6 +728,7 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,
 	struct rpc_clnt *parent;
 	int err;
 
+	args->xprtsec = clnt->cl_xprtsec;
 	xprt = xprt_create_transport(args);
 	if (IS_ERR(xprt))
 		return PTR_ERR(xprt);
@@ -2973,6 +2976,7 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
 
 	if (!xprtargs->ident)
 		xprtargs->ident = ident;
+	xprtargs->xprtsec = clnt->cl_xprtsec;
 	xprt = xprt_create_transport(xprtargs);
 	if (IS_ERR(xprt)) {
 		ret = PTR_ERR(xprt);

[PATCH v2 06/15] SUNRPC: Trace the rpc_create_args

[Chuck Lever]

Pass the upper layer's rpc_create_args to the rpc_clnt_new()
tracepoint so additional parts of the upper layer's request can be
recorded.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/sunrpc.h |   53 +++++++++++++++++++++++++++++++++--------
 net/sunrpc/clnt.c             |    2 +-
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index a66aa1f59ed8..986e135e314f 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -139,36 +139,69 @@ DEFINE_RPC_CLNT_EVENT(release);
 DEFINE_RPC_CLNT_EVENT(replace_xprt);
 DEFINE_RPC_CLNT_EVENT(replace_xprt_err);
 
+TRACE_DEFINE_ENUM(RPC_XPRTSEC_NONE);
+TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_X509);
+TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_PSK);
+
+#define rpc_show_xprtsec_policy(policy)					\
+	__print_symbolic(policy,					\
+		{ RPC_XPRTSEC_NONE,		"none" },		\
+		{ RPC_XPRTSEC_TLS_X509,		"tls-x509" },		\
+		{ RPC_XPRTSEC_TLS_PSK,		"tls-psk" })
+
+#define rpc_show_create_flags(flags)					\
+	__print_flags(flags, "|",					\
+		{ RPC_CLNT_CREATE_HARDRTRY,	"HARDRTRY" },		\
+		{ RPC_CLNT_CREATE_AUTOBIND,	"AUTOBIND" },		\
+		{ RPC_CLNT_CREATE_NONPRIVPORT,	"NONPRIVPORT" },	\
+		{ RPC_CLNT_CREATE_NOPING,	"NOPING" },		\
+		{ RPC_CLNT_CREATE_DISCRTRY,	"DISCRTRY" },		\
+		{ RPC_CLNT_CREATE_QUIET,	"QUIET" },		\
+		{ RPC_CLNT_CREATE_INFINITE_SLOTS,			\
+						"INFINITE_SLOTS" },	\
+		{ RPC_CLNT_CREATE_NO_IDLE_TIMEOUT,			\
+						"NO_IDLE_TIMEOUT" },	\
+		{ RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT,			\
+						"NO_RETRANS_TIMEOUT" },	\
+		{ RPC_CLNT_CREATE_SOFTERR,	"SOFTERR" },		\
+		{ RPC_CLNT_CREATE_REUSEPORT,	"REUSEPORT" })
+
 TRACE_EVENT(rpc_clnt_new,
 	TP_PROTO(
 		const struct rpc_clnt *clnt,
 		const struct rpc_xprt *xprt,
-		const char *program,
-		const char *server
+		const struct rpc_create_args *args
 	),
 
-	TP_ARGS(clnt, xprt, program, server),
+	TP_ARGS(clnt, xprt, args),
 
 	TP_STRUCT__entry(
 		__field(unsigned int, client_id)
+		__field(unsigned long, xprtsec)
+		__field(unsigned long, flags)
+		__string(program, clnt->cl_program->name)
+		__string(server, xprt->servername)
 		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
 		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
-		__string(program, program)
-		__string(server, server)
 	),
 
 	TP_fast_assign(
 		__entry->client_id = clnt->cl_clid;
+		__entry->xprtsec = args->xprtsec;
+		__entry->flags = args->flags;
+		__assign_str(program, clnt->cl_program->name);
+		__assign_str(server, xprt->servername);
 		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
 		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
-		__assign_str(program, program);
-		__assign_str(server, server);
 	),
 
-	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER
-		  " peer=[%s]:%s program=%s server=%s",
+	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER " peer=[%s]:%s"
+		" program=%s server=%s xprtsec=%s flags=%s",
 		__entry->client_id, __get_str(addr), __get_str(port),
-		__get_str(program), __get_str(server))
+		__get_str(program), __get_str(server),
+		rpc_show_xprtsec_policy(__entry->xprtsec),
+		rpc_show_create_flags(__entry->flags)
+	)
 );
 
 TRACE_EVENT(rpc_clnt_new_err,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6dcc88d45f5d..0ca86c92968f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -435,7 +435,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
 	if (parent)
 		refcount_inc(&parent->cl_count);
 
-	trace_rpc_clnt_new(clnt, xprt, program->name, args->servername);
+	trace_rpc_clnt_new(clnt, xprt, args);
 	return clnt;
 
 out_no_path:

[PATCH v2 07/15] SUNRPC: Refactor rpc_call_null_helper()

[Chuck Lever]

I'm about to add a use case that does not want RPC_TASK_NULLCREDS.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/clnt.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 0ca86c92968f..ade83ee13bac 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2751,8 +2751,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt,
 		.rpc_op_cred = cred,
 		.callback_ops = ops ?: &rpc_null_ops,
 		.callback_data = data,
-		.flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
-			 RPC_TASK_NULLCREDS,
+		.flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN,
 	};
 
 	return rpc_run_task(&task_setup_data);
@@ -2760,7 +2759,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt,
 
 struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags)
 {
-	return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL);
+	return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS,
+				    NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(rpc_call_null);
 
@@ -2860,9 +2860,11 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
 		goto success;
 	}
 
-	task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC,
-			&rpc_cb_add_xprt_call_ops, data);
+	task = rpc_call_null_helper(clnt, xprt, NULL,
+				    RPC_TASK_ASYNC | RPC_TASK_NULLCREDS,
+				    &rpc_cb_add_xprt_call_ops, data);
 	data->xps->xps_nunique_destaddr_xprts++;
+
 	rpc_put_task(task);
 success:
 	return 1;
@@ -2903,7 +2905,8 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *clnt,
 		goto out_err;
 
 	/* Test the connection */
-	task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL);
+	task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS,
+				    NULL, NULL);
 	if (IS_ERR(task)) {
 		status = PTR_ERR(task);
 		goto out_err;

[PATCH v2 08/15] SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor

[Chuck Lever]

The new authentication flavor is used only to discover peer support
for RPC-over-TLS.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/auth.h |    1 
 net/sunrpc/Makefile         |    2 -
 net/sunrpc/auth.c           |    2 -
 net/sunrpc/auth_tls.c       |  120 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100644 net/sunrpc/auth_tls.c

diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
index 3e6ce288a7fc..1f13d923f439 100644
--- a/include/linux/sunrpc/auth.h
+++ b/include/linux/sunrpc/auth.h
@@ -144,6 +144,7 @@ struct rpc_credops {
 
 extern const struct rpc_authops	authunix_ops;
 extern const struct rpc_authops	authnull_ops;
+extern const struct rpc_authops	authtls_ops;
 
 int __init		rpc_init_authunix(void);
 int __init		rpcauth_init_module(void);
diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
index 1c8de397d6ad..f89c10fe7e6a 100644
--- a/net/sunrpc/Makefile
+++ b/net/sunrpc/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_SUNRPC_GSS) += auth_gss/
 obj-$(CONFIG_SUNRPC_XPRT_RDMA) += xprtrdma/
 
 sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
-	    auth.o auth_null.o auth_unix.o \
+	    auth.o auth_null.o auth_tls.o auth_unix.o \
 	    svc.o svcsock.o svcauth.o svcauth_unix.o \
 	    addr.o rpcb_clnt.o timer.o xdr.o \
 	    sunrpc_syms.o cache.o rpc_pipe.o sysfs.o \
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 682fcd24bf43..91c21a1c7cdc 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -32,7 +32,7 @@ static unsigned int auth_hashbits = RPC_CREDCACHE_DEFAULT_HASHBITS;
 static const struct rpc_authops __rcu *auth_flavors[RPC_AUTH_MAXFLAVOR] = {
 	[RPC_AUTH_NULL] = (const struct rpc_authops __force __rcu *)&authnull_ops,
 	[RPC_AUTH_UNIX] = (const struct rpc_authops __force __rcu *)&authunix_ops,
-	NULL,			/* others can be loadable modules */
+	[RPC_AUTH_TLS]  = (const struct rpc_authops __force __rcu *)&authtls_ops,
 };
 
 static LIST_HEAD(cred_unused);
diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
new file mode 100644
index 000000000000..cf4185f188e9
--- /dev/null
+++ b/net/sunrpc/auth_tls.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, 2022 Oracle.  All rights reserved.
+ *
+ * The AUTH_TLS credential is used only to probe a remote peer
+ * for RPC-over-TLS support.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/sunrpc/clnt.h>
+
+static const char *starttls_token = "STARTTLS";
+static const size_t starttls_len = 8;
+
+static struct rpc_auth tls_auth;
+static struct rpc_cred tls_cred;
+
+static struct rpc_auth *tls_create(const struct rpc_auth_create_args *args,
+				   struct rpc_clnt *clnt)
+{
+	refcount_inc(&tls_auth.au_count);
+	return &tls_auth;
+}
+
+static void tls_destroy(struct rpc_auth *auth)
+{
+}
+
+static struct rpc_cred *tls_lookup_cred(struct rpc_auth *auth,
+					struct auth_cred *acred, int flags)
+{
+	return get_rpccred(&tls_cred);
+}
+
+static void tls_destroy_cred(struct rpc_cred *cred)
+{
+}
+
+static int tls_match(struct auth_cred *acred, struct rpc_cred *cred, int taskflags)
+{
+	return 1;
+}
+
+static int tls_marshal(struct rpc_task *task, struct xdr_stream *xdr)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, 4 * XDR_UNIT);
+	if (!p)
+		return -EMSGSIZE;
+	/* Credential */
+	*p++ = rpc_auth_tls;
+	*p++ = xdr_zero;
+	/* Verifier */
+	*p++ = rpc_auth_null;
+	*p   = xdr_zero;
+	return 0;
+}
+
+static int tls_refresh(struct rpc_task *task)
+{
+	set_bit(RPCAUTH_CRED_UPTODATE, &task->tk_rqstp->rq_cred->cr_flags);
+	return 0;
+}
+
+static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
+{
+	__be32 *p;
+	void *str;
+
+	p = xdr_inline_decode(xdr, XDR_UNIT);
+	if (!p)
+		return -EIO;
+	if (*p != rpc_auth_null)
+		return -EIO;
+	if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
+		return -EIO;
+	if (memcmp(str, starttls_token, starttls_len))
+		return -EIO;
+	return 0;
+}
+
+const struct rpc_authops authtls_ops = {
+	.owner		= THIS_MODULE,
+	.au_flavor	= RPC_AUTH_TLS,
+	.au_name	= "NULL",
+	.create		= tls_create,
+	.destroy	= tls_destroy,
+	.lookup_cred	= tls_lookup_cred,
+};
+
+static struct rpc_auth tls_auth = {
+	.au_cslack	= NUL_CALLSLACK,
+	.au_rslack	= NUL_REPLYSLACK,
+	.au_verfsize	= NUL_REPLYSLACK,
+	.au_ralign	= NUL_REPLYSLACK,
+	.au_ops		= &authtls_ops,
+	.au_flavor	= RPC_AUTH_TLS,
+	.au_count	= REFCOUNT_INIT(1),
+};
+
+static const struct rpc_credops tls_credops = {
+	.cr_name	= "AUTH_TLS",
+	.crdestroy	= tls_destroy_cred,
+	.crmatch	= tls_match,
+	.crmarshal	= tls_marshal,
+	.crwrap_req	= rpcauth_wrap_req_encode,
+	.crrefresh	= tls_refresh,
+	.crvalidate	= tls_validate,
+	.crunwrap_resp	= rpcauth_unwrap_resp_decode,
+};
+
+static struct rpc_cred tls_cred = {
+	.cr_lru		= LIST_HEAD_INIT(tls_cred.cr_lru),
+	.cr_auth	= &tls_auth,
+	.cr_ops		= &tls_credops,
+	.cr_count	= REFCOUNT_INIT(2),
+	.cr_flags	= 1UL << RPCAUTH_CRED_UPTODATE,
+};

[PATCH v2 09/15] SUNRPC: Ignore data_ready callbacks during TLS handshakes

[Chuck Lever]

The RPC header parser doesn't recognize TLS handshake traffic, so it
will close the connection prematurely with an error. To avoid that,
shunt the transport's data_ready callback when there is a TLS
handshake in progress.

The flag will be toggled by code added in a subsequent patch.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/xprtsock.h |    1 +
 net/sunrpc/xprtsock.c           |    6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
index 38284f25eddf..daef030f4848 100644
--- a/include/linux/sunrpc/xprtsock.h
+++ b/include/linux/sunrpc/xprtsock.h
@@ -90,5 +90,6 @@ struct sock_xprt {
 #define XPRT_SOCK_WAKE_DISCONNECT	(7)
 #define XPRT_SOCK_CONNECT_SENT	(8)
 #define XPRT_SOCK_NOSPACE	(9)
+#define XPRT_SOCK_IGNORE_RECV	(10)
 
 #endif /* _LINUX_SUNRPC_XPRTSOCK_H */
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 73fab802996d..0a521aee0b2f 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -703,6 +703,8 @@ static void xs_poll_check_readable(struct sock_xprt *transport)
 {
 
 	clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state);
+	if (test_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state))
+		return;
 	if (!xs_poll_socket_readable(transport))
 		return;
 	if (!test_and_set_bit(XPRT_SOCK_DATA_READY, &transport->sock_state))
@@ -1394,6 +1396,10 @@ static void xs_data_ready(struct sock *sk)
 		trace_xs_data_ready(xprt);
 
 		transport->old_data_ready(sk);
+
+		if (test_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state))
+			return;
+
 		/* Any data means we had a useful conversation, so
 		 * then we don't need to delay the next reconnect
 		 */

[PATCH v2 10/15] SUNRPC: Capture cmsg metadata on client-side receive

[Chuck Lever]

kTLS sockets use cmsg to report decryption errors and the need
for session re-keying. An "application data" message contains a ULP
payload, and that is passed along to the RPC client. An "alert"
message triggers connection reset. Everything else is discarded.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/net/tls.h             |    2 ++
 include/trace/events/sunrpc.h |   40 +++++++++++++++++++++++++++++++++
 net/sunrpc/xprtsock.c         |   49 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 6b1bf46daa34..54bccb2e4014 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -71,6 +71,8 @@ static inline struct tlsh_sock *tlsh_sk(struct sock *sk)
 
 #define TLS_CRYPTO_INFO_READY(info)	((info)->cipher_type)
 
+#define TLS_RECORD_TYPE_ALERT		0x15
+#define TLS_RECORD_TYPE_HANDSHAKE	0x16
 #define TLS_RECORD_TYPE_DATA		0x17
 
 #define TLS_AAD_SPACE_SIZE		13
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 986e135e314f..d7d07f3b850e 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -1319,6 +1319,46 @@ TRACE_EVENT(xs_data_ready,
 	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
 );
 
+/*
+ * From https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml
+ *
+ * Captured March 2022. Other values are unassigned or reserved.
+ */
+#define rpc_show_tls_content_type(type) \
+	__print_symbolic(type, \
+		{ 20,		"change cipher spec" }, \
+		{ 21,		"alert" }, \
+		{ 22,		"handshake" }, \
+		{ 23,		"application data" }, \
+		{ 24,		"heartbeat" }, \
+		{ 25,		"tls12_cid" }, \
+		{ 26,		"ACK" })
+
+TRACE_EVENT(xs_tls_contenttype,
+	TP_PROTO(
+		const struct rpc_xprt *xprt,
+		unsigned char ctype
+	),
+
+	TP_ARGS(xprt, ctype),
+
+	TP_STRUCT__entry(
+		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
+		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
+		__field(unsigned long, ctype)
+	),
+
+	TP_fast_assign(
+		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
+		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
+		__entry->ctype = ctype;
+	),
+
+	TP_printk("peer=[%s]:%s: %s", __get_str(addr), __get_str(port),
+		rpc_show_tls_content_type(__entry->ctype)
+	)
+);
+
 TRACE_EVENT(xs_stream_read_data,
 	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
 
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 0a521aee0b2f..c73af8f1c3d4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -47,6 +47,8 @@
 #include <net/checksum.h>
 #include <net/udp.h>
 #include <net/tcp.h>
+#include <net/tls.h>
+
 #include <linux/bvec.h>
 #include <linux/highmem.h>
 #include <linux/uio.h>
@@ -350,13 +352,56 @@ xs_alloc_sparse_pages(struct xdr_buf *buf, size_t want, gfp_t gfp)
 	return want;
 }
 
+static int
+xs_sock_process_cmsg(struct socket *sock, struct msghdr *msg,
+		     struct cmsghdr *cmsg, int ret)
+{
+	if (cmsg->cmsg_level == SOL_TLS &&
+	    cmsg->cmsg_type == TLS_GET_RECORD_TYPE) {
+		u8 content_type = *((u8 *)CMSG_DATA(cmsg));
+
+		trace_xs_tls_contenttype(xprt_from_sock(sock->sk), content_type);
+		switch (content_type) {
+		case TLS_RECORD_TYPE_DATA:
+			/* TLS sets EOR at the end of each application data
+			 * record, even though there might be more frames
+			 * waiting to be decrypted. */
+			msg->msg_flags &= ~MSG_EOR;
+			break;
+		case TLS_RECORD_TYPE_ALERT:
+			ret = -ENOTCONN;
+			break;
+		default:
+			ret = -EAGAIN;
+		}
+	}
+	return ret;
+}
+
+static int
+xs_sock_recv_cmsg(struct socket *sock, struct msghdr *msg, int flags)
+{
+	union {
+		struct cmsghdr	cmsg;
+		u8		buf[CMSG_SPACE(sizeof(u8))];
+	} u;
+	int ret;
+
+	msg->msg_control = &u;
+	msg->msg_controllen = sizeof(u);
+	ret = sock_recvmsg(sock, msg, flags);
+	if (msg->msg_controllen != sizeof(u))
+		ret = xs_sock_process_cmsg(sock, msg, &u.cmsg, ret);
+	return ret;
+}
+
 static ssize_t
 xs_sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags, size_t seek)
 {
 	ssize_t ret;
 	if (seek != 0)
 		iov_iter_advance(&msg->msg_iter, seek);
-	ret = sock_recvmsg(sock, msg, flags);
+	ret = xs_sock_recv_cmsg(sock, msg, flags);
 	return ret > 0 ? ret + seek : ret;
 }
 
@@ -382,7 +427,7 @@ xs_read_discard(struct socket *sock, struct msghdr *msg, int flags,
 		size_t count)
 {
 	iov_iter_discard(&msg->msg_iter, READ, count);
-	return sock_recvmsg(sock, msg, flags);
+	return xs_sock_recv_cmsg(sock, msg, flags);
 }
 
 #if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE

[PATCH v2 11/15] SUNRPC: Add a connect worker function for TLS

[Chuck Lever]

Introduce a connect worker function that will handle the AUTH_TLS
probe and TLS handshake after a TCP connection is established.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/xprtsock.h |    2 +
 net/sunrpc/xprtsock.c           |   86 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
index daef030f4848..e0b6009f1f69 100644
--- a/include/linux/sunrpc/xprtsock.h
+++ b/include/linux/sunrpc/xprtsock.h
@@ -76,6 +76,8 @@ struct sock_xprt {
 	void			(*old_state_change)(struct sock *);
 	void			(*old_write_space)(struct sock *);
 	void			(*old_error_report)(struct sock *);
+
+	struct rpc_clnt		*xprtsec_clnt;
 };
 
 /*
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index c73af8f1c3d4..a4fee00412d4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2422,6 +2422,73 @@ static void xs_tcp_setup_socket(struct work_struct *work)
 	current_restore_flags(pflags, PF_MEMALLOC);
 }
 
+#if IS_ENABLED(CONFIG_TLS)
+
+/**
+ * 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->xprtsec_clnt;
+	transport->xprtsec_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_xprtsec_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	= RPC_XPRTSEC_NONE,
+		.flags		= RPC_CLNT_CREATE_NOPING,
+	};
+
+	switch (xprt->xprtsec) {
+	case RPC_XPRTSEC_TLS_X509:
+	case RPC_XPRTSEC_TLS_PSK:
+		transport->xprtsec_clnt = rpc_create(&args);
+		break;
+	default:
+		transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
+	}
+}
+
+#else
+
+static void xs_set_xprtsec_clnt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
+{
+	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+
+	transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
+}
+
+#endif /*CONFIG_TLS */
+
 /**
  * xs_connect - connect a socket to a remote endpoint
  * @xprt: pointer to transport structure
@@ -2453,6 +2520,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_xprtsec_clnt(task->tk_client, xprt);
+
 	queue_delayed_work(xprtiod_workqueue,
 			&transport->connect_worker,
 			delay);
@@ -3068,7 +3137,22 @@ 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) {
+	case RPC_XPRTSEC_NONE:
+		INIT_DELAYED_WORK(&transport->connect_worker, xs_tcp_setup_socket);
+		break;
+	case RPC_XPRTSEC_TLS_X509:
+	case RPC_XPRTSEC_TLS_PSK:
+#if IS_ENABLED(CONFIG_TLS)
+		INIT_DELAYED_WORK(&transport->connect_worker, xs_tls_connect);
+#else
+		ret = ERR_PTR(-EACCES);
+		goto out_err;
+#endif
+		break;
+	}
 
 	switch (addr->sa_family) {
 	case AF_INET:

[PATCH v2 12/15] SUNRPC: Add RPC-with-TLS support to xprtsock.c

[Chuck Lever]

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/xprtsock.h |    1 
 net/sunrpc/xprtsock.c           |  243 ++++++++++++++++++++++++++++++++-------
 2 files changed, 201 insertions(+), 43 deletions(-)

diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
index e0b6009f1f69..eaf3d705f758 100644
--- a/include/linux/sunrpc/xprtsock.h
+++ b/include/linux/sunrpc/xprtsock.h
@@ -57,6 +57,7 @@ struct sock_xprt {
 	struct work_struct	error_worker;
 	struct work_struct	recv_worker;
 	struct mutex		recv_mutex;
+	struct completion	handshake_done;
 	struct sockaddr_storage	srcaddr;
 	unsigned short		srcport;
 	int			xprt_err;
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index a4fee00412d4..63fe97ede573 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -48,6 +48,7 @@
 #include <net/udp.h>
 #include <net/tcp.h>
 #include <net/tls.h>
+#include <net/tlsh.h>
 
 #include <linux/bvec.h>
 #include <linux/highmem.h>
@@ -197,6 +198,11 @@ static struct ctl_table sunrpc_table[] = {
  */
 #define XS_IDLE_DISC_TO		(5U * 60 * HZ)
 
+/*
+ * TLS handshake timeout.
+ */
+#define XS_TLS_HANDSHAKE_TO    (20U * HZ)
+
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 # undef  RPC_DEBUG_DATA
 # define RPCDBG_FACILITY	RPCDBG_TRANS
@@ -1254,6 +1260,8 @@ static void xs_reset_transport(struct sock_xprt *transport)
 	if (atomic_read(&transport->xprt.swapper))
 		sk_clear_memalloc(sk);
 
+	/* TODO: Maybe send a TLS Closure alert */
+
 	kernel_sock_shutdown(sock, SHUT_RDWR);
 
 	mutex_lock(&transport->recv_mutex);
@@ -2424,6 +2432,147 @@ static void xs_tcp_setup_socket(struct work_struct *work)
 
 #if IS_ENABLED(CONFIG_TLS)
 
+/**
+ * xs_tls_handshake_done - TLS handshake completion handler
+ * @data: address of xprt to wake
+ * @status: status of handshake
+ *
+ */
+static void xs_tls_handshake_done(void *data, int status)
+{
+	struct rpc_xprt *xprt = data;
+	struct sock_xprt *transport =
+				container_of(xprt, struct sock_xprt, xprt);
+
+	transport->xprt_err = status ? -EACCES : 0;
+	complete(&transport->handshake_done);
+	xprt_put(xprt);
+}
+
+static int xs_tls_handshake_sync(struct rpc_xprt *xprt, unsigned int xprtsec)
+{
+	struct sock_xprt *transport =
+				container_of(xprt, struct sock_xprt, xprt);
+	int rc;
+
+	init_completion(&transport->handshake_done);
+	set_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
+
+	transport->xprt_err = -ETIMEDOUT;
+	switch (xprtsec) {
+	case RPC_XPRTSEC_TLS_X509:
+		rc = tls_client_hello_x509(transport->sock,
+					   xs_tls_handshake_done, xprt_get(xprt),
+					   TLSH_DEFAULT_PRIORITIES, TLSH_NO_PEERID,
+					   TLSH_NO_CERT);
+		break;
+	case RPC_XPRTSEC_TLS_PSK:
+		rc = tls_client_hello_psk(transport->sock, xs_tls_handshake_done,
+					  xprt_get(xprt), TLSH_DEFAULT_PRIORITIES,
+					  TLSH_NO_PEERID);
+		break;
+	default:
+		rc = -EACCES;
+	}
+	if (rc)
+		goto out;
+
+	rc = wait_for_completion_interruptible_timeout(&transport->handshake_done,
+						       XS_TLS_HANDSHAKE_TO);
+	if (rc < 0)
+		goto out;
+
+	rc = transport->xprt_err;
+
+out:
+	xs_stream_reset_connect(transport);
+	clear_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
+	return rc;
+}
+
+/*
+ * Transfer the connected socket to @dst_transport, then mark that
+ * xprt CONNECTED.
+ */
+static int xs_tls_finish_connecting(struct rpc_xprt *src_xprt,
+				    struct sock_xprt *dst_transport)
+{
+	struct sock_xprt *src_transport =
+			container_of(src_xprt, struct sock_xprt, xprt);
+	struct rpc_xprt *dst_xprt = &dst_transport->xprt;
+
+	if (!dst_transport->inet) {
+		struct socket *sock = src_transport->sock;
+		struct sock *sk = sock->sk;
+
+		/* Avoid temporary address, they are bad for long-lived
+		 * connections such as NFS mounts.
+		 * RFC4941, section 3.6 suggests that:
+		 *    Individual applications, which have specific
+		 *    knowledge about the normal duration of connections,
+		 *    MAY override this as appropriate.
+		 */
+		if (xs_addr(dst_xprt)->sa_family == PF_INET6) {
+			ip6_sock_set_addr_preferences(sk,
+				IPV6_PREFER_SRC_PUBLIC);
+		}
+
+		xs_tcp_set_socket_timeouts(dst_xprt, sock);
+		tcp_sock_set_nodelay(sk);
+
+		lock_sock(sk);
+
+		/*
+		 * @sk is already connected, so it now has the RPC callbacks.
+		 * Reach into @src_transport to save the original ones.
+		 */
+		dst_transport->old_data_ready = src_transport->old_data_ready;
+		dst_transport->old_state_change = src_transport->old_state_change;
+		dst_transport->old_write_space = src_transport->old_write_space;
+		dst_transport->old_error_report = src_transport->old_error_report;
+		sk->sk_user_data = dst_xprt;
+
+		/* socket options */
+		sock_reset_flag(sk, SOCK_LINGER);
+
+		xprt_clear_connected(dst_xprt);
+
+		dst_transport->sock = sock;
+		dst_transport->inet = sk;
+		dst_transport->file = src_transport->file;
+
+		release_sock(sk);
+
+		/* Reset src_transport before shutting down its clnt */
+		mutex_lock(&src_transport->recv_mutex);
+		src_transport->inet = NULL;
+		src_transport->sock = NULL;
+		src_transport->file = NULL;
+
+		xprt_clear_connected(src_xprt);
+		xs_sock_reset_connection_flags(src_xprt);
+		xs_stream_reset_connect(src_transport);
+		mutex_unlock(&src_transport->recv_mutex);
+	}
+
+	if (!xprt_bound(dst_xprt))
+		return -ENOTCONN;
+
+	xs_set_memalloc(dst_xprt);
+
+	if (!xprt_test_and_set_connected(dst_xprt)) {
+		dst_xprt->connect_cookie++;
+		clear_bit(XPRT_SOCK_CONNECTING, &dst_transport->sock_state);
+		xprt_clear_connecting(dst_xprt);
+
+		dst_xprt->stat.connect_count++;
+		dst_xprt->stat.connect_time += (long)jiffies -
+					   dst_xprt->stat.connect_start;
+		xs_run_error_worker(dst_transport, XPRT_SOCK_WAKE_PENDING);
+	}
+	return 0;
+}
+
 /**
  * xs_tls_connect - establish a TLS session on a socket
  * @work: queued work item
@@ -2433,61 +2582,70 @@ static void xs_tls_connect(struct work_struct *work)
 {
 	struct sock_xprt *transport =
 		container_of(work, struct sock_xprt, connect_worker.work);
+	struct rpc_create_args args = {
+		.net		= transport->xprt.xprt_net,
+		.protocol	= transport->xprt.prot,
+		.address	= (struct sockaddr *)&transport->xprt.addr,
+		.addrsize	= transport->xprt.addrlen,
+		.timeout	= transport->xprtsec_clnt->cl_timeout,
+		.servername	= transport->xprt.servername,
+		.nodename	= transport->xprtsec_clnt->cl_nodename,
+		.program	= transport->xprtsec_clnt->cl_program,
+		.prognumber	= transport->xprtsec_clnt->cl_prog,
+		.version	= transport->xprtsec_clnt->cl_vers,
+		.authflavor	= RPC_AUTH_TLS,
+		.cred		= transport->xprtsec_clnt->cl_cred,
+		.xprtsec	= RPC_XPRTSEC_NONE,
+	};
+	unsigned int pflags = current->flags;
 	struct rpc_clnt *clnt;
+	struct rpc_xprt *xprt;
+	int status;
 
-	clnt = transport->xprtsec_clnt;
-	transport->xprtsec_clnt = NULL;
+	if (atomic_read(&transport->xprt.swapper))
+		current->flags |= PF_MEMALLOC;
+
+	xs_stream_start_connect(transport);
+
+	clnt = rpc_create(&args);
 	if (IS_ERR(clnt))
 		goto out_unlock;
+	rcu_read_lock();
+	xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
+	rcu_read_unlock();
 
-	xs_tcp_setup_socket(work);
+	status = xs_tls_handshake_sync(xprt, transport->xprt.xprtsec);
+	if (status)
+		goto out_close;
 
+	status = xs_tls_finish_connecting(xprt, transport);
+	if (status)
+		goto out_close;
+	trace_rpc_socket_connect(&transport->xprt, transport->sock, 0);
+
+	xprt_put(xprt);
 	rpc_shutdown_client(clnt);
 
 out_unlock:
+	xprt_unlock_connect(&transport->xprt, transport);
+	current_restore_flags(pflags, PF_MEMALLOC);
+	transport->xprtsec_clnt = NULL;
 	return;
-}
 
-static void xs_set_xprtsec_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	= RPC_XPRTSEC_NONE,
-		.flags		= RPC_CLNT_CREATE_NOPING,
-	};
-
-	switch (xprt->xprtsec) {
-	case RPC_XPRTSEC_TLS_X509:
-	case RPC_XPRTSEC_TLS_PSK:
-		transport->xprtsec_clnt = rpc_create(&args);
-		break;
-	default:
-		transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
-	}
-}
-
-#else
-
-static void xs_set_xprtsec_clnt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
-{
-	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+out_close:
+	xprt_put(xprt);
+	rpc_shutdown_client(clnt);
 
-	transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
+	/* xprt_force_disconnect() wakes tasks with a fixed tk_status code.
+	 * Wake them first here to ensure they get our tk_status code.
+	 */
+	xprt_wake_pending_tasks(&transport->xprt, status);
+	xs_tcp_force_close(&transport->xprt);
+	xprt_clear_connecting(&transport->xprt);
+	goto out_unlock;
 }
 
-#endif /*CONFIG_TLS */
+#endif /* CONFIG_TLS */
 
 /**
  * xs_connect - connect a socket to a remote endpoint
@@ -2520,8 +2678,7 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
 	} else
 		dprintk("RPC:       xs_connect scheduled xprt %p\n", xprt);
 
-	xs_set_xprtsec_clnt(task->tk_client, xprt);
-
+	transport->xprtsec_clnt = task->tk_client;
 	queue_delayed_work(xprtiod_workqueue,
 			&transport->connect_worker,
 			delay);

[PATCH v2 13/15] SUNRPC: Add RPC-with-TLS tracepoints

[Chuck Lever]

Auditing TLS handshakes is mandatory-to-implement for RPC-with-TLS.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/sunrpc.h |   44 +++++++++++++++++++++++++++++++++++++++++
 net/sunrpc/xprtsock.c         |   10 +++++++--
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index d7d07f3b850e..594e3188dfcd 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -1566,6 +1566,50 @@ TRACE_EVENT(rpcb_unregister,
 	)
 );
 
+/**
+ ** RPC-over-TLS tracepoints
+ **/
+
+DECLARE_EVENT_CLASS(rpc_tls_class,
+	TP_PROTO(
+		const struct rpc_clnt *clnt,
+		const struct rpc_xprt *xprt
+	),
+
+	TP_ARGS(clnt, xprt),
+
+	TP_STRUCT__entry(
+		__field(unsigned long, requested_policy)
+		__field(u32, version)
+		__string(servername, xprt->servername)
+		__string(progname, clnt->cl_program->name)
+	),
+
+	TP_fast_assign(
+		__entry->requested_policy = clnt->cl_xprtsec;
+		__entry->version = clnt->cl_vers;
+		__assign_str(servername, xprt->servername);
+		__assign_str(progname, clnt->cl_program->name)
+	),
+
+	TP_printk("server=%s %sv%u requested_policy=%s",
+		__get_str(servername), __get_str(progname), __entry->version,
+		rpc_show_xprtsec_policy(__entry->requested_policy)
+	)
+);
+
+#define DEFINE_RPC_TLS_EVENT(name) \
+	DEFINE_EVENT(rpc_tls_class, rpc_tls_##name, \
+			TP_PROTO( \
+				const struct rpc_clnt *clnt, \
+				const struct rpc_xprt *xprt \
+			), \
+			TP_ARGS(clnt, xprt))
+
+DEFINE_RPC_TLS_EVENT(unavailable);
+DEFINE_RPC_TLS_EVENT(not_started);
+
+
 /* Record an xdr_buf containing a fully-formed RPC message */
 DECLARE_EVENT_CLASS(svc_xdr_msg_class,
 	TP_PROTO(
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 63fe97ede573..508a7698c2e4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2608,15 +2608,21 @@ static void xs_tls_connect(struct work_struct *work)
 	xs_stream_start_connect(transport);
 
 	clnt = rpc_create(&args);
-	if (IS_ERR(clnt))
+	if (IS_ERR(clnt)) {
+		trace_rpc_tls_unavailable(transport->xprtsec_clnt,
+					  &transport->xprt);
 		goto out_unlock;
+	}
 	rcu_read_lock();
 	xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
 	rcu_read_unlock();
 
 	status = xs_tls_handshake_sync(xprt, transport->xprt.xprtsec);
-	if (status)
+	if (status) {
+		trace_rpc_tls_not_started(transport->xprtsec_clnt,
+					  &transport->xprt);
 		goto out_close;
+	}
 
 	status = xs_tls_finish_connecting(xprt, transport);
 	if (status)

[PATCH v2 14/15] NFS: Have struct nfs_client carry a TLS policy field

[Chuck Lever]

The new field is used to match struct nfs_clients that have the same
TLS policy setting.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/client.c           |    6 ++++++
 fs/nfs/internal.h         |    1 +
 fs/nfs/nfs3client.c       |    1 +
 fs/nfs/nfs4client.c       |   16 +++++++++++-----
 include/linux/nfs_fs_sb.h |    5 ++++-
 5 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index e828504cc396..0896e4f047d1 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -184,6 +184,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
 	clp->cl_net = get_net(cl_init->net);
 
 	clp->cl_principal = "*";
+	clp->cl_xprtsec = cl_init->xprtsec_policy;
 	return clp;
 
 error_cleanup:
@@ -326,6 +327,10 @@ static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *dat
 							   sap))
 				continue;
 
+		/* Match the xprt security policy */
+		if (clp->cl_xprtsec != data->xprtsec_policy)
+			continue;
+
 		refcount_inc(&clp->cl_count);
 		return clp;
 	}
@@ -675,6 +680,7 @@ static int nfs_init_server(struct nfs_server *server,
 		.cred = server->cred,
 		.nconnect = ctx->nfs_server.nconnect,
 		.init_flags = (1UL << NFS_CS_REUSEPORT),
+		.xprtsec_policy = NFS_CS_XPRTSEC_NONE,
 	};
 	struct nfs_client *clp;
 	int error;
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 7eefa16ed381..0a3512c39376 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -81,6 +81,7 @@ struct nfs_client_initdata {
 	struct net *net;
 	const struct rpc_timeout *timeparms;
 	const struct cred *cred;
+	unsigned int xprtsec_policy;
 };
 
 /*
diff --git a/fs/nfs/nfs3client.c b/fs/nfs/nfs3client.c
index 5601e47360c2..d7c5db1f5825 100644
--- a/fs/nfs/nfs3client.c
+++ b/fs/nfs/nfs3client.c
@@ -93,6 +93,7 @@ struct nfs_client *nfs3_set_ds_client(struct nfs_server *mds_srv,
 		.net = mds_clp->cl_net,
 		.timeparms = &ds_timeout,
 		.cred = mds_srv->cred,
+		.xprtsec_policy = mds_clp->cl_xprtsec,
 	};
 	struct nfs_client *clp;
 	char buf[INET6_ADDRSTRLEN + 1];
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 47a6cf892c95..682d47e5977b 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -895,7 +895,8 @@ static int nfs4_set_client(struct nfs_server *server,
 		int proto, const struct rpc_timeout *timeparms,
 		u32 minorversion, unsigned int nconnect,
 		unsigned int max_connect,
-		struct net *net)
+		struct net *net,
+		unsigned int xprtsec_policy)
 {
 	struct nfs_client_initdata cl_init = {
 		.hostname = hostname,
@@ -908,6 +909,7 @@ static int nfs4_set_client(struct nfs_server *server,
 		.net = net,
 		.timeparms = timeparms,
 		.cred = server->cred,
+		.xprtsec_policy = xprtsec_policy,
 	};
 	struct nfs_client *clp;
 
@@ -1156,7 +1158,8 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
 				ctx->minorversion,
 				ctx->nfs_server.nconnect,
 				ctx->nfs_server.max_connect,
-				fc->net_ns);
+				fc->net_ns,
+				NFS_CS_XPRTSEC_NONE);
 	if (error < 0)
 		return error;
 
@@ -1246,7 +1249,8 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc)
 				parent_client->cl_mvops->minor_version,
 				parent_client->cl_nconnect,
 				parent_client->cl_max_connect,
-				parent_client->cl_net);
+				parent_client->cl_net,
+				parent_client->cl_xprtsec);
 	if (!error)
 		goto init_server;
 #endif	/* IS_ENABLED(CONFIG_SUNRPC_XPRT_RDMA) */
@@ -1262,7 +1266,8 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc)
 				parent_client->cl_mvops->minor_version,
 				parent_client->cl_nconnect,
 				parent_client->cl_max_connect,
-				parent_client->cl_net);
+				parent_client->cl_net,
+				parent_client->cl_xprtsec);
 	if (error < 0)
 		goto error;
 
@@ -1335,7 +1340,8 @@ int nfs4_update_server(struct nfs_server *server, const char *hostname,
 	error = nfs4_set_client(server, hostname, sap, salen, buf,
 				clp->cl_proto, clnt->cl_timeout,
 				clp->cl_minorversion,
-				clp->cl_nconnect, clp->cl_max_connect, net);
+				clp->cl_nconnect, clp->cl_max_connect,
+				net, clp->cl_xprtsec);
 	clear_bit(NFS_MIG_TSM_POSSIBLE, &server->mig_status);
 	if (error != 0) {
 		nfs_server_insert_lists(server);
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 157d2bd6b241..7af8fba6f531 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -63,7 +63,10 @@ struct nfs_client {
 	u32			cl_minorversion;/* NFSv4 minorversion */
 	unsigned int		cl_nconnect;	/* Number of connections */
 	unsigned int		cl_max_connect; /* max number of xprts allowed */
-	const char *		cl_principal;  /* used for machine cred */
+	const char *		cl_principal;	/* used for machine cred */
+	unsigned int		cl_xprtsec;	/* xprt security policy */
+#define NFS_CS_XPRTSEC_NONE	(0)
+#define NFS_CS_XPRTSEC_TLS	(1)
 
 #if IS_ENABLED(CONFIG_NFS_V4)
 	struct list_head	cl_ds_clients; /* auth flavor data servers */

[PATCH v2 15/15] NFS: Add an "xprtsec=" NFS mount option

[Chuck Lever]

After some discussion, we decided that controlling transport layer
security policy should be separate from the setting for the user
authentication flavor. To accomplish this, add a new NFS mount
option to select a transport layer security policy for RPC
operations associated with the mount point.

  xprtsec=none     - Transport layer security is disabled.

  xprtsec=tls      - Establish an encryption-only TLS session. If
                     the initial handshake fails, the mount fails.
                     If TLS is not available on a reconnect, drop
                     the connection and try again.

The mount.nfs command will provide an addition setting:

  xprtsec=auto     - Try to establish a TLS session, but proceed
                     with no transport layer security if that fails.

Updates to mount.nfs and nfs(5) will be sent under separate cover.

Future work:

To support client peer authentication, the plan is to add another
xprtsec= choice called "mtls" which will require a second mount
option that specifies the pathname of a directory containing the
private key and an x.509 certificate.

Similarly, pre-shared key authentication can be supported by adding
support for "xprtsec=psk" along with a second mount option that
specifies the name of a file containing the key.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/client.c     |   10 +++++++++-
 fs/nfs/fs_context.c |   40 ++++++++++++++++++++++++++++++++++++++++
 fs/nfs/internal.h   |    1 +
 fs/nfs/nfs4client.c |    2 +-
 fs/nfs/super.c      |    7 +++++++
 5 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 0896e4f047d1..1f26c1ad18b3 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -530,6 +530,14 @@ int nfs_create_rpc_client(struct nfs_client *clp,
 	if (test_bit(NFS_CS_REUSEPORT, &clp->cl_flags))
 		args.flags |= RPC_CLNT_CREATE_REUSEPORT;
 
+	switch (clp->cl_xprtsec) {
+	case NFS_CS_XPRTSEC_TLS:
+		args.xprtsec = RPC_XPRTSEC_TLS_X509;
+		break;
+	default:
+		args.xprtsec = RPC_XPRTSEC_NONE;
+	}
+
 	if (!IS_ERR(clp->cl_rpcclient))
 		return 0;
 
@@ -680,7 +688,7 @@ static int nfs_init_server(struct nfs_server *server,
 		.cred = server->cred,
 		.nconnect = ctx->nfs_server.nconnect,
 		.init_flags = (1UL << NFS_CS_REUSEPORT),
-		.xprtsec_policy = NFS_CS_XPRTSEC_NONE,
+		.xprtsec_policy = ctx->xprtsec_policy,
 	};
 	struct nfs_client *clp;
 	int error;
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 35e400a557b9..3e29dd88b59b 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -88,6 +88,7 @@ enum nfs_param {
 	Opt_vers,
 	Opt_wsize,
 	Opt_write,
+	Opt_xprtsec,
 };
 
 enum {
@@ -194,6 +195,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
 	fsparam_string("vers",		Opt_vers),
 	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
 	fsparam_u32   ("wsize",		Opt_wsize),
+	fsparam_string("xprtsec",	Opt_xprtsec),
 	{}
 };
 
@@ -267,6 +269,18 @@ static const struct constant_table nfs_secflavor_tokens[] = {
 	{}
 };
 
+enum {
+	Opt_xprtsec_none,
+	Opt_xprtsec_tls,
+	nr__Opt_xprtsec
+};
+
+static const struct constant_table nfs_xprtsec_policies[] = {
+	{ "none",	Opt_xprtsec_none },
+	{ "tls",	Opt_xprtsec_tls },
+	{}
+};
+
 /*
  * Sanity-check a server address provided by the mount command.
  *
@@ -431,6 +445,26 @@ static int nfs_parse_security_flavors(struct fs_context *fc,
 	return 0;
 }
 
+static int nfs_parse_xprtsec_policy(struct fs_context *fc,
+				    struct fs_parameter *param)
+{
+	struct nfs_fs_context *ctx = nfs_fc2context(fc);
+
+	trace_nfs_mount_assign(param->key, param->string);
+
+	switch (lookup_constant(nfs_xprtsec_policies, param->string, -1)) {
+	case Opt_xprtsec_none:
+		ctx->xprtsec_policy = NFS_CS_XPRTSEC_NONE;
+		break;
+	case Opt_xprtsec_tls:
+		ctx->xprtsec_policy = NFS_CS_XPRTSEC_TLS;
+		break;
+	default:
+		return nfs_invalf(fc, "NFS: Unrecognized transport security policy");
+	}
+	return 0;
+}
+
 static int nfs_parse_version_string(struct fs_context *fc,
 				    const char *string)
 {
@@ -695,6 +729,11 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		if (ret < 0)
 			return ret;
 		break;
+	case Opt_xprtsec:
+		ret = nfs_parse_xprtsec_policy(fc, param);
+		if (ret < 0)
+			return ret;
+		break;
 
 	case Opt_proto:
 		trace_nfs_mount_assign(param->key, param->string);
@@ -1564,6 +1603,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
 		ctx->selected_flavor	= RPC_AUTH_MAXFLAVOR;
 		ctx->minorversion	= 0;
 		ctx->need_mount		= true;
+		ctx->xprtsec_policy	= NFS_CS_XPRTSEC_NONE;
 
 		fc->s_iflags		|= SB_I_STABLE_WRITES;
 	}
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 0a3512c39376..bc60a556ad92 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -102,6 +102,7 @@ struct nfs_fs_context {
 	unsigned int		bsize;
 	struct nfs_auth_info	auth_info;
 	rpc_authflavor_t	selected_flavor;
+	unsigned int		xprtsec_policy;
 	char			*client_address;
 	unsigned int		version;
 	unsigned int		minorversion;
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 682d47e5977b..8dbdb00859fe 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1159,7 +1159,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
 				ctx->nfs_server.nconnect,
 				ctx->nfs_server.max_connect,
 				fc->net_ns,
-				NFS_CS_XPRTSEC_NONE);
+				ctx->xprtsec_policy);
 	if (error < 0)
 		return error;
 
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 6ab5eeb000dc..66da994500f9 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -491,6 +491,13 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
 	seq_printf(m, ",timeo=%lu", 10U * nfss->client->cl_timeout->to_initval / HZ);
 	seq_printf(m, ",retrans=%u", nfss->client->cl_timeout->to_retries);
 	seq_printf(m, ",sec=%s", nfs_pseudoflavour_to_name(nfss->client->cl_auth->au_flavor));
+	switch (clp->cl_xprtsec) {
+	case NFS_CS_XPRTSEC_TLS:
+		seq_printf(m, ",xprtsec=tls");
+		break;
+	default:
+		break;
+	}
 
 	if (version != 4)
 		nfs_show_mountd_options(m, nfss, showdefaults);

Re: [PATCH v2 01/15] SUNRPC: Fail faster on bad verifier

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> A bad verifier is not a garbage argument, it's an authentication
> failure. Retrying it doesn't make the problem go away, and delays
> upper layer recovery steps.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/clnt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index e2c6eca0271b..ed13d55df720 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2649,7 +2649,7 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
>  
>  out_verifier:
>  	trace_rpc_bad_verifier(task);
> -	goto out_garbage;
> +	goto out_err;
>  
>  out_msg_denied:
>  	error = -EACCES;
> 
> 

Looks like this will mostly manifest as an -EIO return, which is what we
want in most cases for a munged verifer.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 02/15] SUNRPC: Widen rpc_task::tk_flags

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> There is just one unused bit left in rpc_task::tk_flags, and I will
> need two in subsequent patches. Double the width of the field to
> accommodate more flag bits.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/clnt.h  |    6 ++++--
>  include/linux/sunrpc/sched.h |   32 ++++++++++++++++----------------
>  net/sunrpc/clnt.c            |   11 ++++++-----
>  net/sunrpc/debugfs.c         |    2 +-
>  4 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 90501404fa49..cbdd20dc84b7 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -193,11 +193,13 @@ void rpc_prepare_reply_pages(struct rpc_rqst *req, struct page **pages,
>  			     unsigned int hdrsize);
>  void		rpc_call_start(struct rpc_task *);
>  int		rpc_call_async(struct rpc_clnt *clnt,
> -			       const struct rpc_message *msg, int flags,
> +			       const struct rpc_message *msg,
> +			       unsigned int flags,
>  			       const struct rpc_call_ops *tk_ops,
>  			       void *calldata);
>  int		rpc_call_sync(struct rpc_clnt *clnt,
> -			      const struct rpc_message *msg, int flags);
> +			      const struct rpc_message *msg,
> +			      unsigned int flags);
>  struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred,
>  			       int flags);
>  int		rpc_restart_call_prepare(struct rpc_task *);
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> index 1d7a3e51b795..d4b7ebd0a99c 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -82,7 +82,7 @@ struct rpc_task {
>  	ktime_t			tk_start;	/* RPC task init timestamp */
>  
>  	pid_t			tk_owner;	/* Process id for batching tasks */
> -	unsigned short		tk_flags;	/* misc flags */
> +	unsigned int		tk_flags;	/* misc flags */
>  	unsigned short		tk_timeouts;	/* maj timeouts */
>  
>  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) || IS_ENABLED(CONFIG_TRACEPOINTS)
> @@ -112,27 +112,27 @@ struct rpc_task_setup {
>  	const struct rpc_call_ops *callback_ops;
>  	void *callback_data;
>  	struct workqueue_struct *workqueue;
> -	unsigned short flags;
> +	unsigned int flags;
>  	signed char priority;
>  };
>  
>  /*
>   * RPC task flags
>   */
> -#define RPC_TASK_ASYNC		0x0001		/* is an async task */
> -#define RPC_TASK_SWAPPER	0x0002		/* is swapping in/out */
> -#define RPC_TASK_MOVEABLE	0x0004		/* nfs4.1+ rpc tasks */
> -#define RPC_TASK_NULLCREDS	0x0010		/* Use AUTH_NULL credential */
> -#define RPC_CALL_MAJORSEEN	0x0020		/* major timeout seen */
> -#define RPC_TASK_DYNAMIC	0x0080		/* task was kmalloc'ed */
> -#define	RPC_TASK_NO_ROUND_ROBIN	0x0100		/* send requests on "main" xprt */
> -#define RPC_TASK_SOFT		0x0200		/* Use soft timeouts */
> -#define RPC_TASK_SOFTCONN	0x0400		/* Fail if can't connect */
> -#define RPC_TASK_SENT		0x0800		/* message was sent */
> -#define RPC_TASK_TIMEOUT	0x1000		/* fail with ETIMEDOUT on timeout */
> -#define RPC_TASK_NOCONNECT	0x2000		/* return ENOTCONN if not connected */
> -#define RPC_TASK_NO_RETRANS_TIMEOUT	0x4000		/* wait forever for a reply */
> -#define RPC_TASK_CRED_NOREF	0x8000		/* No refcount on the credential */
> +#define RPC_TASK_ASYNC			0x00000001	/* is an async task */
> +#define RPC_TASK_SWAPPER		0x00000002	/* is swapping in/out */
> +#define RPC_TASK_MOVEABLE		0x00000004	/* nfs4.1+ rpc tasks */
> +#define RPC_TASK_NULLCREDS		0x00000010	/* Use AUTH_NULL credential */
> +#define RPC_CALL_MAJORSEEN		0x00000020	/* major timeout seen */
> +#define RPC_TASK_DYNAMIC		0x00000080	/* task was kmalloc'ed */
> +#define	RPC_TASK_NO_ROUND_ROBIN		0x00000100	/* send requests on "main" xprt */
> +#define RPC_TASK_SOFT			0x00000200	/* Use soft timeouts */
> +#define RPC_TASK_SOFTCONN		0x00000400	/* Fail if can't connect */
> +#define RPC_TASK_SENT			0x00000800	/* message was sent */
> +#define RPC_TASK_TIMEOUT		0x00001000	/* fail with ETIMEDOUT on timeout */
> +#define RPC_TASK_NOCONNECT		0x00002000	/* return ENOTCONN if not connected */
> +#define RPC_TASK_NO_RETRANS_TIMEOUT	0x00004000	/* wait forever for a reply */
> +#define RPC_TASK_CRED_NOREF		0x00008000	/* No refcount on the credential */
>  
>  #define RPC_IS_ASYNC(t)		((t)->tk_flags & RPC_TASK_ASYNC)
>  #define RPC_IS_SWAPPER(t)	((t)->tk_flags & RPC_TASK_SWAPPER)
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index ed13d55df720..8fd45de66882 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1161,7 +1161,8 @@ EXPORT_SYMBOL_GPL(rpc_run_task);
>   * @msg: RPC call parameters
>   * @flags: RPC call flags
>   */
> -int rpc_call_sync(struct rpc_clnt *clnt, const struct rpc_message *msg, int flags)
> +int rpc_call_sync(struct rpc_clnt *clnt, const struct rpc_message *msg,
> +		  unsigned int flags)
>  {
>  	struct rpc_task	*task;
>  	struct rpc_task_setup task_setup_data = {
> @@ -1196,9 +1197,9 @@ EXPORT_SYMBOL_GPL(rpc_call_sync);
>   * @tk_ops: RPC call ops
>   * @data: user call data
>   */
> -int
> -rpc_call_async(struct rpc_clnt *clnt, const struct rpc_message *msg, int flags,
> -	       const struct rpc_call_ops *tk_ops, void *data)
> +int rpc_call_async(struct rpc_clnt *clnt, const struct rpc_message *msg,
> +		   unsigned int flags, const struct rpc_call_ops *tk_ops,
> +		   void *data)
>  {
>  	struct rpc_task	*task;
>  	struct rpc_task_setup task_setup_data = {
> @@ -3079,7 +3080,7 @@ static void rpc_show_task(const struct rpc_clnt *clnt,
>  	if (RPC_IS_QUEUED(task))
>  		rpc_waitq = rpc_qname(task->tk_waitqueue);
>  
> -	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
> +	printk(KERN_INFO "%5u %08x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
>  		task->tk_pid, task->tk_flags, task->tk_status,
>  		clnt, task->tk_rqstp, rpc_task_timeout(task), task->tk_ops,
>  		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
> diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> index 7dc9cc929bfd..8b66235a3a49 100644
> --- a/net/sunrpc/debugfs.c
> +++ b/net/sunrpc/debugfs.c
> @@ -30,7 +30,7 @@ tasks_show(struct seq_file *f, void *v)
>  	if (task->tk_rqstp)
>  		xid = be32_to_cpu(task->tk_rqstp->rq_xid);
>  
> -	seq_printf(f, "%5u %04x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
> +	seq_printf(f, "%5u %08x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
>  		task->tk_pid, task->tk_flags, task->tk_status,
>  		clnt->cl_clid, xid, rpc_task_timeout(task), task->tk_ops,
>  		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 03/15] SUNRPC: Replace dprintk() call site in xs_data_ready

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/trace/events/sunrpc.h |   20 ++++++++++++++++++++
>  net/sunrpc/xprtsock.c         |    6 ++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index 3995c58a1c51..a66aa1f59ed8 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -1266,6 +1266,26 @@ TRACE_EVENT(xprt_reserve,
>  	)
>  );
>  
> +TRACE_EVENT(xs_data_ready,
> +	TP_PROTO(
> +		const struct rpc_xprt *xprt
> +	),
> +
> +	TP_ARGS(xprt),
> +
> +	TP_STRUCT__entry(
> +		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
> +		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
> +		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
> +	),
> 
> 

This tracepoint is likely to fire rather often when it's enabled. Would
it be more efficient to store the addr and port as binary data instead?

> +
> +	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
> +);
> +
>  TRACE_EVENT(xs_stream_read_data,
>  	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
>  
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 650102a9c86a..73fab802996d 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1378,7 +1378,7 @@ static void xs_udp_data_receive_workfn(struct work_struct *work)
>  }
>  
>  /**
> - * xs_data_ready - "data ready" callback for UDP sockets
> + * xs_data_ready - "data ready" callback for sockets
>   * @sk: socket with data to read
>   *
>   */
> @@ -1386,11 +1386,13 @@ static void xs_data_ready(struct sock *sk)
>  {
>  	struct rpc_xprt *xprt;
>  
> -	dprintk("RPC:       xs_data_ready...\n");
>  	xprt = xprt_from_sock(sk);
>  	if (xprt != NULL) {
>  		struct sock_xprt *transport = container_of(xprt,
>  				struct sock_xprt, xprt);
> +
> +		trace_xs_data_ready(xprt);
> +
>  		transport->old_data_ready(sk);
>  		/* Any data means we had a useful conversation, so
>  		 * then we don't need to delay the next reconnect
> 
> 

That dprintk was pretty worthless anyway. So the change seems fine to
me.


-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 03/15] SUNRPC: Replace dprintk() call site in xs_data_ready

[Chuck Lever III]

> On Jul 6, 2022, at 1:19 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/trace/events/sunrpc.h | 20 ++++++++++++++++++++
>> net/sunrpc/xprtsock.c | 6 ++++--
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
>> index 3995c58a1c51..a66aa1f59ed8 100644
>> --- a/include/trace/events/sunrpc.h
>> +++ b/include/trace/events/sunrpc.h
>> @@ -1266,6 +1266,26 @@ TRACE_EVENT(xprt_reserve,
>> 	)
>> );
>> 
>> +TRACE_EVENT(xs_data_ready,
>> +	TP_PROTO(
>> +		const struct rpc_xprt *xprt
>> +	),
>> +
>> +	TP_ARGS(xprt),
>> +
>> +	TP_STRUCT__entry(
>> +		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
>> +		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
>> +		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
>> +	),
>> 
>> 
> 
> This tracepoint is likely to fire rather often when it's enabled. Would
> it be more efficient to store the addr and port as binary data instead?

Yes, and it can use get_sockaddr() and friends for that. But those
patches were in flight when I wrote this one.


>> +
>> +	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
>> +);
>> +
>> TRACE_EVENT(xs_stream_read_data,
>> 	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
>> 
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 650102a9c86a..73fab802996d 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -1378,7 +1378,7 @@ static void xs_udp_data_receive_workfn(struct work_struct *work)
>> }
>> 
>> /**
>> - * xs_data_ready - "data ready" callback for UDP sockets
>> + * xs_data_ready - "data ready" callback for sockets
>> * @sk: socket with data to read
>> *
>> */
>> @@ -1386,11 +1386,13 @@ static void xs_data_ready(struct sock *sk)
>> {
>> 	struct rpc_xprt *xprt;
>> 
>> -	dprintk("RPC: xs_data_ready...\n");
>> 	xprt = xprt_from_sock(sk);
>> 	if (xprt != NULL) {
>> 		struct sock_xprt *transport = container_of(xprt,
>> 				struct sock_xprt, xprt);
>> +
>> +		trace_xs_data_ready(xprt);
>> +
>> 		transport->old_data_ready(sk);
>> 		/* Any data means we had a useful conversation, so
>> 		 * then we don't need to delay the next reconnect
>> 
>> 
> 
> That dprintk was pretty worthless anyway. So the change seems fine to
> me.
> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH v2 04/15] NFS: Replace fs_context-related dprintk() call sites with tracepoints

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> Contributed as part of the long patch series that converts NFS from
> using dprintk to tracepoints for observability.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/fs_context.c |   25 ++++++++++-------
>  fs/nfs/nfstrace.h   |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 9a16897e8dc6..35e400a557b9 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -21,6 +21,8 @@
>  #include "nfs.h"
>  #include "internal.h"
>  
> +#include "nfstrace.h"
> +
>  #define NFSDBG_FACILITY		NFSDBG_MOUNT
>  
>  #if IS_ENABLED(CONFIG_NFS_V3)
> @@ -284,7 +286,7 @@ static int nfs_verify_server_address(struct sockaddr *addr)
>  	}
>  	}
>  
> -	dfprintk(MOUNT, "NFS: Invalid IP address specified\n");
> +	trace_nfs_mount_addr_err(addr);
>  	return 0;
>  }
>  
> @@ -378,7 +380,7 @@ static int nfs_parse_security_flavors(struct fs_context *fc,
>  	char *string = param->string, *p;
>  	int ret;
>  
> -	dfprintk(MOUNT, "NFS: parsing %s=%s option\n", param->key, param->string);
> +	trace_nfs_mount_assign(param->key, string);
>  
>  	while ((p = strsep(&string, ":")) != NULL) {
>  		if (!*p)
> @@ -480,7 +482,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  	unsigned int len;
>  	int ret, opt;
>  
> -	dfprintk(MOUNT, "NFS:   parsing nfs mount option '%s'\n", param->key);
> +	trace_nfs_mount_option(param);
>  
>  	opt = fs_parse(fc, nfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -683,6 +685,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  			return ret;
>  		break;
>  	case Opt_vers:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		ret = nfs_parse_version_string(fc, param->string);
>  		if (ret < 0)
>  			return ret;
> @@ -694,6 +697,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		break;
>  
>  	case Opt_proto:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		protofamily = AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
>  		case Opt_xprt_udp6:
> @@ -729,6 +733,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		break;
>  
>  	case Opt_mountproto:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		mountfamily = AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
>  		case Opt_xprt_udp6:
> @@ -751,6 +756,7 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		break;
>  
>  	case Opt_addr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		len = rpc_pton(fc->net_ns, param->string, param->size,
>  			       &ctx->nfs_server.address,
>  			       sizeof(ctx->nfs_server._address));
> @@ -759,16 +765,19 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		ctx->nfs_server.addrlen = len;
>  		break;
>  	case Opt_clientaddr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		kfree(ctx->client_address);
>  		ctx->client_address = param->string;
>  		param->string = NULL;
>  		break;
>  	case Opt_mounthost:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		kfree(ctx->mount_server.hostname);
>  		ctx->mount_server.hostname = param->string;
>  		param->string = NULL;
>  		break;
>  	case Opt_mountaddr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		len = rpc_pton(fc->net_ns, param->string, param->size,
>  			       &ctx->mount_server.address,
>  			       sizeof(ctx->mount_server._address));
> @@ -846,7 +855,6 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		 */
>  	case Opt_sloppy:
>  		ctx->sloppy = true;
> -		dfprintk(MOUNT, "NFS:   relaxing parsing rules\n");
>  		break;
>  	}
>  
> @@ -879,10 +887,8 @@ static int nfs_parse_source(struct fs_context *fc,
>  	size_t len;
>  	const char *end;
>  
> -	if (unlikely(!dev_name || !*dev_name)) {
> -		dfprintk(MOUNT, "NFS: device name not specified\n");
> +	if (unlikely(!dev_name || !*dev_name))
>  		return -EINVAL;
> -	}
>  
>  	/* Is the host name protected with square brakcets? */
>  	if (*dev_name == '[') {
> @@ -922,7 +928,7 @@ static int nfs_parse_source(struct fs_context *fc,
>  	if (!ctx->nfs_server.export_path)
>  		goto out_nomem;
>  
> -	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", ctx->nfs_server.export_path);
> +	trace_nfs_mount_path(ctx->nfs_server.export_path);
>  	return 0;
>  
>  out_bad_devname:
> @@ -1116,7 +1122,6 @@ static int nfs23_parse_monolithic(struct fs_context *fc,
>  	return nfs_invalf(fc, "NFS: nfs_mount_data version supports only AUTH_SYS");
>  
>  out_nomem:
> -	dfprintk(MOUNT, "NFS: not enough memory to handle mount options");
>  	return -ENOMEM;
>  
>  out_no_address:
> @@ -1248,7 +1253,7 @@ static int nfs4_parse_monolithic(struct fs_context *fc,
>  	if (IS_ERR(c))
>  		return PTR_ERR(c);
>  	ctx->nfs_server.export_path = c;
> -	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", c);
> +	trace_nfs_mount_path(c);
>  
>  	c = strndup_user(data->client_addr.data, 16);
>  	if (IS_ERR(c))
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index 012bd7339862..ccaeae42ee77 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -1609,6 +1609,83 @@ TRACE_EVENT(nfs_fh_to_dentry,
>  		)
>  );
>  
> +TRACE_EVENT(nfs_mount_addr_err,
> +	TP_PROTO(
> +		const struct sockaddr *sap
> +	),
> +
> +	TP_ARGS(sap),
> +
> +	TP_STRUCT__entry(
> +		__array(unsigned char, addr, sizeof(struct sockaddr_in6))
> +	),
> +
> +	TP_fast_assign(
> +		memcpy(__entry->addr, sap, sizeof(__entry->addr));
> +	),
> +
> +	TP_printk("addr=%pISpc", __entry->addr)
> +);
> +
> +TRACE_EVENT(nfs_mount_assign,
> +	TP_PROTO(
> +		const char *option,
> +		const char *value
> +	),
> +
> +	TP_ARGS(option, value),
> +
> +	TP_STRUCT__entry(
> +		__string(option, option)
> +		__string(value, value)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(option, option);
> +		__assign_str(value, value);
> +	),
> +
> +	TP_printk("option %s=%s",
> +		__get_str(option), __get_str(value)
> +	)
> +);
> +
> +TRACE_EVENT(nfs_mount_option,
> +	TP_PROTO(
> +		const struct fs_parameter *param
> +	),
> +
> +	TP_ARGS(param),
> +
> +	TP_STRUCT__entry(
> +		__string(option, param->key)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(option, param->key);
> +	),
> +
> +	TP_printk("option %s", __get_str(option))
> +);
> +
> +TRACE_EVENT(nfs_mount_path,
> +	TP_PROTO(
> +		const char *path
> +	),
> +
> +	TP_ARGS(path),
> +
> +	TP_STRUCT__entry(
> +		__string(path, path)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(path, path);
> +	),
> +
> +	TP_printk("path='%s'", __get_str(path))
> +);
> +
>  DECLARE_EVENT_CLASS(nfs_xdr_event,
>  		TP_PROTO(
>  			const struct xdr_stream *xdr,
> 
> 

Seems like an improvement overall. 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 06/15] SUNRPC: Trace the rpc_create_args

[Jeff Layton]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> Pass the upper layer's rpc_create_args to the rpc_clnt_new()
> tracepoint so additional parts of the upper layer's request can be
> recorded.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/trace/events/sunrpc.h |   53 +++++++++++++++++++++++++++++++++--------
>  net/sunrpc/clnt.c             |    2 +-
>  2 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index a66aa1f59ed8..986e135e314f 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -139,36 +139,69 @@ DEFINE_RPC_CLNT_EVENT(release);
>  DEFINE_RPC_CLNT_EVENT(replace_xprt);
>  DEFINE_RPC_CLNT_EVENT(replace_xprt_err);
>  
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_NONE);
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_X509);
> +TRACE_DEFINE_ENUM(RPC_XPRTSEC_TLS_PSK);
> +
> +#define rpc_show_xprtsec_policy(policy)					\
> +	__print_symbolic(policy,					\
> +		{ RPC_XPRTSEC_NONE,		"none" },		\
> +		{ RPC_XPRTSEC_TLS_X509,		"tls-x509" },		\
> +		{ RPC_XPRTSEC_TLS_PSK,		"tls-psk" })
> +
> +#define rpc_show_create_flags(flags)					\
> +	__print_flags(flags, "|",					\
> +		{ RPC_CLNT_CREATE_HARDRTRY,	"HARDRTRY" },		\
> +		{ RPC_CLNT_CREATE_AUTOBIND,	"AUTOBIND" },		\
> +		{ RPC_CLNT_CREATE_NONPRIVPORT,	"NONPRIVPORT" },	\
> +		{ RPC_CLNT_CREATE_NOPING,	"NOPING" },		\
> +		{ RPC_CLNT_CREATE_DISCRTRY,	"DISCRTRY" },		\
> +		{ RPC_CLNT_CREATE_QUIET,	"QUIET" },		\
> +		{ RPC_CLNT_CREATE_INFINITE_SLOTS,			\
> +						"INFINITE_SLOTS" },	\
> +		{ RPC_CLNT_CREATE_NO_IDLE_TIMEOUT,			\
> +						"NO_IDLE_TIMEOUT" },	\
> +		{ RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT,			\
> +						"NO_RETRANS_TIMEOUT" },	\
> +		{ RPC_CLNT_CREATE_SOFTERR,	"SOFTERR" },		\
> +		{ RPC_CLNT_CREATE_REUSEPORT,	"REUSEPORT" })
> +
>  TRACE_EVENT(rpc_clnt_new,
>  	TP_PROTO(
>  		const struct rpc_clnt *clnt,
>  		const struct rpc_xprt *xprt,
> -		const char *program,
> -		const char *server
> +		const struct rpc_create_args *args
>  	),
>  
> -	TP_ARGS(clnt, xprt, program, server),
> +	TP_ARGS(clnt, xprt, args),
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned int, client_id)
> +		__field(unsigned long, xprtsec)
> +		__field(unsigned long, flags)
> +		__string(program, clnt->cl_program->name)
> +		__string(server, xprt->servername)
>  		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
>  		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
> -		__string(program, program)
> -		__string(server, server)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->client_id = clnt->cl_clid;
> +		__entry->xprtsec = args->xprtsec;
> +		__entry->flags = args->flags;
> +		__assign_str(program, clnt->cl_program->name);
> +		__assign_str(server, xprt->servername);
>  		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
>  		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
> -		__assign_str(program, program);
> -		__assign_str(server, server);
>  	),
>  
> -	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER
> -		  " peer=[%s]:%s program=%s server=%s",
> +	TP_printk("client=" SUNRPC_TRACE_CLID_SPECIFIER " peer=[%s]:%s"
> +		" program=%s server=%s xprtsec=%s flags=%s",
>  		__entry->client_id, __get_str(addr), __get_str(port),
> -		__get_str(program), __get_str(server))
> +		__get_str(program), __get_str(server),
> +		rpc_show_xprtsec_policy(__entry->xprtsec),
> +		rpc_show_create_flags(__entry->flags)
> +	)
>  );
>  
>  TRACE_EVENT(rpc_clnt_new_err,
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 6dcc88d45f5d..0ca86c92968f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -435,7 +435,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
>  	if (parent)
>  		refcount_inc(&parent->cl_count);
>  
> -	trace_rpc_clnt_new(clnt, xprt, program->name, args->servername);
> +	trace_rpc_clnt_new(clnt, xprt, args);
>  	return clnt;
>  
>  out_no_path:
> 
> 

All of these tracing patches seem like a reasonable thing to add on
their own, IMO. It might be good to move them and some of the bugfixes
to a separate series and get those merged ahead of the parts that add
TLS support (which are more controversial).

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 06/15] SUNRPC: Trace the rpc_create_args

[Chuck Lever III]

> On Jul 6, 2022, at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> All of these tracing patches seem like a reasonable thing to add on
> their own, IMO. It might be good to move them and some of the bugfixes
> to a separate series and get those merged ahead of the parts that add
> TLS support (which are more controversial).

Well, that's why I moved those to the front of this series. In the
past, Trond hasn't been shy about extracting patches from series
to apply sooner rather than later.

But, yes, if Anna and Trond say they'd like a separate series for
this, I'm happy to oblige.


--
Chuck Lever

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> Now that the initial v5.19 merge window has closed, it's time for
> another round of review for RPC-with-TLS support in the Linux NFS
> client. This is just the RPC-specific portions. The full series is
> available in the "topic-rpc-with-tls-upcall" branch here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> I've taken two or three steps towards implementing the architecture
> Trond requested during the last review. There is now a two-stage
> connection establishment process so that the upper level can use
> XPRT_CONNECTED to determine when a TLS session is ready to use.
> There are probably additional changes and simplifications that can
> be made. Please review and provide feedback.
> 
> I wanted to make more progress on client-side authentication (ie,
> passing an x.509 cert from the client to the server) but NFSD bugs
> have taken all my time for the past few weeks.
> 
> 
> Changes since v1:
> - Rebased on v5.18
> - Re-ordered so generic fixes come first
> - Addressed some of Trond's review comments
> 
> ---
> 
> Chuck Lever (15):
>       SUNRPC: Fail faster on bad verifier
>       SUNRPC: Widen rpc_task::tk_flags
>       SUNRPC: Replace dprintk() call site in xs_data_ready
>       NFS: Replace fs_context-related dprintk() call sites with tracepoints
>       SUNRPC: Plumb an API for setting transport layer security
>       SUNRPC: Trace the rpc_create_args
>       SUNRPC: Refactor rpc_call_null_helper()
>       SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
>       SUNRPC: Ignore data_ready callbacks during TLS handshakes
>       SUNRPC: Capture cmsg metadata on client-side receive
>       SUNRPC: Add a connect worker function for TLS
>       SUNRPC: Add RPC-with-TLS support to xprtsock.c
>       SUNRPC: Add RPC-with-TLS tracepoints
>       NFS: Have struct nfs_client carry a TLS policy field
>       NFS: Add an "xprtsec=" NFS mount option
> 
> 
>  fs/nfs/client.c                 |  14 ++
>  fs/nfs/fs_context.c             |  65 +++++--
>  fs/nfs/internal.h               |   2 +
>  fs/nfs/nfs3client.c             |   1 +
>  fs/nfs/nfs4client.c             |  16 +-
>  fs/nfs/nfstrace.h               |  77 ++++++++
>  fs/nfs/super.c                  |   7 +
>  include/linux/nfs_fs_sb.h       |   5 +-
>  include/linux/sunrpc/auth.h     |   1 +
>  include/linux/sunrpc/clnt.h     |  15 +-
>  include/linux/sunrpc/sched.h    |  32 ++--
>  include/linux/sunrpc/xprt.h     |   2 +
>  include/linux/sunrpc/xprtsock.h |   4 +
>  include/net/tls.h               |   2 +
>  include/trace/events/sunrpc.h   | 157 ++++++++++++++--
>  net/sunrpc/Makefile             |   2 +-
>  net/sunrpc/auth.c               |   2 +-
>  net/sunrpc/auth_tls.c           | 120 +++++++++++++
>  net/sunrpc/clnt.c               |  34 ++--
>  net/sunrpc/debugfs.c            |   2 +-
>  net/sunrpc/xprtsock.c           | 310 +++++++++++++++++++++++++++++++-
>  21 files changed, 805 insertions(+), 65 deletions(-)
>  create mode 100644 net/sunrpc/auth_tls.c
> 
> --
> Chuck Lever
> 

Chuck,

How have you been testing this series? It looks like nfsd support is not
fully in yet, so I was wondering if you had a 3rd party server. I'd like
to do a little testing with this, and was wondering what I needed to
cobble together a test rig.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Chuck Lever III]

> On Jul 12, 2022, at 8:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
>> Now that the initial v5.19 merge window has closed, it's time for
>> another round of review for RPC-with-TLS support in the Linux NFS
>> client. This is just the RPC-specific portions. The full series is
>> available in the "topic-rpc-with-tls-upcall" branch here:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>> 
>> I've taken two or three steps towards implementing the architecture
>> Trond requested during the last review. There is now a two-stage
>> connection establishment process so that the upper level can use
>> XPRT_CONNECTED to determine when a TLS session is ready to use.
>> There are probably additional changes and simplifications that can
>> be made. Please review and provide feedback.
>> 
>> I wanted to make more progress on client-side authentication (ie,
>> passing an x.509 cert from the client to the server) but NFSD bugs
>> have taken all my time for the past few weeks.
>> 
>> 
>> Changes since v1:
>> - Rebased on v5.18
>> - Re-ordered so generic fixes come first
>> - Addressed some of Trond's review comments
>> 
>> ---
>> 
>> Chuck Lever (15):
>>      SUNRPC: Fail faster on bad verifier
>>      SUNRPC: Widen rpc_task::tk_flags
>>      SUNRPC: Replace dprintk() call site in xs_data_ready
>>      NFS: Replace fs_context-related dprintk() call sites with tracepoints
>>      SUNRPC: Plumb an API for setting transport layer security
>>      SUNRPC: Trace the rpc_create_args
>>      SUNRPC: Refactor rpc_call_null_helper()
>>      SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
>>      SUNRPC: Ignore data_ready callbacks during TLS handshakes
>>      SUNRPC: Capture cmsg metadata on client-side receive
>>      SUNRPC: Add a connect worker function for TLS
>>      SUNRPC: Add RPC-with-TLS support to xprtsock.c
>>      SUNRPC: Add RPC-with-TLS tracepoints
>>      NFS: Have struct nfs_client carry a TLS policy field
>>      NFS: Add an "xprtsec=" NFS mount option
>> 
>> 
>> fs/nfs/client.c                 |  14 ++
>> fs/nfs/fs_context.c             |  65 +++++--
>> fs/nfs/internal.h               |   2 +
>> fs/nfs/nfs3client.c             |   1 +
>> fs/nfs/nfs4client.c             |  16 +-
>> fs/nfs/nfstrace.h               |  77 ++++++++
>> fs/nfs/super.c                  |   7 +
>> include/linux/nfs_fs_sb.h       |   5 +-
>> include/linux/sunrpc/auth.h     |   1 +
>> include/linux/sunrpc/clnt.h     |  15 +-
>> include/linux/sunrpc/sched.h    |  32 ++--
>> include/linux/sunrpc/xprt.h     |   2 +
>> include/linux/sunrpc/xprtsock.h |   4 +
>> include/net/tls.h               |   2 +
>> include/trace/events/sunrpc.h   | 157 ++++++++++++++--
>> net/sunrpc/Makefile             |   2 +-
>> net/sunrpc/auth.c               |   2 +-
>> net/sunrpc/auth_tls.c           | 120 +++++++++++++
>> net/sunrpc/clnt.c               |  34 ++--
>> net/sunrpc/debugfs.c            |   2 +-
>> net/sunrpc/xprtsock.c           | 310 +++++++++++++++++++++++++++++++-
>> 21 files changed, 805 insertions(+), 65 deletions(-)
>> create mode 100644 net/sunrpc/auth_tls.c
>> 
>> --
>> Chuck Lever
>> 
> 
> Chuck,
> 
> How have you been testing this series? It looks like nfsd support is not
> fully in yet, so I was wondering if you had a 3rd party server. I'd like
> to do a little testing with this, and was wondering what I needed to
> cobble together a test rig.

Ben Coddington has an ngnix module to support RPC-with-TLS that can
front-end a stock Linux NFSD. Rick has a FreeBSD server implementation
of RPC-with-TLS. Rick's probably taken his server down, but Ben's
server is still up on the bake-a-thon VPN.


--
Chuck Lever

Re: [PATCH v2 12/15] SUNRPC: Add RPC-with-TLS support to xprtsock.c

[Benjamin Coddington]

On 6 Jun 2022, at 10:51, Chuck Lever wrote:

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/xprtsock.h |    1
>  net/sunrpc/xprtsock.c           |  243 
> ++++++++++++++++++++++++++++++++-------
>  2 files changed, 201 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/sunrpc/xprtsock.h 
> b/include/linux/sunrpc/xprtsock.h
> index e0b6009f1f69..eaf3d705f758 100644
> --- a/include/linux/sunrpc/xprtsock.h
> +++ b/include/linux/sunrpc/xprtsock.h
> @@ -57,6 +57,7 @@ struct sock_xprt {
>  	struct work_struct	error_worker;
>  	struct work_struct	recv_worker;
>  	struct mutex		recv_mutex;
> +	struct completion	handshake_done;
>  	struct sockaddr_storage	srcaddr;
>  	unsigned short		srcport;
>  	int			xprt_err;
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index a4fee00412d4..63fe97ede573 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -48,6 +48,7 @@
>  #include <net/udp.h>
>  #include <net/tcp.h>
>  #include <net/tls.h>
> +#include <net/tlsh.h>

Ah, maybe helpful to others to note this depends on the RFC "net/tls: 
Add support for PF_TLSH":

https://lore.kernel.org/linux-nfs/165030059051.5073.16723746870370826608.stgit@oracle-102.nfsv4.dev/

.. which (of course) exists in the topic branch in the cover-letter.

Ben

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Rick Macklem]

As I already posted to Jeff, I can put the server up for
a day or two at any time anyone would like to test
against it.

It now does TLS1.3 and I'll note the one thing the
server did that caught the FreeBSD client "off guard"
was it sends a couple of post handshake handshake
records. (The FreeBSD client now just tosses these away.)

Just email if/when you'd like to test, rick

________________________________________
From: Chuck Lever III <chuck.lever@oracle.com>
Sent: Tuesday, July 12, 2022 9:48 AM
To: Jeff Layton
Cc: Linux NFS Mailing List; trondmy@hammerspace.com
Subject: Re: [PATCH v2 00/15] RPC-with-TLS client side

CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca



> On Jul 12, 2022, at 8:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
>> Now that the initial v5.19 merge window has closed, it's time for
>> another round of review for RPC-with-TLS support in the Linux NFS
>> client. This is just the RPC-specific portions. The full series is
>> available in the "topic-rpc-with-tls-upcall" branch here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>>
>> I've taken two or three steps towards implementing the architecture
>> Trond requested during the last review. There is now a two-stage
>> connection establishment process so that the upper level can use
>> XPRT_CONNECTED to determine when a TLS session is ready to use.
>> There are probably additional changes and simplifications that can
>> be made. Please review and provide feedback.
>>
>> I wanted to make more progress on client-side authentication (ie,
>> passing an x.509 cert from the client to the server) but NFSD bugs
>> have taken all my time for the past few weeks.
>>
>>
>> Changes since v1:
>> - Rebased on v5.18
>> - Re-ordered so generic fixes come first
>> - Addressed some of Trond's review comments
>>
>> ---
>>
>> Chuck Lever (15):
>>      SUNRPC: Fail faster on bad verifier
>>      SUNRPC: Widen rpc_task::tk_flags
>>      SUNRPC: Replace dprintk() call site in xs_data_ready
>>      NFS: Replace fs_context-related dprintk() call sites with tracepoints
>>      SUNRPC: Plumb an API for setting transport layer security
>>      SUNRPC: Trace the rpc_create_args
>>      SUNRPC: Refactor rpc_call_null_helper()
>>      SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
>>      SUNRPC: Ignore data_ready callbacks during TLS handshakes
>>      SUNRPC: Capture cmsg metadata on client-side receive
>>      SUNRPC: Add a connect worker function for TLS
>>      SUNRPC: Add RPC-with-TLS support to xprtsock.c
>>      SUNRPC: Add RPC-with-TLS tracepoints
>>      NFS: Have struct nfs_client carry a TLS policy field
>>      NFS: Add an "xprtsec=" NFS mount option
>>
>>
>> fs/nfs/client.c                 |  14 ++
>> fs/nfs/fs_context.c             |  65 +++++--
>> fs/nfs/internal.h               |   2 +
>> fs/nfs/nfs3client.c             |   1 +
>> fs/nfs/nfs4client.c             |  16 +-
>> fs/nfs/nfstrace.h               |  77 ++++++++
>> fs/nfs/super.c                  |   7 +
>> include/linux/nfs_fs_sb.h       |   5 +-
>> include/linux/sunrpc/auth.h     |   1 +
>> include/linux/sunrpc/clnt.h     |  15 +-
>> include/linux/sunrpc/sched.h    |  32 ++--
>> include/linux/sunrpc/xprt.h     |   2 +
>> include/linux/sunrpc/xprtsock.h |   4 +
>> include/net/tls.h               |   2 +
>> include/trace/events/sunrpc.h   | 157 ++++++++++++++--
>> net/sunrpc/Makefile             |   2 +-
>> net/sunrpc/auth.c               |   2 +-
>> net/sunrpc/auth_tls.c           | 120 +++++++++++++
>> net/sunrpc/clnt.c               |  34 ++--
>> net/sunrpc/debugfs.c            |   2 +-
>> net/sunrpc/xprtsock.c           | 310 +++++++++++++++++++++++++++++++-
>> 21 files changed, 805 insertions(+), 65 deletions(-)
>> create mode 100644 net/sunrpc/auth_tls.c
>>
>> --
>> Chuck Lever
>>
>
> Chuck,
>
> How have you been testing this series? It looks like nfsd support is not
> fully in yet, so I was wondering if you had a 3rd party server. I'd like
> to do a little testing with this, and was wondering what I needed to
> cobble together a test rig.

Ben Coddington has an ngnix module to support RPC-with-TLS that can
front-end a stock Linux NFSD. Rick has a FreeBSD server implementation
of RPC-with-TLS. Rick's probably taken his server down, but Ben's
server is still up on the bake-a-thon VPN.


--
Chuck Lever

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Benjamin Coddington]

On 12 Jul 2022, at 20:51, Rick Macklem wrote:

> As I already posted to Jeff, I can put the server up for
> a day or two at any time anyone would like to test
> against it.
>
> It now does TLS1.3 and I'll note the one thing the
> server did that caught the FreeBSD client "off guard"
> was it sends a couple of post handshake handshake
> records. (The FreeBSD client now just tosses these away.)
>
> Just email if/when you'd like to test, rick

Hey Chuck, is the bakeathon root or intermediate certificate published
somewhere so we can add them to our trust stores?

Ben

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Chuck Lever III]

> On Jul 13, 2022, at 9:22 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 12 Jul 2022, at 20:51, Rick Macklem wrote:
> 
>> As I already posted to Jeff, I can put the server up for
>> a day or two at any time anyone would like to test
>> against it.
>> 
>> It now does TLS1.3 and I'll note the one thing the
>> server did that caught the FreeBSD client "off guard"
>> was it sends a couple of post handshake handshake
>> records. (The FreeBSD client now just tosses these away.)
>> 
>> Just email if/when you'd like to test, rick
> 
> Hey Chuck, is the bakeathon root or intermediate certificate published
> somewhere so we can add them to our trust stores?

oracle-102:/export has the bundle and instructions, some of them in English! :-D

--
Chuck Lever

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Benjamin Coddington]

On 12 Jul 2022, at 9:48, Chuck Lever III wrote:

>> On Jul 12, 2022, at 8:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>
>> On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
>>> Now that the initial v5.19 merge window has closed, it's time for
>>> another round of review for RPC-with-TLS support in the Linux NFS
>>> client. This is just the RPC-specific portions. The full series is
>>> available in the "topic-rpc-with-tls-upcall" branch here:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
>>>
>>> I've taken two or three steps towards implementing the architecture
>>> Trond requested during the last review. There is now a two-stage
>>> connection establishment process so that the upper level can use
>>> XPRT_CONNECTED to determine when a TLS session is ready to use.
>>> There are probably additional changes and simplifications that can
>>> be made. Please review and provide feedback.
>>>
>>> I wanted to make more progress on client-side authentication (ie,
>>> passing an x.509 cert from the client to the server) but NFSD bugs
>>> have taken all my time for the past few weeks.
>>>
>>>
>>> Changes since v1:
>>> - Rebased on v5.18
>>> - Re-ordered so generic fixes come first
>>> - Addressed some of Trond's review comments
>>>
>>> ---
>>>
>>> Chuck Lever (15):
>>>      SUNRPC: Fail faster on bad verifier
>>>      SUNRPC: Widen rpc_task::tk_flags
>>>      SUNRPC: Replace dprintk() call site in xs_data_ready
>>>      NFS: Replace fs_context-related dprintk() call sites with 
>>> tracepoints
>>>      SUNRPC: Plumb an API for setting transport layer security
>>>      SUNRPC: Trace the rpc_create_args
>>>      SUNRPC: Refactor rpc_call_null_helper()
>>>      SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
>>>      SUNRPC: Ignore data_ready callbacks during TLS handshakes
>>>      SUNRPC: Capture cmsg metadata on client-side receive
>>>      SUNRPC: Add a connect worker function for TLS
>>>      SUNRPC: Add RPC-with-TLS support to xprtsock.c
>>>      SUNRPC: Add RPC-with-TLS tracepoints
>>>      NFS: Have struct nfs_client carry a TLS policy field
>>>      NFS: Add an "xprtsec=" NFS mount option
>>>
>>>
>>> fs/nfs/client.c                 |  14 ++
>>> fs/nfs/fs_context.c             |  65 +++++--
>>> fs/nfs/internal.h               |   2 +
>>> fs/nfs/nfs3client.c             |   1 +
>>> fs/nfs/nfs4client.c             |  16 +-
>>> fs/nfs/nfstrace.h               |  77 ++++++++
>>> fs/nfs/super.c                  |   7 +
>>> include/linux/nfs_fs_sb.h       |   5 +-
>>> include/linux/sunrpc/auth.h     |   1 +
>>> include/linux/sunrpc/clnt.h     |  15 +-
>>> include/linux/sunrpc/sched.h    |  32 ++--
>>> include/linux/sunrpc/xprt.h     |   2 +
>>> include/linux/sunrpc/xprtsock.h |   4 +
>>> include/net/tls.h               |   2 +
>>> include/trace/events/sunrpc.h   | 157 ++++++++++++++--
>>> net/sunrpc/Makefile             |   2 +-
>>> net/sunrpc/auth.c               |   2 +-
>>> net/sunrpc/auth_tls.c           | 120 +++++++++++++
>>> net/sunrpc/clnt.c               |  34 ++--
>>> net/sunrpc/debugfs.c            |   2 +-
>>> net/sunrpc/xprtsock.c           | 310 
>>> +++++++++++++++++++++++++++++++-
>>> 21 files changed, 805 insertions(+), 65 deletions(-)
>>> create mode 100644 net/sunrpc/auth_tls.c
>>>
>>> --
>>> Chuck Lever
>>>
>>
>> Chuck,
>>
>> How have you been testing this series? It looks like nfsd support is 
>> not
>> fully in yet, so I was wondering if you had a 3rd party server. I'd 
>> like
>> to do a little testing with this, and was wondering what I needed to
>> cobble together a test rig.
>
> Ben Coddington has an ngnix module to support RPC-with-TLS that can
> front-end a stock Linux NFSD. Rick has a FreeBSD server implementation
> of RPC-with-TLS. Rick's probably taken his server down, but Ben's
> server is still up on the bake-a-thon VPN.

That server now has a proper certificate for CN=boson.nfsv4.dev signed 
by the bakeathon CA (thanks Chuck).

I've also (finally) put the nginx module code up on github if anyone 
else wants to throw it in front of a server:
https://github.com/bcodding/nginx-rpc-tls

Ben

Re: [PATCH v2 07/15] SUNRPC: Refactor rpc_call_null_helper()

[Jeff Layton]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> I'm about to add a use case that does not want RPC_TASK_NULLCREDS.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/clnt.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 0ca86c92968f..ade83ee13bac 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2751,8 +2751,7 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt,
>  		.rpc_op_cred = cred,
>  		.callback_ops = ops ?: &rpc_null_ops,
>  		.callback_data = data,
> -		.flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN |
> -			 RPC_TASK_NULLCREDS,
> +		.flags = flags | RPC_TASK_SOFT | RPC_TASK_SOFTCONN,
>  	};
>  
>  	return rpc_run_task(&task_setup_data);
> @@ -2760,7 +2759,8 @@ struct rpc_task *rpc_call_null_helper(struct rpc_clnt *clnt,
>  
>  struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int flags)
>  {
> -	return rpc_call_null_helper(clnt, NULL, cred, flags, NULL, NULL);
> +	return rpc_call_null_helper(clnt, NULL, cred, flags | RPC_TASK_NULLCREDS,
> +				    NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(rpc_call_null);
>  
> @@ -2860,9 +2860,11 @@ int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>  		goto success;
>  	}
>  
> -	task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_ASYNC,
> -			&rpc_cb_add_xprt_call_ops, data);
> +	task = rpc_call_null_helper(clnt, xprt, NULL,
> +				    RPC_TASK_ASYNC | RPC_TASK_NULLCREDS,
> +				    &rpc_cb_add_xprt_call_ops, data);
>  	data->xps->xps_nunique_destaddr_xprts++;
> +
>  	rpc_put_task(task);
>  success:
>  	return 1;
> @@ -2903,7 +2905,8 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *clnt,
>  		goto out_err;
>  
>  	/* Test the connection */
> -	task = rpc_call_null_helper(clnt, xprt, NULL, 0, NULL, NULL);
> +	task = rpc_call_null_helper(clnt, xprt, NULL, RPC_TASK_NULLCREDS,
> +				    NULL, NULL);
>  	if (IS_ERR(task)) {
>  		status = PTR_ERR(task);
>  		goto out_err;
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 05/15] SUNRPC: Plumb an API for setting transport layer security

[Jeff Layton]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> Add an initial set of policies along with fields for upper layers to
> pass the requested policy down to the transport layer.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/clnt.h |    9 +++++++++
>  include/linux/sunrpc/xprt.h |    2 ++
>  net/sunrpc/clnt.c           |    4 ++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index cbdd20dc84b7..85c2f810d4bb 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -58,6 +58,7 @@ struct rpc_clnt {
>  				cl_noretranstimeo: 1,/* No retransmit timeouts */
>  				cl_autobind : 1,/* use getport() */
>  				cl_chatty   : 1;/* be verbose */
> +	unsigned int		cl_xprtsec;	/* transport security policy */
>  
>  	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
>  	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
> @@ -139,6 +140,7 @@ struct rpc_create_args {
>  	struct svc_xprt		*bc_xprt;	/* NFSv4.1 backchannel */
>  	const struct cred	*cred;
>  	unsigned int		max_connect;
> +	unsigned int		xprtsec;
>  };
>  
>  struct rpc_add_xprt_test {
> @@ -162,6 +164,13 @@ struct rpc_add_xprt_test {
>  #define RPC_CLNT_CREATE_REUSEPORT	(1UL << 11)
>  #define RPC_CLNT_CREATE_CONNECTED	(1UL << 12)
>  
> +/* RPC transport layer security policies */
> +enum {
> +	RPC_XPRTSEC_NONE = 0,
> +	RPC_XPRTSEC_TLS_X509,
> +	RPC_XPRTSEC_TLS_PSK,
> +};
> +
>  struct rpc_clnt *rpc_create(struct rpc_create_args *args);
>  struct rpc_clnt	*rpc_bind_new_program(struct rpc_clnt *,
>  				const struct rpc_program *, u32);
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index 522bbf937957..d091ad2b7340 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -228,6 +228,7 @@ struct rpc_xprt {
>  	 */
>  	unsigned long		bind_timeout,
>  				reestablish_timeout;
> +	unsigned int		xprtsec;
>  	unsigned int		connect_cookie;	/* A cookie that gets bumped
>  						   every time the transport
>  						   is reconnected */
> @@ -332,6 +333,7 @@ struct xprt_create {
>  	struct svc_xprt		*bc_xprt;	/* NFSv4.1 backchannel */
>  	struct rpc_xprt_switch	*bc_xps;
>  	unsigned int		flags;
> +	unsigned int		xprtsec;
>  };
>  
>  struct xprt_class {
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 8fd45de66882..6dcc88d45f5d 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -385,6 +385,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
>  	if (!clnt)
>  		goto out_err;
>  	clnt->cl_parent = parent ? : clnt;
> +	clnt->cl_xprtsec = args->xprtsec;
>  
>  	err = rpc_alloc_clid(clnt);
>  	if (err)
> @@ -532,6 +533,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
>  		.addrlen = args->addrsize,
>  		.servername = args->servername,
>  		.bc_xprt = args->bc_xprt,
> +		.xprtsec = args->xprtsec,
>  	};
>  	char servername[48];
>  	struct rpc_clnt *clnt;
> @@ -726,6 +728,7 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,
>  	struct rpc_clnt *parent;
>  	int err;
>  
> +	args->xprtsec = clnt->cl_xprtsec;
>  	xprt = xprt_create_transport(args);
>  	if (IS_ERR(xprt))
>  		return PTR_ERR(xprt);
> @@ -2973,6 +2976,7 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
>  
>  	if (!xprtargs->ident)
>  		xprtargs->ident = ident;
> +	xprtargs->xprtsec = clnt->cl_xprtsec;
>  	xprt = xprt_create_transport(xprtargs);
>  	if (IS_ERR(xprt)) {
>  		ret = PTR_ERR(xprt);
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 10/15] SUNRPC: Capture cmsg metadata on client-side receive

[Jeff Layton]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> kTLS sockets use cmsg to report decryption errors and the need
> for session re-keying. An "application data" message contains a ULP
> payload, and that is passed along to the RPC client. An "alert"
> message triggers connection reset. Everything else is discarded.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/net/tls.h             |    2 ++
>  include/trace/events/sunrpc.h |   40 +++++++++++++++++++++++++++++++++
>  net/sunrpc/xprtsock.c         |   49 +++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 6b1bf46daa34..54bccb2e4014 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -71,6 +71,8 @@ static inline struct tlsh_sock *tlsh_sk(struct sock *sk)
>  
>  #define TLS_CRYPTO_INFO_READY(info)	((info)->cipher_type)
>  
> +#define TLS_RECORD_TYPE_ALERT		0x15
> +#define TLS_RECORD_TYPE_HANDSHAKE	0x16
>  #define TLS_RECORD_TYPE_DATA		0x17
>  
>  #define TLS_AAD_SPACE_SIZE		13
> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
> index 986e135e314f..d7d07f3b850e 100644
> --- a/include/trace/events/sunrpc.h
> +++ b/include/trace/events/sunrpc.h
> @@ -1319,6 +1319,46 @@ TRACE_EVENT(xs_data_ready,
>  	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
>  );
>  
> +/*
> + * From https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml
> + *
> + * Captured March 2022. Other values are unassigned or reserved.
> + */
> +#define rpc_show_tls_content_type(type) \
> +	__print_symbolic(type, \
> +		{ 20,		"change cipher spec" }, \
> +		{ 21,		"alert" }, \
> +		{ 22,		"handshake" }, \
> +		{ 23,		"application data" }, \
> +		{ 24,		"heartbeat" }, \
> +		{ 25,		"tls12_cid" }, \
> +		{ 26,		"ACK" })
> +
> +TRACE_EVENT(xs_tls_contenttype,
> +	TP_PROTO(
> +		const struct rpc_xprt *xprt,
> +		unsigned char ctype
> +	),
> +
> +	TP_ARGS(xprt, ctype),
> +
> +	TP_STRUCT__entry(
> +		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
> +		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
> +		__field(unsigned long, ctype)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
> +		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
> +		__entry->ctype = ctype;
> +	),
> +
> +	TP_printk("peer=[%s]:%s: %s", __get_str(addr), __get_str(port),
> +		rpc_show_tls_content_type(__entry->ctype)
> +	)
> +);
> +
>  TRACE_EVENT(xs_stream_read_data,
>  	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
>  
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 0a521aee0b2f..c73af8f1c3d4 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -47,6 +47,8 @@
>  #include <net/checksum.h>
>  #include <net/udp.h>
>  #include <net/tcp.h>
> +#include <net/tls.h>
> +
>  #include <linux/bvec.h>
>  #include <linux/highmem.h>
>  #include <linux/uio.h>
> @@ -350,13 +352,56 @@ xs_alloc_sparse_pages(struct xdr_buf *buf, size_t want, gfp_t gfp)
>  	return want;
>  }
>  
> +static int
> +xs_sock_process_cmsg(struct socket *sock, struct msghdr *msg,
> +		     struct cmsghdr *cmsg, int ret)
> +{
> +	if (cmsg->cmsg_level == SOL_TLS &&
> +	    cmsg->cmsg_type == TLS_GET_RECORD_TYPE) {
> +		u8 content_type = *((u8 *)CMSG_DATA(cmsg));
> +
> +		trace_xs_tls_contenttype(xprt_from_sock(sock->sk), content_type);
> +		switch (content_type) {
> +		case TLS_RECORD_TYPE_DATA:
> +			/* TLS sets EOR at the end of each application data
> +			 * record, even though there might be more frames
> +			 * waiting to be decrypted. */
> +			msg->msg_flags &= ~MSG_EOR;
> +			break;
> +		case TLS_RECORD_TYPE_ALERT:
> +			ret = -ENOTCONN;
> +			break;
> +		default:
> +			ret = -EAGAIN;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int
> +xs_sock_recv_cmsg(struct socket *sock, struct msghdr *msg, int flags)
> +{
> +	union {
> +		struct cmsghdr	cmsg;
> +		u8		buf[CMSG_SPACE(sizeof(u8))];
> +	} u;
> +	int ret;
> +
> +	msg->msg_control = &u;
> +	msg->msg_controllen = sizeof(u);
> +	ret = sock_recvmsg(sock, msg, flags);
> +	if (msg->msg_controllen != sizeof(u))

Do you also need to check for ret < 0 here? Can msg_controllen be
trusted if sock_recvmsg returns an error?

> +		ret = xs_sock_process_cmsg(sock, msg, &u.cmsg, ret);
> +	return ret;
> +}
> +
>  static ssize_t
>  xs_sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags, size_t seek)
>  {
>  	ssize_t ret;
>  	if (seek != 0)
>  		iov_iter_advance(&msg->msg_iter, seek);
> -	ret = sock_recvmsg(sock, msg, flags);
> +	ret = xs_sock_recv_cmsg(sock, msg, flags);
>  	return ret > 0 ? ret + seek : ret;
>  }
>  
> @@ -382,7 +427,7 @@ xs_read_discard(struct socket *sock, struct msghdr *msg, int flags,
>  		size_t count)
>  {
>  	iov_iter_discard(&msg->msg_iter, READ, count);
> -	return sock_recvmsg(sock, msg, flags);
> +	return xs_sock_recv_cmsg(sock, msg, flags);
>  }
>  
>  #if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 12/15] SUNRPC: Add RPC-with-TLS support to xprtsock.c

[Jeff Layton]

On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/xprtsock.h |    1 
>  net/sunrpc/xprtsock.c           |  243 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 201 insertions(+), 43 deletions(-)
> 
> diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
> index e0b6009f1f69..eaf3d705f758 100644
> --- a/include/linux/sunrpc/xprtsock.h
> +++ b/include/linux/sunrpc/xprtsock.h
> @@ -57,6 +57,7 @@ struct sock_xprt {
>  	struct work_struct	error_worker;
>  	struct work_struct	recv_worker;
>  	struct mutex		recv_mutex;
> +	struct completion	handshake_done;
>  	struct sockaddr_storage	srcaddr;
>  	unsigned short		srcport;
>  	int			xprt_err;
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index a4fee00412d4..63fe97ede573 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -48,6 +48,7 @@
>  #include <net/udp.h>
>  #include <net/tcp.h>
>  #include <net/tls.h>
> +#include <net/tlsh.h>
>  
>  #include <linux/bvec.h>
>  #include <linux/highmem.h>
> @@ -197,6 +198,11 @@ static struct ctl_table sunrpc_table[] = {
>   */
>  #define XS_IDLE_DISC_TO		(5U * 60 * HZ)
>  
> +/*
> + * TLS handshake timeout.
> + */
> +#define XS_TLS_HANDSHAKE_TO    (20U * HZ)
> +
>  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>  # undef  RPC_DEBUG_DATA
>  # define RPCDBG_FACILITY	RPCDBG_TRANS
> @@ -1254,6 +1260,8 @@ static void xs_reset_transport(struct sock_xprt *transport)
>  	if (atomic_read(&transport->xprt.swapper))
>  		sk_clear_memalloc(sk);
>  
> +	/* TODO: Maybe send a TLS Closure alert */
> +
>  	kernel_sock_shutdown(sock, SHUT_RDWR);
>  
>  	mutex_lock(&transport->recv_mutex);
> @@ -2424,6 +2432,147 @@ static void xs_tcp_setup_socket(struct work_struct *work)
>  
>  #if IS_ENABLED(CONFIG_TLS)
>  
> +/**
> + * xs_tls_handshake_done - TLS handshake completion handler
> + * @data: address of xprt to wake
> + * @status: status of handshake
> + *
> + */
> +static void xs_tls_handshake_done(void *data, int status)
> +{
> +	struct rpc_xprt *xprt = data;
> +	struct sock_xprt *transport =
> +				container_of(xprt, struct sock_xprt, xprt);
> +
> +	transport->xprt_err = status ? -EACCES : 0;
> +	complete(&transport->handshake_done);
> +	xprt_put(xprt);
> +}
> +
> +static int xs_tls_handshake_sync(struct rpc_xprt *xprt, unsigned int xprtsec)
> +{
> +	struct sock_xprt *transport =
> +				container_of(xprt, struct sock_xprt, xprt);
> +	int rc;
> +
> +	init_completion(&transport->handshake_done);
> +	set_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
> +
> +	transport->xprt_err = -ETIMEDOUT;
> +	switch (xprtsec) {
> +	case RPC_XPRTSEC_TLS_X509:
> +		rc = tls_client_hello_x509(transport->sock,
> +					   xs_tls_handshake_done, xprt_get(xprt),
> +					   TLSH_DEFAULT_PRIORITIES, TLSH_NO_PEERID,
> +					   TLSH_NO_CERT);
> +		break;
> +	case RPC_XPRTSEC_TLS_PSK:
> +		rc = tls_client_hello_psk(transport->sock, xs_tls_handshake_done,
> +					  xprt_get(xprt), TLSH_DEFAULT_PRIORITIES,
> +					  TLSH_NO_PEERID);
> +		break;
> +	default:
> +		rc = -EACCES;
> +	}
> +	if (rc)
> +		goto out;
> +
> +	rc = wait_for_completion_interruptible_timeout(&transport->handshake_done,
> +						       XS_TLS_HANDSHAKE_TO);

Should this be interruptible or killable? I'm not sure we want to give
up on a non-fatal signal, do we? (e.g. SIGCHLD).

Actually...it looks like this function always runs in workqueue context,
so this should probably just be wait_for_completion...or better yet,
consider doing this asynchronously so we don't block a workqueue thread.

> +	if (rc < 0)
> +		goto out;
> +
> +	rc = transport->xprt_err;
> +
> +out:
> +	xs_stream_reset_connect(transport);
> +	clear_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
> +	return rc;
> +}
> +
> +/*
> + * Transfer the connected socket to @dst_transport, then mark that
> + * xprt CONNECTED.
> + */
> +static int xs_tls_finish_connecting(struct rpc_xprt *src_xprt,
> +				    struct sock_xprt *dst_transport)
> +{
> +	struct sock_xprt *src_transport =
> +			container_of(src_xprt, struct sock_xprt, xprt);
> +	struct rpc_xprt *dst_xprt = &dst_transport->xprt;
> +
> +	if (!dst_transport->inet) {
> +		struct socket *sock = src_transport->sock;
> +		struct sock *sk = sock->sk;
> +
> +		/* Avoid temporary address, they are bad for long-lived
> +		 * connections such as NFS mounts.
> +		 * RFC4941, section 3.6 suggests that:
> +		 *    Individual applications, which have specific
> +		 *    knowledge about the normal duration of connections,
> +		 *    MAY override this as appropriate.
> +		 */
> +		if (xs_addr(dst_xprt)->sa_family == PF_INET6) {
> +			ip6_sock_set_addr_preferences(sk,
> +				IPV6_PREFER_SRC_PUBLIC);
> +		}
> +
> +		xs_tcp_set_socket_timeouts(dst_xprt, sock);
> +		tcp_sock_set_nodelay(sk);
> +
> +		lock_sock(sk);
> +
> +		/*
> +		 * @sk is already connected, so it now has the RPC callbacks.
> +		 * Reach into @src_transport to save the original ones.
> +		 */
> +		dst_transport->old_data_ready = src_transport->old_data_ready;
> +		dst_transport->old_state_change = src_transport->old_state_change;
> +		dst_transport->old_write_space = src_transport->old_write_space;
> +		dst_transport->old_error_report = src_transport->old_error_report;
> +		sk->sk_user_data = dst_xprt;
> +
> +		/* socket options */
> +		sock_reset_flag(sk, SOCK_LINGER);
> +
> +		xprt_clear_connected(dst_xprt);
> +
> +		dst_transport->sock = sock;
> +		dst_transport->inet = sk;
> +		dst_transport->file = src_transport->file;
> +
> +		release_sock(sk);
> +
> +		/* Reset src_transport before shutting down its clnt */
> +		mutex_lock(&src_transport->recv_mutex);
> +		src_transport->inet = NULL;
> +		src_transport->sock = NULL;
> +		src_transport->file = NULL;
> +
> +		xprt_clear_connected(src_xprt);
> +		xs_sock_reset_connection_flags(src_xprt);
> +		xs_stream_reset_connect(src_transport);
> +		mutex_unlock(&src_transport->recv_mutex);
> +	}
> +
> +	if (!xprt_bound(dst_xprt))
> +		return -ENOTCONN;
> +
> +	xs_set_memalloc(dst_xprt);
> +
> +	if (!xprt_test_and_set_connected(dst_xprt)) {
> +		dst_xprt->connect_cookie++;
> +		clear_bit(XPRT_SOCK_CONNECTING, &dst_transport->sock_state);
> +		xprt_clear_connecting(dst_xprt);
> +
> +		dst_xprt->stat.connect_count++;
> +		dst_xprt->stat.connect_time += (long)jiffies -
> +					   dst_xprt->stat.connect_start;
> +		xs_run_error_worker(dst_transport, XPRT_SOCK_WAKE_PENDING);
> +	}
> +	return 0;
> +}
> +
>  /**
>   * xs_tls_connect - establish a TLS session on a socket
>   * @work: queued work item
> @@ -2433,61 +2582,70 @@ static void xs_tls_connect(struct work_struct *work)
>  {
>  	struct sock_xprt *transport =
>  		container_of(work, struct sock_xprt, connect_worker.work);
> +	struct rpc_create_args args = {
> +		.net		= transport->xprt.xprt_net,
> +		.protocol	= transport->xprt.prot,
> +		.address	= (struct sockaddr *)&transport->xprt.addr,
> +		.addrsize	= transport->xprt.addrlen,
> +		.timeout	= transport->xprtsec_clnt->cl_timeout,
> +		.servername	= transport->xprt.servername,
> +		.nodename	= transport->xprtsec_clnt->cl_nodename,
> +		.program	= transport->xprtsec_clnt->cl_program,
> +		.prognumber	= transport->xprtsec_clnt->cl_prog,
> +		.version	= transport->xprtsec_clnt->cl_vers,
> +		.authflavor	= RPC_AUTH_TLS,
> +		.cred		= transport->xprtsec_clnt->cl_cred,
> +		.xprtsec	= RPC_XPRTSEC_NONE,
> +	};
> +	unsigned int pflags = current->flags;
>  	struct rpc_clnt *clnt;
> +	struct rpc_xprt *xprt;
> +	int status;
>  
> -	clnt = transport->xprtsec_clnt;
> -	transport->xprtsec_clnt = NULL;
> +	if (atomic_read(&transport->xprt.swapper))
> +		current->flags |= PF_MEMALLOC;
> +
> +	xs_stream_start_connect(transport);
> +
> +	clnt = rpc_create(&args);
>  	if (IS_ERR(clnt))
>  		goto out_unlock;
> +	rcu_read_lock();
> +	xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
> +	rcu_read_unlock();
>  
> -	xs_tcp_setup_socket(work);
> +	status = xs_tls_handshake_sync(xprt, transport->xprt.xprtsec);
> +	if (status)
> +		goto out_close;
>  
> +	status = xs_tls_finish_connecting(xprt, transport);
> +	if (status)
> +		goto out_close;
> +	trace_rpc_socket_connect(&transport->xprt, transport->sock, 0);
> +
> +	xprt_put(xprt);
>  	rpc_shutdown_client(clnt);
>  
>  out_unlock:
> +	xprt_unlock_connect(&transport->xprt, transport);
> +	current_restore_flags(pflags, PF_MEMALLOC);
> +	transport->xprtsec_clnt = NULL;
>  	return;
> -}
>  
> -static void xs_set_xprtsec_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	= RPC_XPRTSEC_NONE,
> -		.flags		= RPC_CLNT_CREATE_NOPING,
> -	};
> -
> -	switch (xprt->xprtsec) {
> -	case RPC_XPRTSEC_TLS_X509:
> -	case RPC_XPRTSEC_TLS_PSK:
> -		transport->xprtsec_clnt = rpc_create(&args);
> -		break;
> -	default:
> -		transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
> -	}
> -}
> -
> -#else
> -
> -static void xs_set_xprtsec_clnt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
> -{
> -	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> +out_close:
> +	xprt_put(xprt);
> +	rpc_shutdown_client(clnt);
>  
> -	transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
> +	/* xprt_force_disconnect() wakes tasks with a fixed tk_status code.
> +	 * Wake them first here to ensure they get our tk_status code.
> +	 */
> +	xprt_wake_pending_tasks(&transport->xprt, status);
> +	xs_tcp_force_close(&transport->xprt);
> +	xprt_clear_connecting(&transport->xprt);
> +	goto out_unlock;
>  }
>  
> -#endif /*CONFIG_TLS */
> +#endif /* CONFIG_TLS */
>  
>  /**
>   * xs_connect - connect a socket to a remote endpoint
> @@ -2520,8 +2678,7 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
>  	} else
>  		dprintk("RPC:       xs_connect scheduled xprt %p\n", xprt);
>  
> -	xs_set_xprtsec_clnt(task->tk_client, xprt);
> -
> +	transport->xprtsec_clnt = task->tk_client;
>  	queue_delayed_work(xprtiod_workqueue,
>  			&transport->connect_worker,
>  			delay);
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 15/15] NFS: Add an "xprtsec=" NFS mount option

[Jeff Layton]

On Mon, 2022-06-06 at 10:52 -0400, Chuck Lever wrote:
> After some discussion, we decided that controlling transport layer
> security policy should be separate from the setting for the user
> authentication flavor. To accomplish this, add a new NFS mount
> option to select a transport layer security policy for RPC
> operations associated with the mount point.
> 
>   xprtsec=none     - Transport layer security is disabled.
> 
>   xprtsec=tls      - Establish an encryption-only TLS session. If
>                      the initial handshake fails, the mount fails.
>                      If TLS is not available on a reconnect, drop
>                      the connection and try again.
> 
> The mount.nfs command will provide an addition setting:
> 
>   xprtsec=auto     - Try to establish a TLS session, but proceed
>                      with no transport layer security if that fails.
> 
> Updates to mount.nfs and nfs(5) will be sent under separate cover.
> 

Seems like a reasonable interface.

> Future work:
> 
> To support client peer authentication, the plan is to add another
> xprtsec= choice called "mtls" which will require a second mount
> option that specifies the pathname of a directory containing the
> private key and an x.509 certificate.
> 
> Similarly, pre-shared key authentication can be supported by adding
> support for "xprtsec=psk" along with a second mount option that
> specifies the name of a file containing the key.
> 


^^^
We might want something more flexible than this over the long haul. 
Container orchestration like kubernetes would probably prefer to manage
this without sprinkling files all over.

Maybe we can allow this info to be set in the kernel keyring of the
mounting process instead of requiring files? In any case, we can cross
that bridge when we come to it.

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/client.c     |   10 +++++++++-
>  fs/nfs/fs_context.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  fs/nfs/internal.h   |    1 +
>  fs/nfs/nfs4client.c |    2 +-
>  fs/nfs/super.c      |    7 +++++++
>  5 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 0896e4f047d1..1f26c1ad18b3 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -530,6 +530,14 @@ int nfs_create_rpc_client(struct nfs_client *clp,
>  	if (test_bit(NFS_CS_REUSEPORT, &clp->cl_flags))
>  		args.flags |= RPC_CLNT_CREATE_REUSEPORT;
>  
> +	switch (clp->cl_xprtsec) {
> +	case NFS_CS_XPRTSEC_TLS:
> +		args.xprtsec = RPC_XPRTSEC_TLS_X509;
> +		break;
> +	default:
> +		args.xprtsec = RPC_XPRTSEC_NONE;
> +	}
> +
>  	if (!IS_ERR(clp->cl_rpcclient))
>  		return 0;
>  
> @@ -680,7 +688,7 @@ static int nfs_init_server(struct nfs_server *server,
>  		.cred = server->cred,
>  		.nconnect = ctx->nfs_server.nconnect,
>  		.init_flags = (1UL << NFS_CS_REUSEPORT),
> -		.xprtsec_policy = NFS_CS_XPRTSEC_NONE,
> +		.xprtsec_policy = ctx->xprtsec_policy,
>  	};
>  	struct nfs_client *clp;
>  	int error;
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 35e400a557b9..3e29dd88b59b 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -88,6 +88,7 @@ enum nfs_param {
>  	Opt_vers,
>  	Opt_wsize,
>  	Opt_write,
> +	Opt_xprtsec,
>  };
>  
>  enum {
> @@ -194,6 +195,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>  	fsparam_string("vers",		Opt_vers),
>  	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
>  	fsparam_u32   ("wsize",		Opt_wsize),
> +	fsparam_string("xprtsec",	Opt_xprtsec),
>  	{}
>  };
>  
> @@ -267,6 +269,18 @@ static const struct constant_table nfs_secflavor_tokens[] = {
>  	{}
>  };
>  
> +enum {
> +	Opt_xprtsec_none,
> +	Opt_xprtsec_tls,
> +	nr__Opt_xprtsec
> +};
> +
> +static const struct constant_table nfs_xprtsec_policies[] = {
> +	{ "none",	Opt_xprtsec_none },
> +	{ "tls",	Opt_xprtsec_tls },
> +	{}
> +};
> +
>  /*
>   * Sanity-check a server address provided by the mount command.
>   *
> @@ -431,6 +445,26 @@ static int nfs_parse_security_flavors(struct fs_context *fc,
>  	return 0;
>  }
>  
> +static int nfs_parse_xprtsec_policy(struct fs_context *fc,
> +				    struct fs_parameter *param)
> +{
> +	struct nfs_fs_context *ctx = nfs_fc2context(fc);
> +
> +	trace_nfs_mount_assign(param->key, param->string);
> +
> +	switch (lookup_constant(nfs_xprtsec_policies, param->string, -1)) {
> +	case Opt_xprtsec_none:
> +		ctx->xprtsec_policy = NFS_CS_XPRTSEC_NONE;
> +		break;
> +	case Opt_xprtsec_tls:
> +		ctx->xprtsec_policy = NFS_CS_XPRTSEC_TLS;
> +		break;
> +	default:
> +		return nfs_invalf(fc, "NFS: Unrecognized transport security policy");
> +	}
> +	return 0;
> +}
> +
>  static int nfs_parse_version_string(struct fs_context *fc,
>  				    const char *string)
>  {
> @@ -695,6 +729,11 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		if (ret < 0)
>  			return ret;
>  		break;
> +	case Opt_xprtsec:
> +		ret = nfs_parse_xprtsec_policy(fc, param);
> +		if (ret < 0)
> +			return ret;
> +		break;
>  
>  	case Opt_proto:
>  		trace_nfs_mount_assign(param->key, param->string);
> @@ -1564,6 +1603,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
>  		ctx->selected_flavor	= RPC_AUTH_MAXFLAVOR;
>  		ctx->minorversion	= 0;
>  		ctx->need_mount		= true;
> +		ctx->xprtsec_policy	= NFS_CS_XPRTSEC_NONE;
>  
>  		fc->s_iflags		|= SB_I_STABLE_WRITES;
>  	}
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 0a3512c39376..bc60a556ad92 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -102,6 +102,7 @@ struct nfs_fs_context {
>  	unsigned int		bsize;
>  	struct nfs_auth_info	auth_info;
>  	rpc_authflavor_t	selected_flavor;
> +	unsigned int		xprtsec_policy;
>  	char			*client_address;
>  	unsigned int		version;
>  	unsigned int		minorversion;
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 682d47e5977b..8dbdb00859fe 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -1159,7 +1159,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
>  				ctx->nfs_server.nconnect,
>  				ctx->nfs_server.max_connect,
>  				fc->net_ns,
> -				NFS_CS_XPRTSEC_NONE);
> +				ctx->xprtsec_policy);
>  	if (error < 0)
>  		return error;
>  
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 6ab5eeb000dc..66da994500f9 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -491,6 +491,13 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
>  	seq_printf(m, ",timeo=%lu", 10U * nfss->client->cl_timeout->to_initval / HZ);
>  	seq_printf(m, ",retrans=%u", nfss->client->cl_timeout->to_retries);
>  	seq_printf(m, ",sec=%s", nfs_pseudoflavour_to_name(nfss->client->cl_auth->au_flavor));
> +	switch (clp->cl_xprtsec) {
> +	case NFS_CS_XPRTSEC_TLS:
> +		seq_printf(m, ",xprtsec=tls");
> +		break;
> +	default:
> +		break;
> +	}
>  
>  	if (version != 4)
>  		nfs_show_mountd_options(m, nfss, showdefaults);
> 
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH v2 00/15] RPC-with-TLS client side

[Jeff Layton]

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> Now that the initial v5.19 merge window has closed, it's time for
> another round of review for RPC-with-TLS support in the Linux NFS
> client. This is just the RPC-specific portions. The full series is
> available in the "topic-rpc-with-tls-upcall" branch here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> I've taken two or three steps towards implementing the architecture
> Trond requested during the last review. There is now a two-stage
> connection establishment process so that the upper level can use
> XPRT_CONNECTED to determine when a TLS session is ready to use.
> There are probably additional changes and simplifications that can
> be made. Please review and provide feedback.
> 
> I wanted to make more progress on client-side authentication (ie,
> passing an x.509 cert from the client to the server) but NFSD bugs
> have taken all my time for the past few weeks.
> 
> 
> Changes since v1:
> - Rebased on v5.18
> - Re-ordered so generic fixes come first
> - Addressed some of Trond's review comments
> 
> ---
> 
> Chuck Lever (15):
>       SUNRPC: Fail faster on bad verifier
>       SUNRPC: Widen rpc_task::tk_flags
>       SUNRPC: Replace dprintk() call site in xs_data_ready
>       NFS: Replace fs_context-related dprintk() call sites with tracepoints
>       SUNRPC: Plumb an API for setting transport layer security
>       SUNRPC: Trace the rpc_create_args
>       SUNRPC: Refactor rpc_call_null_helper()
>       SUNRPC: Add RPC client support for the RPC_AUTH_TLS auth flavor
>       SUNRPC: Ignore data_ready callbacks during TLS handshakes
>       SUNRPC: Capture cmsg metadata on client-side receive
>       SUNRPC: Add a connect worker function for TLS
>       SUNRPC: Add RPC-with-TLS support to xprtsock.c
>       SUNRPC: Add RPC-with-TLS tracepoints
>       NFS: Have struct nfs_client carry a TLS policy field
>       NFS: Add an "xprtsec=" NFS mount option
> 
> 
>  fs/nfs/client.c                 |  14 ++
>  fs/nfs/fs_context.c             |  65 +++++--
>  fs/nfs/internal.h               |   2 +
>  fs/nfs/nfs3client.c             |   1 +
>  fs/nfs/nfs4client.c             |  16 +-
>  fs/nfs/nfstrace.h               |  77 ++++++++
>  fs/nfs/super.c                  |   7 +
>  include/linux/nfs_fs_sb.h       |   5 +-
>  include/linux/sunrpc/auth.h     |   1 +
>  include/linux/sunrpc/clnt.h     |  15 +-
>  include/linux/sunrpc/sched.h    |  32 ++--
>  include/linux/sunrpc/xprt.h     |   2 +
>  include/linux/sunrpc/xprtsock.h |   4 +
>  include/net/tls.h               |   2 +
>  include/trace/events/sunrpc.h   | 157 ++++++++++++++--
>  net/sunrpc/Makefile             |   2 +-
>  net/sunrpc/auth.c               |   2 +-
>  net/sunrpc/auth_tls.c           | 120 +++++++++++++
>  net/sunrpc/clnt.c               |  34 ++--
>  net/sunrpc/debugfs.c            |   2 +-
>  net/sunrpc/xprtsock.c           | 310 +++++++++++++++++++++++++++++++-
>  21 files changed, 805 insertions(+), 65 deletions(-)
>  create mode 100644 net/sunrpc/auth_tls.c
> 
> --
> Chuck Lever
> 

This looks pretty good overall. Nice work, Chuck. FWIW, I pulled these
and ktls-utils down and gave them a spin and they worked just fine. You
can add:

Tested-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 15/15] NFS: Add an "xprtsec=" NFS mount option

[Chuck Lever III]

> On Jul 18, 2022, at 4:24 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> On Mon, 2022-06-06 at 10:52 -0400, Chuck Lever wrote:
>> After some discussion, we decided that controlling transport layer
>> security policy should be separate from the setting for the user
>> authentication flavor. To accomplish this, add a new NFS mount
>> option to select a transport layer security policy for RPC
>> operations associated with the mount point.
>> 
>>  xprtsec=none     - Transport layer security is disabled.
>> 
>>  xprtsec=tls      - Establish an encryption-only TLS session. If
>>                     the initial handshake fails, the mount fails.
>>                     If TLS is not available on a reconnect, drop
>>                     the connection and try again.
>> 
>> The mount.nfs command will provide an addition setting:
>> 
>>  xprtsec=auto     - Try to establish a TLS session, but proceed
>>                     with no transport layer security if that fails.
>> 
>> Updates to mount.nfs and nfs(5) will be sent under separate cover.
>> 
> 
> Seems like a reasonable interface.

Note we are trying to keep the administrative interfaces on all
implementations as similar as is possible. Some of this may feel
a little non-Linuxy, and that might be why.


>> Future work:
>> 
>> To support client peer authentication, the plan is to add another
>> xprtsec= choice called "mtls" which will require a second mount
>> option that specifies the pathname of a directory containing the
>> private key and an x.509 certificate.
>> 
>> Similarly, pre-shared key authentication can be supported by adding
>> support for "xprtsec=psk" along with a second mount option that
>> specifies the name of a file containing the key.
>> 
> 
> 
> ^^^
> We might want something more flexible than this over the long haul. 
> Container orchestration like kubernetes would probably prefer to manage
> this without sprinkling files all over.
> 
> Maybe we can allow this info to be set in the kernel keyring of the
> mounting process instead of requiring files? In any case, we can cross
> that bridge when we come to it.

In the prototype I'm working on now, mount.nfs will read the client's
certificate from a file and use add_key(2) to inject it into the
kernel. It passes the serial number of that key to the kernel via
mount(2).

During the TLS handshake, the kernel gives a connected socket and
the key's serial number to tlshd to do the handshake with client
TLS authentication.

When we get to the server-side, it's likely that rpc.nfsd or
some other utility will do the same for the server's certificate.

So we do indeed plan to use keyrings for this.

Meanwhile, if you have some specific recommendations for handling
the orchestration requirements, or know someone who has informed
opinions about it that I should talk to, please hook me up!


>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfs/client.c     |   10 +++++++++-
>> fs/nfs/fs_context.c |   40 ++++++++++++++++++++++++++++++++++++++++
>> fs/nfs/internal.h   |    1 +
>> fs/nfs/nfs4client.c |    2 +-
>> fs/nfs/super.c      |    7 +++++++
>> 5 files changed, 58 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>> index 0896e4f047d1..1f26c1ad18b3 100644
>> --- a/fs/nfs/client.c
>> +++ b/fs/nfs/client.c
>> @@ -530,6 +530,14 @@ int nfs_create_rpc_client(struct nfs_client *clp,
>> 	if (test_bit(NFS_CS_REUSEPORT, &clp->cl_flags))
>> 		args.flags |= RPC_CLNT_CREATE_REUSEPORT;
>> 
>> +	switch (clp->cl_xprtsec) {
>> +	case NFS_CS_XPRTSEC_TLS:
>> +		args.xprtsec = RPC_XPRTSEC_TLS_X509;
>> +		break;
>> +	default:
>> +		args.xprtsec = RPC_XPRTSEC_NONE;
>> +	}
>> +
>> 	if (!IS_ERR(clp->cl_rpcclient))
>> 		return 0;
>> 
>> @@ -680,7 +688,7 @@ static int nfs_init_server(struct nfs_server *server,
>> 		.cred = server->cred,
>> 		.nconnect = ctx->nfs_server.nconnect,
>> 		.init_flags = (1UL << NFS_CS_REUSEPORT),
>> -		.xprtsec_policy = NFS_CS_XPRTSEC_NONE,
>> +		.xprtsec_policy = ctx->xprtsec_policy,
>> 	};
>> 	struct nfs_client *clp;
>> 	int error;
>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>> index 35e400a557b9..3e29dd88b59b 100644
>> --- a/fs/nfs/fs_context.c
>> +++ b/fs/nfs/fs_context.c
>> @@ -88,6 +88,7 @@ enum nfs_param {
>> 	Opt_vers,
>> 	Opt_wsize,
>> 	Opt_write,
>> +	Opt_xprtsec,
>> };
>> 
>> enum {
>> @@ -194,6 +195,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>> 	fsparam_string("vers",		Opt_vers),
>> 	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
>> 	fsparam_u32   ("wsize",		Opt_wsize),
>> +	fsparam_string("xprtsec",	Opt_xprtsec),
>> 	{}
>> };
>> 
>> @@ -267,6 +269,18 @@ static const struct constant_table nfs_secflavor_tokens[] = {
>> 	{}
>> };
>> 
>> +enum {
>> +	Opt_xprtsec_none,
>> +	Opt_xprtsec_tls,
>> +	nr__Opt_xprtsec
>> +};
>> +
>> +static const struct constant_table nfs_xprtsec_policies[] = {
>> +	{ "none",	Opt_xprtsec_none },
>> +	{ "tls",	Opt_xprtsec_tls },
>> +	{}
>> +};
>> +
>> /*
>>  * Sanity-check a server address provided by the mount command.
>>  *
>> @@ -431,6 +445,26 @@ static int nfs_parse_security_flavors(struct fs_context *fc,
>> 	return 0;
>> }
>> 
>> +static int nfs_parse_xprtsec_policy(struct fs_context *fc,
>> +				    struct fs_parameter *param)
>> +{
>> +	struct nfs_fs_context *ctx = nfs_fc2context(fc);
>> +
>> +	trace_nfs_mount_assign(param->key, param->string);
>> +
>> +	switch (lookup_constant(nfs_xprtsec_policies, param->string, -1)) {
>> +	case Opt_xprtsec_none:
>> +		ctx->xprtsec_policy = NFS_CS_XPRTSEC_NONE;
>> +		break;
>> +	case Opt_xprtsec_tls:
>> +		ctx->xprtsec_policy = NFS_CS_XPRTSEC_TLS;
>> +		break;
>> +	default:
>> +		return nfs_invalf(fc, "NFS: Unrecognized transport security policy");
>> +	}
>> +	return 0;
>> +}
>> +
>> static int nfs_parse_version_string(struct fs_context *fc,
>> 				    const char *string)
>> {
>> @@ -695,6 +729,11 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>> 		if (ret < 0)
>> 			return ret;
>> 		break;
>> +	case Opt_xprtsec:
>> +		ret = nfs_parse_xprtsec_policy(fc, param);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> 
>> 	case Opt_proto:
>> 		trace_nfs_mount_assign(param->key, param->string);
>> @@ -1564,6 +1603,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
>> 		ctx->selected_flavor	= RPC_AUTH_MAXFLAVOR;
>> 		ctx->minorversion	= 0;
>> 		ctx->need_mount		= true;
>> +		ctx->xprtsec_policy	= NFS_CS_XPRTSEC_NONE;
>> 
>> 		fc->s_iflags		|= SB_I_STABLE_WRITES;
>> 	}
>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>> index 0a3512c39376..bc60a556ad92 100644
>> --- a/fs/nfs/internal.h
>> +++ b/fs/nfs/internal.h
>> @@ -102,6 +102,7 @@ struct nfs_fs_context {
>> 	unsigned int		bsize;
>> 	struct nfs_auth_info	auth_info;
>> 	rpc_authflavor_t	selected_flavor;
>> +	unsigned int		xprtsec_policy;
>> 	char			*client_address;
>> 	unsigned int		version;
>> 	unsigned int		minorversion;
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index 682d47e5977b..8dbdb00859fe 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -1159,7 +1159,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc)
>> 				ctx->nfs_server.nconnect,
>> 				ctx->nfs_server.max_connect,
>> 				fc->net_ns,
>> -				NFS_CS_XPRTSEC_NONE);
>> +				ctx->xprtsec_policy);
>> 	if (error < 0)
>> 		return error;
>> 
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 6ab5eeb000dc..66da994500f9 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -491,6 +491,13 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
>> 	seq_printf(m, ",timeo=%lu", 10U * nfss->client->cl_timeout->to_initval / HZ);
>> 	seq_printf(m, ",retrans=%u", nfss->client->cl_timeout->to_retries);
>> 	seq_printf(m, ",sec=%s", nfs_pseudoflavour_to_name(nfss->client->cl_auth->au_flavor));
>> +	switch (clp->cl_xprtsec) {
>> +	case NFS_CS_XPRTSEC_TLS:
>> +		seq_printf(m, ",xprtsec=tls");
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> 
>> 	if (version != 4)
>> 		nfs_show_mountd_options(m, nfss, showdefaults);
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

--
Chuck Lever

Re: [PATCH v2 12/15] SUNRPC: Add RPC-with-TLS support to xprtsock.c

[Chuck Lever III]

> On Jul 18, 2022, at 4:10 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/linux/sunrpc/xprtsock.h |    1 
>> net/sunrpc/xprtsock.c           |  243 ++++++++++++++++++++++++++++++++-------
>> 2 files changed, 201 insertions(+), 43 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
>> index e0b6009f1f69..eaf3d705f758 100644
>> --- a/include/linux/sunrpc/xprtsock.h
>> +++ b/include/linux/sunrpc/xprtsock.h
>> @@ -57,6 +57,7 @@ struct sock_xprt {
>> 	struct work_struct	error_worker;
>> 	struct work_struct	recv_worker;
>> 	struct mutex		recv_mutex;
>> +	struct completion	handshake_done;
>> 	struct sockaddr_storage	srcaddr;
>> 	unsigned short		srcport;
>> 	int			xprt_err;
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index a4fee00412d4..63fe97ede573 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -48,6 +48,7 @@
>> #include <net/udp.h>
>> #include <net/tcp.h>
>> #include <net/tls.h>
>> +#include <net/tlsh.h>
>> 
>> #include <linux/bvec.h>
>> #include <linux/highmem.h>
>> @@ -197,6 +198,11 @@ static struct ctl_table sunrpc_table[] = {
>>  */
>> #define XS_IDLE_DISC_TO		(5U * 60 * HZ)
>> 
>> +/*
>> + * TLS handshake timeout.
>> + */
>> +#define XS_TLS_HANDSHAKE_TO    (20U * HZ)
>> +
>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>> # undef  RPC_DEBUG_DATA
>> # define RPCDBG_FACILITY	RPCDBG_TRANS
>> @@ -1254,6 +1260,8 @@ static void xs_reset_transport(struct sock_xprt *transport)
>> 	if (atomic_read(&transport->xprt.swapper))
>> 		sk_clear_memalloc(sk);
>> 
>> +	/* TODO: Maybe send a TLS Closure alert */
>> +
>> 	kernel_sock_shutdown(sock, SHUT_RDWR);
>> 
>> 	mutex_lock(&transport->recv_mutex);
>> @@ -2424,6 +2432,147 @@ static void xs_tcp_setup_socket(struct work_struct *work)
>> 
>> #if IS_ENABLED(CONFIG_TLS)
>> 
>> +/**
>> + * xs_tls_handshake_done - TLS handshake completion handler
>> + * @data: address of xprt to wake
>> + * @status: status of handshake
>> + *
>> + */
>> +static void xs_tls_handshake_done(void *data, int status)
>> +{
>> +	struct rpc_xprt *xprt = data;
>> +	struct sock_xprt *transport =
>> +				container_of(xprt, struct sock_xprt, xprt);
>> +
>> +	transport->xprt_err = status ? -EACCES : 0;
>> +	complete(&transport->handshake_done);
>> +	xprt_put(xprt);
>> +}
>> +
>> +static int xs_tls_handshake_sync(struct rpc_xprt *xprt, unsigned int xprtsec)
>> +{
>> +	struct sock_xprt *transport =
>> +				container_of(xprt, struct sock_xprt, xprt);
>> +	int rc;
>> +
>> +	init_completion(&transport->handshake_done);
>> +	set_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
>> +
>> +	transport->xprt_err = -ETIMEDOUT;
>> +	switch (xprtsec) {
>> +	case RPC_XPRTSEC_TLS_X509:
>> +		rc = tls_client_hello_x509(transport->sock,
>> +					   xs_tls_handshake_done, xprt_get(xprt),
>> +					   TLSH_DEFAULT_PRIORITIES, TLSH_NO_PEERID,
>> +					   TLSH_NO_CERT);
>> +		break;
>> +	case RPC_XPRTSEC_TLS_PSK:
>> +		rc = tls_client_hello_psk(transport->sock, xs_tls_handshake_done,
>> +					  xprt_get(xprt), TLSH_DEFAULT_PRIORITIES,
>> +					  TLSH_NO_PEERID);
>> +		break;
>> +	default:
>> +		rc = -EACCES;
>> +	}
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = wait_for_completion_interruptible_timeout(&transport->handshake_done,
>> +						       XS_TLS_HANDSHAKE_TO);
> 
> Should this be interruptible or killable? I'm not sure we want to give
> up on a non-fatal signal, do we? (e.g. SIGCHLD).
> 
> Actually...it looks like this function always runs in workqueue context,
> so this should probably just be wait_for_completion...or better yet,
> consider doing this asynchronously so we don't block a workqueue thread.

This code is similar to rpcrdma_create_id(), which IIRC also runs in
a workqueue thread and has been this way for many years.

If one is wrong, the other is too. But I see recently merged code (in
drivers/nvme/rdma/host.c) that does exactly the same.

<shrug> I'm not married to the approach used in the patch, but it
doesn't seem strange to me.


>> +	if (rc < 0)
>> +		goto out;
>> +
>> +	rc = transport->xprt_err;
>> +
>> +out:
>> +	xs_stream_reset_connect(transport);
>> +	clear_bit(XPRT_SOCK_IGNORE_RECV, &transport->sock_state);
>> +	return rc;
>> +}
>> +
>> +/*
>> + * Transfer the connected socket to @dst_transport, then mark that
>> + * xprt CONNECTED.
>> + */
>> +static int xs_tls_finish_connecting(struct rpc_xprt *src_xprt,
>> +				    struct sock_xprt *dst_transport)
>> +{
>> +	struct sock_xprt *src_transport =
>> +			container_of(src_xprt, struct sock_xprt, xprt);
>> +	struct rpc_xprt *dst_xprt = &dst_transport->xprt;
>> +
>> +	if (!dst_transport->inet) {
>> +		struct socket *sock = src_transport->sock;
>> +		struct sock *sk = sock->sk;
>> +
>> +		/* Avoid temporary address, they are bad for long-lived
>> +		 * connections such as NFS mounts.
>> +		 * RFC4941, section 3.6 suggests that:
>> +		 *    Individual applications, which have specific
>> +		 *    knowledge about the normal duration of connections,
>> +		 *    MAY override this as appropriate.
>> +		 */
>> +		if (xs_addr(dst_xprt)->sa_family == PF_INET6) {
>> +			ip6_sock_set_addr_preferences(sk,
>> +				IPV6_PREFER_SRC_PUBLIC);
>> +		}
>> +
>> +		xs_tcp_set_socket_timeouts(dst_xprt, sock);
>> +		tcp_sock_set_nodelay(sk);
>> +
>> +		lock_sock(sk);
>> +
>> +		/*
>> +		 * @sk is already connected, so it now has the RPC callbacks.
>> +		 * Reach into @src_transport to save the original ones.
>> +		 */
>> +		dst_transport->old_data_ready = src_transport->old_data_ready;
>> +		dst_transport->old_state_change = src_transport->old_state_change;
>> +		dst_transport->old_write_space = src_transport->old_write_space;
>> +		dst_transport->old_error_report = src_transport->old_error_report;
>> +		sk->sk_user_data = dst_xprt;
>> +
>> +		/* socket options */
>> +		sock_reset_flag(sk, SOCK_LINGER);
>> +
>> +		xprt_clear_connected(dst_xprt);
>> +
>> +		dst_transport->sock = sock;
>> +		dst_transport->inet = sk;
>> +		dst_transport->file = src_transport->file;
>> +
>> +		release_sock(sk);
>> +
>> +		/* Reset src_transport before shutting down its clnt */
>> +		mutex_lock(&src_transport->recv_mutex);
>> +		src_transport->inet = NULL;
>> +		src_transport->sock = NULL;
>> +		src_transport->file = NULL;
>> +
>> +		xprt_clear_connected(src_xprt);
>> +		xs_sock_reset_connection_flags(src_xprt);
>> +		xs_stream_reset_connect(src_transport);
>> +		mutex_unlock(&src_transport->recv_mutex);
>> +	}
>> +
>> +	if (!xprt_bound(dst_xprt))
>> +		return -ENOTCONN;
>> +
>> +	xs_set_memalloc(dst_xprt);
>> +
>> +	if (!xprt_test_and_set_connected(dst_xprt)) {
>> +		dst_xprt->connect_cookie++;
>> +		clear_bit(XPRT_SOCK_CONNECTING, &dst_transport->sock_state);
>> +		xprt_clear_connecting(dst_xprt);
>> +
>> +		dst_xprt->stat.connect_count++;
>> +		dst_xprt->stat.connect_time += (long)jiffies -
>> +					   dst_xprt->stat.connect_start;
>> +		xs_run_error_worker(dst_transport, XPRT_SOCK_WAKE_PENDING);
>> +	}
>> +	return 0;
>> +}
>> +
>> /**
>>  * xs_tls_connect - establish a TLS session on a socket
>>  * @work: queued work item
>> @@ -2433,61 +2582,70 @@ static void xs_tls_connect(struct work_struct *work)
>> {
>> 	struct sock_xprt *transport =
>> 		container_of(work, struct sock_xprt, connect_worker.work);
>> +	struct rpc_create_args args = {
>> +		.net		= transport->xprt.xprt_net,
>> +		.protocol	= transport->xprt.prot,
>> +		.address	= (struct sockaddr *)&transport->xprt.addr,
>> +		.addrsize	= transport->xprt.addrlen,
>> +		.timeout	= transport->xprtsec_clnt->cl_timeout,
>> +		.servername	= transport->xprt.servername,
>> +		.nodename	= transport->xprtsec_clnt->cl_nodename,
>> +		.program	= transport->xprtsec_clnt->cl_program,
>> +		.prognumber	= transport->xprtsec_clnt->cl_prog,
>> +		.version	= transport->xprtsec_clnt->cl_vers,
>> +		.authflavor	= RPC_AUTH_TLS,
>> +		.cred		= transport->xprtsec_clnt->cl_cred,
>> +		.xprtsec	= RPC_XPRTSEC_NONE,
>> +	};
>> +	unsigned int pflags = current->flags;
>> 	struct rpc_clnt *clnt;
>> +	struct rpc_xprt *xprt;
>> +	int status;
>> 
>> -	clnt = transport->xprtsec_clnt;
>> -	transport->xprtsec_clnt = NULL;
>> +	if (atomic_read(&transport->xprt.swapper))
>> +		current->flags |= PF_MEMALLOC;
>> +
>> +	xs_stream_start_connect(transport);
>> +
>> +	clnt = rpc_create(&args);
>> 	if (IS_ERR(clnt))
>> 		goto out_unlock;
>> +	rcu_read_lock();
>> +	xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
>> +	rcu_read_unlock();
>> 
>> -	xs_tcp_setup_socket(work);
>> +	status = xs_tls_handshake_sync(xprt, transport->xprt.xprtsec);
>> +	if (status)
>> +		goto out_close;
>> 
>> +	status = xs_tls_finish_connecting(xprt, transport);
>> +	if (status)
>> +		goto out_close;
>> +	trace_rpc_socket_connect(&transport->xprt, transport->sock, 0);
>> +
>> +	xprt_put(xprt);
>> 	rpc_shutdown_client(clnt);
>> 
>> out_unlock:
>> +	xprt_unlock_connect(&transport->xprt, transport);
>> +	current_restore_flags(pflags, PF_MEMALLOC);
>> +	transport->xprtsec_clnt = NULL;
>> 	return;
>> -}
>> 
>> -static void xs_set_xprtsec_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	= RPC_XPRTSEC_NONE,
>> -		.flags		= RPC_CLNT_CREATE_NOPING,
>> -	};
>> -
>> -	switch (xprt->xprtsec) {
>> -	case RPC_XPRTSEC_TLS_X509:
>> -	case RPC_XPRTSEC_TLS_PSK:
>> -		transport->xprtsec_clnt = rpc_create(&args);
>> -		break;
>> -	default:
>> -		transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
>> -	}
>> -}
>> -
>> -#else
>> -
>> -static void xs_set_xprtsec_clnt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
>> -{
>> -	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
>> +out_close:
>> +	xprt_put(xprt);
>> +	rpc_shutdown_client(clnt);
>> 
>> -	transport->xprtsec_clnt = ERR_PTR(-ENOTCONN);
>> +	/* xprt_force_disconnect() wakes tasks with a fixed tk_status code.
>> +	 * Wake them first here to ensure they get our tk_status code.
>> +	 */
>> +	xprt_wake_pending_tasks(&transport->xprt, status);
>> +	xs_tcp_force_close(&transport->xprt);
>> +	xprt_clear_connecting(&transport->xprt);
>> +	goto out_unlock;
>> }
>> 
>> -#endif /*CONFIG_TLS */
>> +#endif /* CONFIG_TLS */
>> 
>> /**
>>  * xs_connect - connect a socket to a remote endpoint
>> @@ -2520,8 +2678,7 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
>> 	} else
>> 		dprintk("RPC:       xs_connect scheduled xprt %p\n", xprt);
>> 
>> -	xs_set_xprtsec_clnt(task->tk_client, xprt);
>> -
>> +	transport->xprtsec_clnt = task->tk_client;
>> 	queue_delayed_work(xprtiod_workqueue,
>> 			&transport->connect_worker,
>> 			delay);
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH v2 10/15] SUNRPC: Capture cmsg metadata on client-side receive

[Chuck Lever III]

> On Jul 18, 2022, at 3:53 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-06-06 at 10:51 -0400, Chuck Lever wrote:
>> kTLS sockets use cmsg to report decryption errors and the need
>> for session re-keying. An "application data" message contains a ULP
>> payload, and that is passed along to the RPC client. An "alert"
>> message triggers connection reset. Everything else is discarded.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/net/tls.h             |    2 ++
>> include/trace/events/sunrpc.h |   40 +++++++++++++++++++++++++++++++++
>> net/sunrpc/xprtsock.c         |   49 +++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 89 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/net/tls.h b/include/net/tls.h
>> index 6b1bf46daa34..54bccb2e4014 100644
>> --- a/include/net/tls.h
>> +++ b/include/net/tls.h
>> @@ -71,6 +71,8 @@ static inline struct tlsh_sock *tlsh_sk(struct sock *sk)
>> 
>> #define TLS_CRYPTO_INFO_READY(info)	((info)->cipher_type)
>> 
>> +#define TLS_RECORD_TYPE_ALERT		0x15
>> +#define TLS_RECORD_TYPE_HANDSHAKE	0x16
>> #define TLS_RECORD_TYPE_DATA		0x17
>> 
>> #define TLS_AAD_SPACE_SIZE		13
>> diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
>> index 986e135e314f..d7d07f3b850e 100644
>> --- a/include/trace/events/sunrpc.h
>> +++ b/include/trace/events/sunrpc.h
>> @@ -1319,6 +1319,46 @@ TRACE_EVENT(xs_data_ready,
>> 	TP_printk("peer=[%s]:%s", __get_str(addr), __get_str(port))
>> );
>> 
>> +/*
>> + * From https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml
>> + *
>> + * Captured March 2022. Other values are unassigned or reserved.
>> + */
>> +#define rpc_show_tls_content_type(type) \
>> +	__print_symbolic(type, \
>> +		{ 20,		"change cipher spec" }, \
>> +		{ 21,		"alert" }, \
>> +		{ 22,		"handshake" }, \
>> +		{ 23,		"application data" }, \
>> +		{ 24,		"heartbeat" }, \
>> +		{ 25,		"tls12_cid" }, \
>> +		{ 26,		"ACK" })
>> +
>> +TRACE_EVENT(xs_tls_contenttype,
>> +	TP_PROTO(
>> +		const struct rpc_xprt *xprt,
>> +		unsigned char ctype
>> +	),
>> +
>> +	TP_ARGS(xprt, ctype),
>> +
>> +	TP_STRUCT__entry(
>> +		__string(addr, xprt->address_strings[RPC_DISPLAY_ADDR])
>> +		__string(port, xprt->address_strings[RPC_DISPLAY_PORT])
>> +		__field(unsigned long, ctype)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__assign_str(addr, xprt->address_strings[RPC_DISPLAY_ADDR]);
>> +		__assign_str(port, xprt->address_strings[RPC_DISPLAY_PORT]);
>> +		__entry->ctype = ctype;
>> +	),
>> +
>> +	TP_printk("peer=[%s]:%s: %s", __get_str(addr), __get_str(port),
>> +		rpc_show_tls_content_type(__entry->ctype)
>> +	)
>> +);
>> +
>> TRACE_EVENT(xs_stream_read_data,
>> 	TP_PROTO(struct rpc_xprt *xprt, ssize_t err, size_t total),
>> 
>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>> index 0a521aee0b2f..c73af8f1c3d4 100644
>> --- a/net/sunrpc/xprtsock.c
>> +++ b/net/sunrpc/xprtsock.c
>> @@ -47,6 +47,8 @@
>> #include <net/checksum.h>
>> #include <net/udp.h>
>> #include <net/tcp.h>
>> +#include <net/tls.h>
>> +
>> #include <linux/bvec.h>
>> #include <linux/highmem.h>
>> #include <linux/uio.h>
>> @@ -350,13 +352,56 @@ xs_alloc_sparse_pages(struct xdr_buf *buf, size_t want, gfp_t gfp)
>> 	return want;
>> }
>> 
>> +static int
>> +xs_sock_process_cmsg(struct socket *sock, struct msghdr *msg,
>> +		     struct cmsghdr *cmsg, int ret)
>> +{
>> +	if (cmsg->cmsg_level == SOL_TLS &&
>> +	    cmsg->cmsg_type == TLS_GET_RECORD_TYPE) {
>> +		u8 content_type = *((u8 *)CMSG_DATA(cmsg));
>> +
>> +		trace_xs_tls_contenttype(xprt_from_sock(sock->sk), content_type);
>> +		switch (content_type) {
>> +		case TLS_RECORD_TYPE_DATA:
>> +			/* TLS sets EOR at the end of each application data
>> +			 * record, even though there might be more frames
>> +			 * waiting to be decrypted. */
>> +			msg->msg_flags &= ~MSG_EOR;
>> +			break;
>> +		case TLS_RECORD_TYPE_ALERT:
>> +			ret = -ENOTCONN;
>> +			break;
>> +		default:
>> +			ret = -EAGAIN;
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int
>> +xs_sock_recv_cmsg(struct socket *sock, struct msghdr *msg, int flags)
>> +{
>> +	union {
>> +		struct cmsghdr	cmsg;
>> +		u8		buf[CMSG_SPACE(sizeof(u8))];
>> +	} u;
>> +	int ret;
>> +
>> +	msg->msg_control = &u;
>> +	msg->msg_controllen = sizeof(u);
>> +	ret = sock_recvmsg(sock, msg, flags);
>> +	if (msg->msg_controllen != sizeof(u))
> 
> Do you also need to check for ret < 0 here? Can msg_controllen be
> trusted if sock_recvmsg returns an error?

This is not the most clear of APIs, and code I audited to help me
understand how it should work was inconsistent. Some just outright
does not work.

sock_recvmsg() has to explicitly modify the msg_controllen field
to indicate there is a control message. I think we are good here.

Also, no-one barked at this when it was posted on netdev.


>> +		ret = xs_sock_process_cmsg(sock, msg, &u.cmsg, ret);
>> +	return ret;
>> +}
>> +
>> static ssize_t
>> xs_sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags, size_t seek)
>> {
>> 	ssize_t ret;
>> 	if (seek != 0)
>> 		iov_iter_advance(&msg->msg_iter, seek);
>> -	ret = sock_recvmsg(sock, msg, flags);
>> +	ret = xs_sock_recv_cmsg(sock, msg, flags);
>> 	return ret > 0 ? ret + seek : ret;
>> }
>> 
>> @@ -382,7 +427,7 @@ xs_read_discard(struct socket *sock, struct msghdr *msg, int flags,
>> 		size_t count)
>> {
>> 	iov_iter_discard(&msg->msg_iter, READ, count);
>> -	return sock_recvmsg(sock, msg, flags);
>> +	return xs_sock_recv_cmsg(sock, msg, flags);
>> }
>> 
>> #if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever