[PATCH v2 0/7] nvme-tcp: Support receiving KeyUpdate requests

[PATCH v2 0/7] nvme-tcp: Support receiving KeyUpdate requests

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

The TLS 1.3 specification allows the TLS client or server to send a
KeyUpdate. This is generally used when the sequence is about to
overflow or after a certain amount of bytes have been encrypted.

The TLS spec doesn't mandate the conditions though, so a KeyUpdate
can be sent by the TLS client or server at any time. This includes
when running NVMe-OF over a TLS 1.3 connection.

As such Linux should be able to handle a KeyUpdate event, as the
other NVMe side could initiate a KeyUpdate.

Upcoming WD NVMe-TCP hardware controllers implement TLS support
and send KeyUpdate requests.

This series builds on top of the existing TLS EKEYEXPIRED work,
which already detects a KeyUpdate request. We can now pass that
information up to the NVMe layer (target and host) and then pass
it up to userspace.

Userspace (ktls-utils) will need to save the connection state
in the keyring during the initial handshake. The kernel then
provides the key serial back to userspace when handling a
KeyUpdate. Userspace can use this to restore the connection
information and then update the keys, this final process
is similar to the initial handshake.

Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3

v2:
 - Change "key-serial" to "session-id"
 - Fix reported build failures
 - Drop tls_clear_err() function
 - Stop keep alive timer during KeyUpdate
 - Drop handshake message decoding in the NVMe layer

Alistair Francis (7):
  net/handshake: Store the key serial number on completion
  net/handshake: Make handshake_req_cancel public
  net/handshake: Expose handshake_sk_destruct_req publically
  nvmet: Expose nvmet_stop_keep_alive_timer publically
  net/handshake: Support KeyUpdate message types
  nvme-tcp: Support KeyUpdate
  nvmet-tcp: Support KeyUpdate

 Documentation/netlink/specs/handshake.yaml |  19 +++-
 Documentation/networking/tls-handshake.rst |   4 +-
 drivers/nvme/host/tcp.c                    |  88 +++++++++++++++--
 drivers/nvme/target/core.c                 |   1 +
 drivers/nvme/target/tcp.c                  | 104 +++++++++++++++++++--
 include/net/handshake.h                    |  17 +++-
 include/uapi/linux/handshake.h             |  14 +++
 net/handshake/genl.c                       |   5 +-
 net/handshake/handshake.h                  |   1 -
 net/handshake/request.c                    |  18 ++++
 net/handshake/tlshd.c                      |  46 +++++++--
 net/sunrpc/svcsock.c                       |   3 +-
 net/sunrpc/xprtsock.c                      |   3 +-
 13 files changed, 289 insertions(+), 34 deletions(-)

-- 
2.50.1

[PATCH v2 1/7] net/handshake: Store the key serial number on completion

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Allow userspace to include a key serial number when completing a
handshake with the HANDSHAKE_CMD_DONE command.

We then store this serial number and will provide it back to userspace
in the future. This allows userspace to save data to the keyring and
then restore that data later.

This will be used to support the TLS KeyUpdate operation, as now
userspace can resume information about a established session.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Change "key-serial" to "session-id"

 Documentation/netlink/specs/handshake.yaml |  4 ++++
 drivers/nvme/host/tcp.c                    |  3 ++-
 drivers/nvme/target/tcp.c                  |  3 ++-
 include/net/handshake.h                    |  3 ++-
 include/uapi/linux/handshake.h             |  1 +
 net/handshake/genl.c                       |  5 +++--
 net/handshake/tlshd.c                      | 15 +++++++++++++--
 net/sunrpc/svcsock.c                       |  3 ++-
 net/sunrpc/xprtsock.c                      |  3 ++-
 9 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
index 95c3fade7a8d..a273bc74d26f 100644
--- a/Documentation/netlink/specs/handshake.yaml
+++ b/Documentation/netlink/specs/handshake.yaml
@@ -87,6 +87,9 @@ attribute-sets:
         name: remote-auth
         type: u32
         multi-attr: true
+      -
+        name: session-id
+        type: u32
 
 operations:
   list:
@@ -123,6 +126,7 @@ operations:
             - status
             - sockfd
             - remote-auth
+            - session-id
 
 mcast-groups:
   list:
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index c0fe8cfb7229..2700ff3b8e85 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1673,7 +1673,8 @@ static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
 		qid, queue->io_cpu);
 }
 
-static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
+static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid,
+	key_serial_t user_session_id)
 {
 	struct nvme_tcp_queue *queue = data;
 	struct nvme_tcp_ctrl *ctrl = queue->ctrl;
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 470bf37e5a63..4ef4dd140ada 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1780,7 +1780,8 @@ static int nvmet_tcp_tls_key_lookup(struct nvmet_tcp_queue *queue,
 }
 
 static void nvmet_tcp_tls_handshake_done(void *data, int status,
-					 key_serial_t peerid)
+					 key_serial_t peerid,
+					 key_serial_t user_session_id)
 {
 	struct nvmet_tcp_queue *queue = data;
 
diff --git a/include/net/handshake.h b/include/net/handshake.h
index 8ebd4f9ed26e..a07fecea87eb 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -18,7 +18,8 @@ enum {
 };
 
 typedef void	(*tls_done_func_t)(void *data, int status,
-				   key_serial_t peerid);
+				   key_serial_t peerid,
+				   key_serial_t user_session_id);
 
 struct tls_handshake_args {
 	struct socket		*ta_sock;
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index 662e7de46c54..b68ffbaa5f31 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -55,6 +55,7 @@ enum {
 	HANDSHAKE_A_DONE_STATUS = 1,
 	HANDSHAKE_A_DONE_SOCKFD,
 	HANDSHAKE_A_DONE_REMOTE_AUTH,
+	HANDSHAKE_A_DONE_SESSION_ID,
 
 	__HANDSHAKE_A_DONE_MAX,
 	HANDSHAKE_A_DONE_MAX = (__HANDSHAKE_A_DONE_MAX - 1)
diff --git a/net/handshake/genl.c b/net/handshake/genl.c
index f55d14d7b726..6cdce7e5dbc0 100644
--- a/net/handshake/genl.c
+++ b/net/handshake/genl.c
@@ -16,10 +16,11 @@ static const struct nla_policy handshake_accept_nl_policy[HANDSHAKE_A_ACCEPT_HAN
 };
 
 /* HANDSHAKE_CMD_DONE - do */
-static const struct nla_policy handshake_done_nl_policy[HANDSHAKE_A_DONE_REMOTE_AUTH + 1] = {
+static const struct nla_policy handshake_done_nl_policy[HANDSHAKE_A_DONE_SESSION_ID + 1] = {
 	[HANDSHAKE_A_DONE_STATUS] = { .type = NLA_U32, },
 	[HANDSHAKE_A_DONE_SOCKFD] = { .type = NLA_S32, },
 	[HANDSHAKE_A_DONE_REMOTE_AUTH] = { .type = NLA_U32, },
+	[HANDSHAKE_A_DONE_SESSION_ID] = { .type = NLA_U32, },
 };
 
 /* Ops table for handshake */
@@ -35,7 +36,7 @@ static const struct genl_split_ops handshake_nl_ops[] = {
 		.cmd		= HANDSHAKE_CMD_DONE,
 		.doit		= handshake_nl_done_doit,
 		.policy		= handshake_done_nl_policy,
-		.maxattr	= HANDSHAKE_A_DONE_REMOTE_AUTH,
+		.maxattr	= HANDSHAKE_A_DONE_SESSION_ID,
 		.flags		= GENL_CMD_CAP_DO,
 	},
 };
diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c
index 081093dfd553..f78c3edd5e09 100644
--- a/net/handshake/tlshd.c
+++ b/net/handshake/tlshd.c
@@ -26,7 +26,8 @@
 
 struct tls_handshake_req {
 	void			(*th_consumer_done)(void *data, int status,
-						    key_serial_t peerid);
+						    key_serial_t peerid,
+						    key_serial_t user_session_id);
 	void			*th_consumer_data;
 
 	int			th_type;
@@ -39,6 +40,8 @@ struct tls_handshake_req {
 
 	unsigned int		th_num_peerids;
 	key_serial_t		th_peerid[5];
+
+	key_serial_t		user_key_serial;
 };
 
 static struct tls_handshake_req *
@@ -55,6 +58,7 @@ tls_handshake_req_init(struct handshake_req *req,
 	treq->th_num_peerids = 0;
 	treq->th_certificate = TLS_NO_CERT;
 	treq->th_privkey = TLS_NO_PRIVKEY;
+	treq->user_key_serial = TLS_NO_PRIVKEY;
 	return treq;
 }
 
@@ -83,6 +87,13 @@ static void tls_handshake_remote_peerids(struct tls_handshake_req *treq,
 		if (i >= treq->th_num_peerids)
 			break;
 	}
+
+	nla_for_each_attr(nla, head, len, rem) {
+		if (nla_type(nla) == HANDSHAKE_A_DONE_SESSION_ID) {
+			treq->user_session_id = nla_get_u32(nla);
+			break;
+		}
+	}
 }
 
 /**
@@ -105,7 +116,7 @@ static void tls_handshake_done(struct handshake_req *req,
 		set_bit(HANDSHAKE_F_REQ_SESSION, &req->hr_flags);
 
 	treq->th_consumer_done(treq->th_consumer_data, -status,
-			       treq->th_peerid[0]);
+			       treq->th_peerid[0], treq->user_session_id);
 }
 
 #if IS_ENABLED(CONFIG_KEYS)
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index e2c5e0e626f9..1d5829aecf45 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -450,7 +450,8 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
  * is present" flag on the xprt and let an upper layer enforce local
  * security policy.
  */
-static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid)
+static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid,
+				   key_serial_t user_session_id)
 {
 	struct svc_xprt *xprt = data;
 	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index c5f7bbf5775f..3489c4693ff4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2591,7 +2591,8 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
  * @peerid: serial number of key containing the remote's identity
  *
  */
-static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid)
+static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid,
+				  key_serial_t user_session_id)
 {
 	struct rpc_xprt *lower_xprt = data;
 	struct sock_xprt *lower_transport =
-- 
2.50.1

[PATCH v2 2/7] net/handshake: Make handshake_req_cancel public

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

As part of supporting KeyUpdate we are going to want to call
handshake_req_cancel() to cancel an existing handshake in order to
instead start a KeyUpdate request.

This is required to avoid hash conflicts when handshake_req_hash_add()
is called as part of submitting the KeyUpdate request.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Fix build failures

 include/net/handshake.h   | 2 ++
 net/handshake/handshake.h | 1 -
 net/handshake/request.c   | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/net/handshake.h b/include/net/handshake.h
index a07fecea87eb..10f301f3c660 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -43,6 +43,8 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
 bool tls_handshake_cancel(struct sock *sk);
 void tls_handshake_close(struct socket *sock);
 
+bool handshake_req_cancel(struct sock *sk);
+
 u8 tls_get_record_type(const struct sock *sk, const struct cmsghdr *msg);
 void tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
 		    u8 *level, u8 *description);
diff --git a/net/handshake/handshake.h b/net/handshake/handshake.h
index a48163765a7a..55c25eaba0f4 100644
--- a/net/handshake/handshake.h
+++ b/net/handshake/handshake.h
@@ -88,6 +88,5 @@ int handshake_req_submit(struct socket *sock, struct handshake_req *req,
 			 gfp_t flags);
 void handshake_complete(struct handshake_req *req, unsigned int status,
 			struct genl_info *info);
-bool handshake_req_cancel(struct sock *sk);
 
 #endif /* _INTERNAL_HANDSHAKE_H */
diff --git a/net/handshake/request.c b/net/handshake/request.c
index 274d2c89b6b2..02269f212c70 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -17,6 +17,7 @@
 
 #include <net/sock.h>
 #include <net/genetlink.h>
+#include <net/handshake.h>
 #include <net/netns/generic.h>
 
 #include <kunit/visibility.h>
-- 
2.50.1

[PATCH v2 3/7] net/handshake: Expose handshake_sk_destruct_req publically

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Define a `handshake_sk_destruct_req()` function and expose it publically
so that other subsystems can destruct the handshake req.

This will be used as part of the KeyUpdate to ensure any existing
requests anre cancelled and destructed if required.

This is required to avoid hash conflicts when handshake_req_hash_add()
is called as part of submitting the KeyUpdate request.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Fix build failures

 include/net/handshake.h |  1 +
 net/handshake/request.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/net/handshake.h b/include/net/handshake.h
index 10f301f3c660..89dc169eae89 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -44,6 +44,7 @@ bool tls_handshake_cancel(struct sock *sk);
 void tls_handshake_close(struct socket *sock);
 
 bool handshake_req_cancel(struct sock *sk);
+void handshake_sk_destruct_req(struct sock *sk);
 
 u8 tls_get_record_type(const struct sock *sk, const struct cmsghdr *msg);
 void tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
diff --git a/net/handshake/request.c b/net/handshake/request.c
index 02269f212c70..bb61c9a1a03d 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -342,3 +342,20 @@ bool handshake_req_cancel(struct sock *sk)
 	return true;
 }
 EXPORT_SYMBOL(handshake_req_cancel);
+
+/**
+ * handshake_sk_destruct_req - destroy an existing request
+ * @sk: socket on which there is an existing request
+ */
+void handshake_sk_destruct_req(struct sock *sk)
+{
+	struct handshake_req *req;
+
+	req = handshake_req_hash_lookup(sk);
+	if (!req)
+		return;
+
+	trace_handshake_destruct(sock_net(sk), req, sk);
+	handshake_req_destroy(req);
+}
+EXPORT_SYMBOL(handshake_sk_destruct_req);
-- 
2.50.1

[PATCH v2 4/7] nvmet: Expose nvmet_stop_keep_alive_timer publically

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 drivers/nvme/target/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 0dd7bd99afa3..bed1c6ebe83a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -430,6 +430,7 @@ void nvmet_stop_keep_alive_timer(struct nvmet_ctrl *ctrl)
 
 	cancel_delayed_work_sync(&ctrl->ka_work);
 }
+EXPORT_SYMBOL_GPL(nvmet_stop_keep_alive_timer);
 
 u16 nvmet_req_find_ns(struct nvmet_req *req)
 {
-- 
2.50.1

[PATCH v2 5/7] net/handshake: Support KeyUpdate message types

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

When reporting the msg-type to userspace let's also support reporting
KeyUpdate events. This supports reporting a client/server event and if
the other side requested a KeyUpdateRequest.

Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 Documentation/netlink/specs/handshake.yaml | 15 +++++++++-
 Documentation/networking/tls-handshake.rst |  4 +--
 drivers/nvme/host/tcp.c                    | 12 ++++++--
 drivers/nvme/target/tcp.c                  | 11 +++++--
 include/net/handshake.h                    | 11 +++++--
 include/uapi/linux/handshake.h             | 13 ++++++++
 net/handshake/tlshd.c                      | 35 ++++++++++++++++++----
 7 files changed, 84 insertions(+), 17 deletions(-)

diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
index a273bc74d26f..1a3312fad410 100644
--- a/Documentation/netlink/specs/handshake.yaml
+++ b/Documentation/netlink/specs/handshake.yaml
@@ -21,12 +21,17 @@ definitions:
     type: enum
     name: msg-type
     value-start: 0
-    entries: [unspec, clienthello, serverhello]
+    entries: [unspec, clienthello, serverhello, clientkeyupdate, clientkeyupdaterequest, serverkeyupdate, serverkeyupdaterequest]
   -
     type: enum
     name: auth
     value-start: 0
     entries: [unspec, unauth, psk, x509]
+  -
+    type: enum
+    name: key-update-type
+    value-start: 0
+    entries: [unspec, send, received, received_request_update]
 
 attribute-sets:
   -
@@ -74,6 +79,13 @@ attribute-sets:
       -
         name: keyring
         type: u32
+      -
+        name: key-update-request
+        type: u32
+        enum: key-update-type
+      -
+        name: key-serial
+        type: u32
   -
     name: done
     attributes:
@@ -116,6 +128,7 @@ operations:
             - certificate
             - peername
             - keyring
+            - key-serial
     -
       name: done
       doc: Handler reports handshake completion
diff --git a/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
index 6f5ea1646a47..64a70847bd8b 100644
--- a/Documentation/networking/tls-handshake.rst
+++ b/Documentation/networking/tls-handshake.rst
@@ -108,7 +108,7 @@ To initiate a client-side TLS handshake with a pre-shared key, use:
 
 .. code-block:: c
 
-  ret = tls_client_hello_psk(args, gfp_flags);
+  ret = tls_client_hello_psk(args, gfp_flags, handshake_key_update_type);
 
 However, in this case, the consumer fills in the @ta_my_peerids array
 with serial numbers of keys containing the peer identities it wishes
@@ -138,7 +138,7 @@ or
 
 .. code-block:: c
 
-  ret = tls_server_hello_psk(args, gfp_flags);
+  ret = tls_server_hello_psk(args, gfp_flags, handshake_key_update_type);
 
 The argument structure is filled in as above.
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2700ff3b8e85..776047a71436 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -19,6 +19,7 @@
 #include <linux/blk-mq.h>
 #include <net/busy_poll.h>
 #include <trace/events/sock.h>
+#include <uapi/linux/handshake.h>
 
 #include "nvme.h"
 #include "fabrics.h"
@@ -205,6 +206,10 @@ static struct workqueue_struct *nvme_tcp_wq;
 static const struct blk_mq_ops nvme_tcp_mq_ops;
 static const struct blk_mq_ops nvme_tcp_admin_mq_ops;
 static int nvme_tcp_try_send(struct nvme_tcp_queue *queue);
+static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
+			      struct nvme_tcp_queue *queue,
+			      key_serial_t pskid,
+			      handshake_key_update_type keyupdate);
 
 static inline struct nvme_tcp_ctrl *to_tcp_ctrl(struct nvme_ctrl *ctrl)
 {
@@ -1708,7 +1713,8 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid,
 
 static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 			      struct nvme_tcp_queue *queue,
-			      key_serial_t pskid)
+			      key_serial_t pskid,
+			      handshake_key_update_type keyupdate)
 {
 	int qid = nvme_tcp_queue_id(queue);
 	int ret;
@@ -1730,7 +1736,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 	args.ta_timeout_ms = tls_handshake_timeout * 1000;
 	queue->tls_err = -EOPNOTSUPP;
 	init_completion(&queue->tls_complete);
-	ret = tls_client_hello_psk(&args, GFP_KERNEL);
+	ret = tls_client_hello_psk(&args, GFP_KERNEL, keyupdate);
 	if (ret) {
 		dev_err(nctrl->device, "queue %d: failed to start TLS: %d\n",
 			qid, ret);
@@ -1880,7 +1886,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 
 	/* If PSKs are configured try to start TLS */
 	if (nvme_tcp_tls_configured(nctrl) && pskid) {
-		ret = nvme_tcp_start_tls(nctrl, queue, pskid);
+		ret = nvme_tcp_start_tls(nctrl, queue, pskid, HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC);
 		if (ret)
 			goto err_init_connect;
 	}
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 4ef4dd140ada..bee0355195f5 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -214,6 +214,10 @@ static struct workqueue_struct *nvmet_tcp_wq;
 static const struct nvmet_fabrics_ops nvmet_tcp_ops;
 static void nvmet_tcp_free_cmd(struct nvmet_tcp_cmd *c);
 static void nvmet_tcp_free_cmd_buffers(struct nvmet_tcp_cmd *cmd);
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue,
+				   handshake_key_update_type keyupdate);
+#endif
 
 static inline u16 nvmet_tcp_cmd_tag(struct nvmet_tcp_queue *queue,
 		struct nvmet_tcp_cmd *cmd)
@@ -1833,7 +1837,8 @@ static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w)
 	kref_put(&queue->kref, nvmet_tcp_release_queue);
 }
 
-static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue)
+static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue,
+	handshake_key_update_type keyupdate)
 {
 	int ret = -EOPNOTSUPP;
 	struct tls_handshake_args args;
@@ -1852,7 +1857,7 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue)
 	args.ta_keyring = key_serial(queue->port->nport->keyring);
 	args.ta_timeout_ms = tls_handshake_timeout * 1000;
 
-	ret = tls_server_hello_psk(&args, GFP_KERNEL);
+	ret = tls_server_hello_psk(&args, GFP_KERNEL, keyupdate);
 	if (ret) {
 		kref_put(&queue->kref, nvmet_tcp_release_queue);
 		pr_err("failed to start TLS, err=%d\n", ret);
@@ -1934,7 +1939,7 @@ static void nvmet_tcp_alloc_queue(struct nvmet_tcp_port *port,
 		sk->sk_data_ready = port->data_ready;
 		write_unlock_bh(&sk->sk_callback_lock);
 		if (!nvmet_tcp_try_peek_pdu(queue)) {
-			if (!nvmet_tcp_tls_handshake(queue))
+			if (!nvmet_tcp_tls_handshake(queue, HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC))
 				return;
 			/* TLS handshake failed, terminate the connection */
 			goto out_destroy_sq;
diff --git a/include/net/handshake.h b/include/net/handshake.h
index 89dc169eae89..52036993b2b8 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -10,6 +10,10 @@
 #ifndef _NET_HANDSHAKE_H
 #define _NET_HANDSHAKE_H
 
+#include <uapi/linux/handshake.h>
+
+#define handshake_key_update_type u32
+
 enum {
 	TLS_NO_KEYRING = 0,
 	TLS_NO_PEERID = 0,
@@ -32,13 +36,16 @@ struct tls_handshake_args {
 	key_serial_t		ta_my_privkey;
 	unsigned int		ta_num_peerids;
 	key_serial_t		ta_my_peerids[5];
+	key_serial_t		user_session_id;
 };
 
 int tls_client_hello_anon(const struct tls_handshake_args *args, gfp_t flags);
 int tls_client_hello_x509(const struct tls_handshake_args *args, gfp_t flags);
-int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
+int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
+			 handshake_key_update_type keyupdate);
 int tls_server_hello_x509(const struct tls_handshake_args *args, gfp_t flags);
-int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
+int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
+			 handshake_key_update_type keyupdate);
 
 bool tls_handshake_cancel(struct sock *sk);
 void tls_handshake_close(struct socket *sock);
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index b68ffbaa5f31..b691530073c6 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -19,6 +19,10 @@ enum handshake_msg_type {
 	HANDSHAKE_MSG_TYPE_UNSPEC,
 	HANDSHAKE_MSG_TYPE_CLIENTHELLO,
 	HANDSHAKE_MSG_TYPE_SERVERHELLO,
+	HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE,
+	HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATEREQUEST,
+	HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE,
+	HANDSHAKE_MSG_TYPE_SERVERKEYUPDATEREQUEST,
 };
 
 enum handshake_auth {
@@ -28,6 +32,13 @@ enum handshake_auth {
 	HANDSHAKE_AUTH_X509,
 };
 
+enum handshake_key_update_type {
+	HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC,
+	HANDSHAKE_KEY_UPDATE_TYPE_SEND,
+	HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED,
+	HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED_REQUEST_UPDATE,
+};
+
 enum {
 	HANDSHAKE_A_X509_CERT = 1,
 	HANDSHAKE_A_X509_PRIVKEY,
@@ -46,6 +57,8 @@ enum {
 	HANDSHAKE_A_ACCEPT_CERTIFICATE,
 	HANDSHAKE_A_ACCEPT_PEERNAME,
 	HANDSHAKE_A_ACCEPT_KEYRING,
+	HANDSHAKE_A_ACCEPT_KEY_UPDATE_REQUEST,
+	HANDSHAKE_A_ACCEPT_KEY_SERIAL,
 
 	__HANDSHAKE_A_ACCEPT_MAX,
 	HANDSHAKE_A_ACCEPT_MAX = (__HANDSHAKE_A_ACCEPT_MAX - 1)
diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c
index f78c3edd5e09..ebdf32c67f37 100644
--- a/net/handshake/tlshd.c
+++ b/net/handshake/tlshd.c
@@ -41,7 +41,9 @@ struct tls_handshake_req {
 	unsigned int		th_num_peerids;
 	key_serial_t		th_peerid[5];
 
-	key_serial_t		user_key_serial;
+	int			th_key_update_request;
+
+	key_serial_t		user_session_id;
 };
 
 static struct tls_handshake_req *
@@ -58,7 +60,8 @@ tls_handshake_req_init(struct handshake_req *req,
 	treq->th_num_peerids = 0;
 	treq->th_certificate = TLS_NO_CERT;
 	treq->th_privkey = TLS_NO_PRIVKEY;
-	treq->user_key_serial = TLS_NO_PRIVKEY;
+	treq->user_session_id = args->user_session_id;
+
 	return treq;
 }
 
@@ -265,6 +268,16 @@ static int tls_handshake_accept(struct handshake_req *req,
 		break;
 	}
 
+	ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_KEY_SERIAL,
+			  treq->user_session_id);
+	if (ret < 0)
+		goto out_cancel;
+
+	ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_KEY_UPDATE_REQUEST,
+			  treq->th_key_update_request);
+	if (ret < 0)
+		goto out_cancel;
+
 	genlmsg_end(msg, hdr);
 	return genlmsg_reply(msg, info);
 
@@ -348,7 +361,8 @@ EXPORT_SYMBOL(tls_client_hello_x509);
  *   %-ESRCH: No user agent is available
  *   %-ENOMEM: Memory allocation failed
  */
-int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
+int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
+			 handshake_key_update_type keyupdate)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -362,7 +376,11 @@ int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
 	if (!req)
 		return -ENOMEM;
 	treq = tls_handshake_req_init(req, args);
-	treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTHELLO;
+	if (keyupdate != HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
+		treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
+	else
+		treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTHELLO;
+	treq->th_key_update_request = keyupdate;
 	treq->th_auth_mode = HANDSHAKE_AUTH_PSK;
 	treq->th_num_peerids = args->ta_num_peerids;
 	for (i = 0; i < args->ta_num_peerids; i++)
@@ -410,7 +428,8 @@ EXPORT_SYMBOL(tls_server_hello_x509);
  *   %-ESRCH: No user agent is available
  *   %-ENOMEM: Memory allocation failed
  */
-int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
+int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
+			 handshake_key_update_type keyupdate)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -419,7 +438,11 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
 	if (!req)
 		return -ENOMEM;
 	treq = tls_handshake_req_init(req, args);
-	treq->th_type = HANDSHAKE_MSG_TYPE_SERVERHELLO;
+	if (keyupdate != HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
+		treq->th_type = HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE;
+	else
+		treq->th_type = HANDSHAKE_MSG_TYPE_SERVERHELLO;
+	treq->th_key_update_request = keyupdate;
 	treq->th_auth_mode = HANDSHAKE_AUTH_PSK;
 	treq->th_num_peerids = 1;
 	treq->th_peerid[0] = args->ta_my_peerids[0];
-- 
2.50.1

[PATCH v2 6/7] nvme-tcp: Support KeyUpdate

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

If the nvme_tcp_try_send() or nvme_tcp_try_recv() functions return
EKEYEXPIRED then the underlying TLS keys need to be updated. This occurs
on an KeyUpdate event.

If the NVMe Target (TLS server) initiates a KeyUpdate this patch will
allow the NVMe layer to process the KeyUpdate request and forward the
request to userspace. Userspace must then update the key to keep the
connection alive.

This patch allows us to handle the NVMe target sending a KeyUpdate
request without aborting the connection. At this time we don't support
initiating a KeyUpdate.

Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Don't change the state
 - Use a helper function for KeyUpdates
 - Continue sending in nvme_tcp_send_all() after a KeyUpdate
 - Remove command message using recvmsg
 
 drivers/nvme/host/tcp.c | 73 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 776047a71436..b6449effc2ac 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -171,6 +171,7 @@ struct nvme_tcp_queue {
 	bool			tls_enabled;
 	u32			rcv_crc;
 	u32			snd_crc;
+	key_serial_t		user_session_id;
 	__le32			exp_ddgst;
 	__le32			recv_ddgst;
 	struct completion       tls_complete;
@@ -210,6 +211,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 			      struct nvme_tcp_queue *queue,
 			      key_serial_t pskid,
 			      handshake_key_update_type keyupdate);
+static void update_tls_keys(struct nvme_tcp_queue *queue);
 
 static inline struct nvme_tcp_ctrl *to_tcp_ctrl(struct nvme_ctrl *ctrl)
 {
@@ -393,6 +395,14 @@ static inline void nvme_tcp_send_all(struct nvme_tcp_queue *queue)
 	do {
 		ret = nvme_tcp_try_send(queue);
 	} while (ret > 0);
+
+	if (ret == -EKEYEXPIRED) {
+		update_tls_keys(queue);
+
+		do {
+			ret = nvme_tcp_try_send(queue);
+		} while (ret > 0);
+	}
 }
 
 static inline bool nvme_tcp_queue_has_pending(struct nvme_tcp_queue *queue)
@@ -1347,6 +1357,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
 done:
 	if (ret == -EAGAIN) {
 		ret = 0;
+	} else if (ret == -EKEYEXPIRED) {
+		goto out;
 	} else if (ret < 0) {
 		dev_err(queue->ctrl->ctrl.device,
 			"failed to send request %d\n", ret);
@@ -1371,9 +1383,56 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
 	queue->nr_cqe = 0;
 	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
 	release_sock(sk);
+
+	/* If we received EINVAL from read_sock then it generally means the
+	 * other side sent a command message. So let's try to clear it from
+	 * our queue with a recvmsg, otherwise we get stuck in an infinite
+	 * loop.
+	 */
+	if (consumed == -EINVAL) {
+		char cbuf[CMSG_LEN(sizeof(char))] = {};
+		struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
+		struct bio_vec bvec;
+
+		bvec_set_virt(&bvec, (void *)cbuf, sizeof(cbuf));
+		iov_iter_bvec(&msg.msg_iter, ITER_DEST, &bvec, 1, sizeof(cbuf));
+
+		msg.msg_control = cbuf;
+		msg.msg_controllen = sizeof(cbuf);
+
+		consumed = sock_recvmsg(sock, &msg, msg.msg_flags);
+	}
+
 	return consumed == -EAGAIN ? 0 : consumed;
 }
 
+static void update_tls_keys(struct nvme_tcp_queue *queue)
+{
+	int qid = nvme_tcp_queue_id(queue);
+	int ret;
+
+	dev_dbg(queue->ctrl->ctrl.device,
+		"updating key for queue %d\n", qid);
+
+	cancel_work(&queue->io_work);
+	handshake_req_cancel(queue->sock->sk);
+	handshake_sk_destruct_req(queue->sock->sk);
+
+	nvme_stop_keep_alive(&(queue->ctrl->ctrl));
+	flush_work(&(queue->ctrl->ctrl).async_event_work);
+
+	ret = nvme_tcp_start_tls(&(queue->ctrl->ctrl),
+				 queue, queue->ctrl->ctrl.tls_pskid,
+				 HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
+
+	if (ret < 0) {
+		dev_err(queue->ctrl->ctrl.device,
+			"failed to update the keys %d\n", ret);
+		nvme_tcp_fail_request(queue->request);
+		nvme_tcp_done_send_req(queue);
+	}
+}
+
 static void nvme_tcp_io_work(struct work_struct *w)
 {
 	struct nvme_tcp_queue *queue =
@@ -1389,15 +1448,21 @@ static void nvme_tcp_io_work(struct work_struct *w)
 			mutex_unlock(&queue->send_mutex);
 			if (result > 0)
 				pending = true;
-			else if (unlikely(result < 0))
+			else if (unlikely(result < 0)) {
+				if (result == -EKEYEXPIRED)
+					update_tls_keys(queue);
 				break;
+			}
 		}
 
 		result = nvme_tcp_try_recv(queue);
 		if (result > 0)
 			pending = true;
-		else if (unlikely(result < 0))
-			return;
+		else if (unlikely(result < 0)) {
+			if (result == -EKEYEXPIRED)
+				update_tls_keys(queue);
+			break;
+		}
 
 		/* did we get some space after spending time in recv? */
 		if (nvme_tcp_queue_has_pending(queue) &&
@@ -1705,6 +1770,7 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid,
 			ctrl->ctrl.tls_pskid = key_serial(tls_key);
 		key_put(tls_key);
 		queue->tls_err = 0;
+		queue->user_session_id = user_session_id;
 	}
 
 out_complete:
@@ -1734,6 +1800,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 		keyring = key_serial(nctrl->opts->keyring);
 	args.ta_keyring = keyring;
 	args.ta_timeout_ms = tls_handshake_timeout * 1000;
+	args.user_session_id = queue->user_session_id;
 	queue->tls_err = -EOPNOTSUPP;
 	init_completion(&queue->tls_complete);
 	ret = tls_client_hello_psk(&args, GFP_KERNEL, keyupdate);
-- 
2.50.1

[PATCH v2 7/7] nvmet-tcp: Support KeyUpdate

[alistair23]

From: Alistair Francis <alistair.francis@wdc.com>

If the nvmet_tcp_try_recv() function return EKEYEXPIRED or if we receive
a KeyUpdate handshake type then the underlying TLS keys need to be
updated.

If the NVMe Host (TLS client) initiates a KeyUpdate this patch will
allow the NVMe layer to process the KeyUpdate request and forward the
request to userspace. Userspace must then update the key to keep the
connection alive.

This patch allows us to handle the NVMe host sending a KeyUpdate
request without aborting the connection. At this time we don't support
initiating a KeyUpdate.

Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Use a helper function for KeyUpdates
 - Ensure keep alive timer is stopped
 - Wait for TLS KeyUpdate to complete

 drivers/nvme/target/tcp.c | 90 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index bee0355195f5..dd09940e9635 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -175,6 +175,7 @@ struct nvmet_tcp_queue {
 
 	/* TLS state */
 	key_serial_t		tls_pskid;
+	key_serial_t		user_session_id;
 	struct delayed_work	tls_handshake_tmo_work;
 
 	unsigned long           poll_end;
@@ -186,6 +187,8 @@ struct nvmet_tcp_queue {
 	struct sockaddr_storage	sockaddr_peer;
 	struct work_struct	release_work;
 
+	struct completion       tls_complete;
+
 	int			idx;
 	struct list_head	queue_list;
 
@@ -836,6 +839,11 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
 	return 1;
 }
 
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static int nvmet_tcp_try_peek_pdu(struct nvmet_tcp_queue *queue);
+static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w);
+#endif
+
 static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
 		int budget, int *sends)
 {
@@ -844,6 +852,13 @@ static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
 	for (i = 0; i < budget; i++) {
 		ret = nvmet_tcp_try_send_one(queue, i == budget - 1);
 		if (unlikely(ret < 0)) {
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+			if (ret == -EKEYEXPIRED &&
+				queue->state != NVMET_TCP_Q_DISCONNECTING &&
+				queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
+					goto done;
+			}
+#endif
 			nvmet_tcp_socket_error(queue, ret);
 			goto done;
 		} else if (ret == 0) {
@@ -1110,11 +1125,52 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
 	return false;
 }
 
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static int update_tls_keys(struct nvmet_tcp_queue *queue)
+{
+	int ret;
+
+	cancel_work(&queue->io_work);
+	handshake_req_cancel(queue->sock->sk);
+	handshake_sk_destruct_req(queue->sock->sk);
+	queue->state = NVMET_TCP_Q_TLS_HANDSHAKE;
+
+	/* Restore the default callbacks before starting upcall */
+	read_lock_bh(&queue->sock->sk->sk_callback_lock);
+	queue->sock->sk->sk_data_ready =  queue->data_ready;
+	queue->sock->sk->sk_state_change = queue->state_change;
+	queue->sock->sk->sk_write_space = queue->write_space;
+	queue->sock->sk->sk_user_data = NULL;
+	read_unlock_bh(&queue->sock->sk->sk_callback_lock);
+
+	nvmet_stop_keep_alive_timer(queue->nvme_sq.ctrl);
+
+	INIT_DELAYED_WORK(&queue->tls_handshake_tmo_work,
+			  nvmet_tcp_tls_handshake_timeout);
+
+	ret = nvmet_tcp_tls_handshake(queue, HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
+
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_interruptible_timeout(&queue->tls_complete, 10 * HZ);
+
+	if (ret <= 0) {
+		tls_handshake_cancel(queue->sock->sk);
+		return ret;
+	}
+
+	queue->state = NVMET_TCP_Q_LIVE;
+
+	return ret;
+}
+#endif
+
 static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
 		struct msghdr *msg, char *cbuf)
 {
 	struct cmsghdr *cmsg = (struct cmsghdr *)cbuf;
-	u8 ctype, level, description;
+	u8 ctype, htype, level, description;
 	int ret = 0;
 
 	ctype = tls_get_record_type(queue->sock->sk, cmsg);
@@ -1135,6 +1191,9 @@ static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
 			ret = -EAGAIN;
 		}
 		break;
+	case TLS_RECORD_TYPE_HANDSHAKE:
+		ret = -EAGAIN;
+		break;
 	default:
 		/* discard this record type */
 		pr_err("queue %d: TLS record %d unhandled\n",
@@ -1344,6 +1403,13 @@ static int nvmet_tcp_try_recv(struct nvmet_tcp_queue *queue,
 	for (i = 0; i < budget; i++) {
 		ret = nvmet_tcp_try_recv_one(queue);
 		if (unlikely(ret < 0)) {
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+			if (ret == -EKEYEXPIRED &&
+				queue->state != NVMET_TCP_Q_DISCONNECTING &&
+				queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
+					goto done;
+			}
+#endif
 			nvmet_tcp_socket_error(queue, ret);
 			goto done;
 		} else if (ret == 0) {
@@ -1408,14 +1474,22 @@ static void nvmet_tcp_io_work(struct work_struct *w)
 		ret = nvmet_tcp_try_recv(queue, NVMET_TCP_RECV_BUDGET, &ops);
 		if (ret > 0)
 			pending = true;
-		else if (ret < 0)
-			return;
+		else if (ret < 0) {
+			if (ret == -EKEYEXPIRED)
+				update_tls_keys(queue);
+			else
+				return;
+		}
 
 		ret = nvmet_tcp_try_send(queue, NVMET_TCP_SEND_BUDGET, &ops);
 		if (ret > 0)
 			pending = true;
-		else if (ret < 0)
-			return;
+		else if (ret < 0) {
+			if (ret == -EKEYEXPIRED)
+				update_tls_keys(queue);
+			else
+				return;
+		}
 
 	} while (pending && ops < NVMET_TCP_IO_WORK_BUDGET);
 
@@ -1798,6 +1872,7 @@ static void nvmet_tcp_tls_handshake_done(void *data, int status,
 	}
 	if (!status) {
 		queue->tls_pskid = peerid;
+		queue->user_session_id = user_session_id;
 		queue->state = NVMET_TCP_Q_CONNECTING;
 	} else
 		queue->state = NVMET_TCP_Q_FAILED;
@@ -1813,6 +1888,7 @@ static void nvmet_tcp_tls_handshake_done(void *data, int status,
 	else
 		nvmet_tcp_set_queue_sock(queue);
 	kref_put(&queue->kref, nvmet_tcp_release_queue);
+	complete(&queue->tls_complete);
 }
 
 static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w)
@@ -1843,7 +1919,7 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue,
 	int ret = -EOPNOTSUPP;
 	struct tls_handshake_args args;
 
-	if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
+	if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE && !keyupdate) {
 		pr_warn("cannot start TLS in state %d\n", queue->state);
 		return -EINVAL;
 	}
@@ -1856,7 +1932,9 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue,
 	args.ta_data = queue;
 	args.ta_keyring = key_serial(queue->port->nport->keyring);
 	args.ta_timeout_ms = tls_handshake_timeout * 1000;
+	args.user_session_id = queue->user_session_id;
 
+	init_completion(&queue->tls_complete);
 	ret = tls_server_hello_psk(&args, GFP_KERNEL, keyupdate);
 	if (ret) {
 		kref_put(&queue->kref, nvmet_tcp_release_queue);
-- 
2.50.1

Re: [PATCH v2 7/7] nvmet-tcp: Support KeyUpdate

[Maurizio Lombardi]

On Fri Sep 5, 2025 at 4:46 AM CEST, alistair23 wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> If the nvmet_tcp_try_recv() function return EKEYEXPIRED or if we receive
> a KeyUpdate handshake type then the underlying TLS keys need to be
> updated.
>
> If the NVMe Host (TLS client) initiates a KeyUpdate this patch will
> allow the NVMe layer to process the KeyUpdate request and forward the
> request to userspace. Userspace must then update the key to keep the
> connection alive.
>
> This patch allows us to handle the NVMe host sending a KeyUpdate
> request without aborting the connection. At this time we don't support
> initiating a KeyUpdate.
>
> Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
>  - Use a helper function for KeyUpdates
>  - Ensure keep alive timer is stopped
>  - Wait for TLS KeyUpdate to complete
>
>  drivers/nvme/target/tcp.c | 90 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 84 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index bee0355195f5..dd09940e9635 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -175,6 +175,7 @@ struct nvmet_tcp_queue {
>  
>  	/* TLS state */
>  	key_serial_t		tls_pskid;
> +	key_serial_t		user_session_id;
>  	struct delayed_work	tls_handshake_tmo_work;
>  
>  	unsigned long           poll_end;
> @@ -186,6 +187,8 @@ struct nvmet_tcp_queue {
>  	struct sockaddr_storage	sockaddr_peer;
>  	struct work_struct	release_work;
>  
> +	struct completion       tls_complete;
> +
>  	int			idx;
>  	struct list_head	queue_list;
>  
> @@ -836,6 +839,11 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
>  	return 1;
>  }
>  
> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> +static int nvmet_tcp_try_peek_pdu(struct nvmet_tcp_queue *queue);
> +static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w);
> +#endif
> +
>  static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
>  		int budget, int *sends)
>  {
> @@ -844,6 +852,13 @@ static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
>  	for (i = 0; i < budget; i++) {
>  		ret = nvmet_tcp_try_send_one(queue, i == budget - 1);
>  		if (unlikely(ret < 0)) {
> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> +			if (ret == -EKEYEXPIRED &&
> +				queue->state != NVMET_TCP_Q_DISCONNECTING &&
> +				queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
> +					goto done;
> +			}
> +#endif
>  			nvmet_tcp_socket_error(queue, ret);
>  			goto done;
>  		} else if (ret == 0) {
> @@ -1110,11 +1125,52 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
>  	return false;
>  }
>  
> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> +static int update_tls_keys(struct nvmet_tcp_queue *queue)
> +{
> +	int ret;
> +
> +	cancel_work(&queue->io_work);
> +	handshake_req_cancel(queue->sock->sk);
> +	handshake_sk_destruct_req(queue->sock->sk);
> +	queue->state = NVMET_TCP_Q_TLS_HANDSHAKE;
> +
> +	/* Restore the default callbacks before starting upcall */
> +	read_lock_bh(&queue->sock->sk->sk_callback_lock);
> +	queue->sock->sk->sk_data_ready =  queue->data_ready;
> +	queue->sock->sk->sk_state_change = queue->state_change;
> +	queue->sock->sk->sk_write_space = queue->write_space;
> +	queue->sock->sk->sk_user_data = NULL;

Shouldn't "sk_user_data = NULL" be protected by a write lock?

Maurizio

Re: [PATCH v2 1/7] net/handshake: Store the key serial number on completion

[Simon Horman]

On Fri, Sep 05, 2025 at 12:46:53PM +1000, alistair23@gmail.com wrote:

...

> diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c

...

> @@ -83,6 +87,13 @@ static void tls_handshake_remote_peerids(struct tls_handshake_req *treq,
>  		if (i >= treq->th_num_peerids)
>  			break;
>  	}
> +
> +	nla_for_each_attr(nla, head, len, rem) {
> +		if (nla_type(nla) == HANDSHAKE_A_DONE_SESSION_ID) {
> +			treq->user_session_id = nla_get_u32(nla);

Hi Alistair,

This appears to be addressed by patch 5/7 of this series.
But struct tls_handshake_req does not have a user_session_id member
with (only) this patch applied on top of net-next.

This breaks bisection and should be addressed: when each patch is applied
in turn the resulting source tree should compile.

...

> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index e2c5e0e626f9..1d5829aecf45 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -450,7 +450,8 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
>   * is present" flag on the xprt and let an upper layer enforce local
>   * security policy.
>   */

Please also add user_session_id to the kernel doc for this function (above).

Flagged by W=1 builds and ./scripts/kernel-doc -none 

> -static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid)
> +static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid,
> +				   key_serial_t user_session_id)
>  {
>  	struct svc_xprt *xprt = data;
>  	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index c5f7bbf5775f..3489c4693ff4 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2591,7 +2591,8 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
>   * @peerid: serial number of key containing the remote's identity
>   *
>   */
> -static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid)
> +static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid,
> +				  key_serial_t user_session_id)

Ditto.

>  {
>  	struct rpc_xprt *lower_xprt = data;
>  	struct sock_xprt *lower_transport =
> -- 
> 2.50.1
> 
> 

Re: [PATCH v2 7/7] nvmet-tcp: Support KeyUpdate

[Maurizio Lombardi]

On Fri Sep 5, 2025 at 4:46 AM CEST, alistair23 wrote:
> +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> +static int update_tls_keys(struct nvmet_tcp_queue *queue)
> +{
> +	int ret;
> +
> +	cancel_work(&queue->io_work);
> +	handshake_req_cancel(queue->sock->sk);
> +	handshake_sk_destruct_req(queue->sock->sk);
> +	queue->state = NVMET_TCP_Q_TLS_HANDSHAKE;
> +
> +	/* Restore the default callbacks before starting upcall */
> +	read_lock_bh(&queue->sock->sk->sk_callback_lock);
> +	queue->sock->sk->sk_data_ready =  queue->data_ready;
> +	queue->sock->sk->sk_state_change = queue->state_change;
> +	queue->sock->sk->sk_write_space = queue->write_space;
> +	queue->sock->sk->sk_user_data = NULL;
> +	read_unlock_bh(&queue->sock->sk->sk_callback_lock);
> +
> +	nvmet_stop_keep_alive_timer(queue->nvme_sq.ctrl);
> +
> +	INIT_DELAYED_WORK(&queue->tls_handshake_tmo_work,
> +			  nvmet_tcp_tls_handshake_timeout);
> +
> +	ret = nvmet_tcp_tls_handshake(queue, HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = wait_for_completion_interruptible_timeout(&queue->tls_complete, 10 * HZ);
> +
> +	if (ret <= 0) {
> +		tls_handshake_cancel(queue->sock->sk);
> +		return ret;
> +	}
> +
> +	queue->state = NVMET_TCP_Q_LIVE;
> +
> +	return ret;
> +}
> +#endif



> @@ -1408,14 +1474,22 @@ static void nvmet_tcp_io_work(struct work_struct *w)
>  		ret = nvmet_tcp_try_recv(queue, NVMET_TCP_RECV_BUDGET, &ops);
>  		if (ret > 0)
>  			pending = true;
> -		else if (ret < 0)
> -			return;
> +		else if (ret < 0) {
> +			if (ret == -EKEYEXPIRED)
> +				update_tls_keys(queue);
> +			else
> +				return;
> +		}
>  

What happens if CONFIG_NVME_TARGET_TCP_TLS is disabled?
I suspect the kernel build will fail with an update_tls_keys
implicit declaration error.

Maurizio

Re: [PATCH v2 5/7] net/handshake: Support KeyUpdate message types

[Simon Horman]

On Fri, Sep 05, 2025 at 12:46:57PM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> When reporting the msg-type to userspace let's also support reporting
> KeyUpdate events. This supports reporting a client/server event and if
> the other side requested a KeyUpdateRequest.
> 
> Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

...

> diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> index a273bc74d26f..1a3312fad410 100644
> --- a/Documentation/netlink/specs/handshake.yaml
> +++ b/Documentation/netlink/specs/handshake.yaml
> @@ -21,12 +21,17 @@ definitions:
>      type: enum
>      name: msg-type
>      value-start: 0
> -    entries: [unspec, clienthello, serverhello]
> +    entries: [unspec, clienthello, serverhello, clientkeyupdate, clientkeyupdaterequest, serverkeyupdate, serverkeyupdaterequest]
>    -

This line seems excessively long.
The preference is for lines no longer than 80 characters wide.

Flagged by yamllint.

...

> diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c

...

> @@ -348,7 +361,8 @@ EXPORT_SYMBOL(tls_client_hello_x509);
>   *   %-ESRCH: No user agent is available
>   *   %-ENOMEM: Memory allocation failed
>   */

Please also add keyupdate to the Kernel doc for this function.

Flagged by ./scripts/kernel-doc --none

> -int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> +int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
> +			 handshake_key_update_type keyupdate)
>  {
>  	struct tls_handshake_req *treq;
>  	struct handshake_req *req;

...

> @@ -410,7 +428,8 @@ EXPORT_SYMBOL(tls_server_hello_x509);
>   *   %-ESRCH: No user agent is available
>   *   %-ENOMEM: Memory allocation failed
>   */

Ditto.

> -int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> +int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags,
> +			 handshake_key_update_type keyupdate)
>  {
>  	struct tls_handshake_req *treq;
>  	struct handshake_req *req;

...

Re: [PATCH v2 7/7] nvmet-tcp: Support KeyUpdate

[Simon Horman]

On Fri, Sep 05, 2025 at 12:46:59PM +1000, alistair23@gmail.com wrote:

...

> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c

...

>  static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
>  		struct msghdr *msg, char *cbuf)
>  {
>  	struct cmsghdr *cmsg = (struct cmsghdr *)cbuf;
> -	u8 ctype, level, description;
> +	u8 ctype, htype, level, description;

nit: htype is unused in this function

Flagged by W=1 builds.

>  	int ret = 0;
>  
>  	ctype = tls_get_record_type(queue->sock->sk, cmsg);

...

Re: [PATCH v2 7/7] nvmet-tcp: Support KeyUpdate

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on net/main net-next/main linus/master v6.17-rc4 next-20250905]
[cannot apply to linux-nvme/for-next horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/alistair23-gmail-com/net-handshake-Store-the-key-serial-number-on-completion/20250905-105201
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250905024659.811386-8-alistair.francis%40wdc.com
patch subject: [PATCH v2 7/7] nvmet-tcp: Support KeyUpdate
config: s390-randconfig-001-20250905 (https://download.01.org/0day-ci/archive/20250905/202509052153.luapQMZm-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 9.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509052153.luapQMZm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509052153.luapQMZm-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/nvme/target/tcp.c: In function 'nvmet_tcp_tls_record_ok':
>> drivers/nvme/target/tcp.c:1173:12: warning: unused variable 'htype' [-Wunused-variable]
    1173 |  u8 ctype, htype, level, description;
         |            ^~~~~
   drivers/nvme/target/tcp.c: In function 'nvmet_tcp_io_work':
   drivers/nvme/target/tcp.c:1479:5: error: implicit declaration of function 'update_tls_keys'; did you mean 'update_cr_regs'? [-Werror=implicit-function-declaration]
    1479 |     update_tls_keys(queue);
         |     ^~~~~~~~~~~~~~~
         |     update_cr_regs
   cc1: some warnings being treated as errors


vim +/htype +1173 drivers/nvme/target/tcp.c

  1168	
  1169	static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
  1170			struct msghdr *msg, char *cbuf)
  1171	{
  1172		struct cmsghdr *cmsg = (struct cmsghdr *)cbuf;
> 1173		u8 ctype, htype, level, description;
  1174		int ret = 0;
  1175	
  1176		ctype = tls_get_record_type(queue->sock->sk, cmsg);
  1177		switch (ctype) {
  1178		case 0:
  1179			break;
  1180		case TLS_RECORD_TYPE_DATA:
  1181			break;
  1182		case TLS_RECORD_TYPE_ALERT:
  1183			tls_alert_recv(queue->sock->sk, msg, &level, &description);
  1184			if (level == TLS_ALERT_LEVEL_FATAL) {
  1185				pr_err("queue %d: TLS Alert desc %u\n",
  1186				       queue->idx, description);
  1187				ret = -ENOTCONN;
  1188			} else {
  1189				pr_warn("queue %d: TLS Alert desc %u\n",
  1190				       queue->idx, description);
  1191				ret = -EAGAIN;
  1192			}
  1193			break;
  1194		case TLS_RECORD_TYPE_HANDSHAKE:
  1195			ret = -EAGAIN;
  1196			break;
  1197		default:
  1198			/* discard this record type */
  1199			pr_err("queue %d: TLS record %d unhandled\n",
  1200			       queue->idx, ctype);
  1201			ret = -EAGAIN;
  1202			break;
  1203		}
  1204		return ret;
  1205	}
  1206	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/7] net/handshake: Make handshake_req_cancel public

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on net/main net-next/main linus/master linux-nvme/for-next v6.17-rc4 next-20250905]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/alistair23-gmail-com/net-handshake-Store-the-key-serial-number-on-completion/20250905-105201
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250905024659.811386-3-alistair.francis%40wdc.com
patch subject: [PATCH v2 2/7] net/handshake: Make handshake_req_cancel public
config: x86_64-randconfig-001-20250905 (https://download.01.org/0day-ci/archive/20250905/202509052149.ChcoGfkh-lkp@intel.com/config)
compiler: gcc-13 (Debian 13.3.0-16) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509052149.ChcoGfkh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509052149.ChcoGfkh-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/handshake/handshake-test.c: In function 'handshake_req_submit_test4':
>> net/handshake/handshake-test.c:237:9: error: implicit declaration of function 'handshake_req_cancel'; did you mean 'handshake_req_next'? [-Werror=implicit-function-declaration]
     237 |         handshake_req_cancel(sock->sk);
         |         ^~~~~~~~~~~~~~~~~~~~
         |         handshake_req_next
   cc1: some warnings being treated as errors


vim +237 net/handshake/handshake-test.c

88232ec1ec5ecf Chuck Lever 2023-04-17  207  
88232ec1ec5ecf Chuck Lever 2023-04-17  208  static void handshake_req_submit_test4(struct kunit *test)
88232ec1ec5ecf Chuck Lever 2023-04-17  209  {
88232ec1ec5ecf Chuck Lever 2023-04-17  210  	struct handshake_req *req, *result;
88232ec1ec5ecf Chuck Lever 2023-04-17  211  	struct socket *sock;
18c40a1cc1d990 Chuck Lever 2023-05-19  212  	struct file *filp;
88232ec1ec5ecf Chuck Lever 2023-04-17  213  	int err;
88232ec1ec5ecf Chuck Lever 2023-04-17  214  
88232ec1ec5ecf Chuck Lever 2023-04-17  215  	/* Arrange */
88232ec1ec5ecf Chuck Lever 2023-04-17  216  	req = handshake_req_alloc(&handshake_req_alloc_proto_good, GFP_KERNEL);
88232ec1ec5ecf Chuck Lever 2023-04-17  217  	KUNIT_ASSERT_NOT_NULL(test, req);
88232ec1ec5ecf Chuck Lever 2023-04-17  218  
88232ec1ec5ecf Chuck Lever 2023-04-17  219  	err = __sock_create(&init_net, PF_INET, SOCK_STREAM, IPPROTO_TCP,
88232ec1ec5ecf Chuck Lever 2023-04-17  220  			    &sock, 1);
88232ec1ec5ecf Chuck Lever 2023-04-17  221  	KUNIT_ASSERT_EQ(test, err, 0);
18c40a1cc1d990 Chuck Lever 2023-05-19  222  	filp = sock_alloc_file(sock, O_NONBLOCK, NULL);
18c40a1cc1d990 Chuck Lever 2023-05-19  223  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);
88232ec1ec5ecf Chuck Lever 2023-04-17  224  	KUNIT_ASSERT_NOT_NULL(test, sock->sk);
18c40a1cc1d990 Chuck Lever 2023-05-19  225  	sock->file = filp;
88232ec1ec5ecf Chuck Lever 2023-04-17  226  
88232ec1ec5ecf Chuck Lever 2023-04-17  227  	err = handshake_req_submit(sock, req, GFP_KERNEL);
88232ec1ec5ecf Chuck Lever 2023-04-17  228  	KUNIT_ASSERT_EQ(test, err, 0);
88232ec1ec5ecf Chuck Lever 2023-04-17  229  
88232ec1ec5ecf Chuck Lever 2023-04-17  230  	/* Act */
88232ec1ec5ecf Chuck Lever 2023-04-17  231  	result = handshake_req_hash_lookup(sock->sk);
88232ec1ec5ecf Chuck Lever 2023-04-17  232  
88232ec1ec5ecf Chuck Lever 2023-04-17  233  	/* Assert */
88232ec1ec5ecf Chuck Lever 2023-04-17  234  	KUNIT_EXPECT_NOT_NULL(test, result);
88232ec1ec5ecf Chuck Lever 2023-04-17  235  	KUNIT_EXPECT_PTR_EQ(test, req, result);
88232ec1ec5ecf Chuck Lever 2023-04-17  236  
88232ec1ec5ecf Chuck Lever 2023-04-17 @237  	handshake_req_cancel(sock->sk);
4a0f07d71b0483 Jinjie Ruan 2023-09-19  238  	fput(filp);
88232ec1ec5ecf Chuck Lever 2023-04-17  239  }
88232ec1ec5ecf Chuck Lever 2023-04-17  240  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Hannes Reinecke]

On 9/5/25 04:46, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> The TLS 1.3 specification allows the TLS client or server to send a
> KeyUpdate. This is generally used when the sequence is about to
> overflow or after a certain amount of bytes have been encrypted.
> 
> The TLS spec doesn't mandate the conditions though, so a KeyUpdate
> can be sent by the TLS client or server at any time. This includes
> when running NVMe-OF over a TLS 1.3 connection.
> 
> As such Linux should be able to handle a KeyUpdate event, as the
> other NVMe side could initiate a KeyUpdate.
> 
> Upcoming WD NVMe-TCP hardware controllers implement TLS support
> and send KeyUpdate requests.
> 
> This series builds on top of the existing TLS EKEYEXPIRED work,
> which already detects a KeyUpdate request. We can now pass that
> information up to the NVMe layer (target and host) and then pass
> it up to userspace.
> 
> Userspace (ktls-utils) will need to save the connection state
> in the keyring during the initial handshake. The kernel then
> provides the key serial back to userspace when handling a
> KeyUpdate. Userspace can use this to restore the connection
> information and then update the keys, this final process
> is similar to the initial handshake.
> 
> Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> 
> v2:
>   - Change "key-serial" to "session-id"
>   - Fix reported build failures
>   - Drop tls_clear_err() function
>   - Stop keep alive timer during KeyUpdate
>   - Drop handshake message decoding in the NVMe layer
> 
> Alistair Francis (7):
>    net/handshake: Store the key serial number on completion
>    net/handshake: Make handshake_req_cancel public
>    net/handshake: Expose handshake_sk_destruct_req publically
>    nvmet: Expose nvmet_stop_keep_alive_timer publically
>    net/handshake: Support KeyUpdate message types
>    nvme-tcp: Support KeyUpdate
>    nvmet-tcp: Support KeyUpdate
> 
>   Documentation/netlink/specs/handshake.yaml |  19 +++-
>   Documentation/networking/tls-handshake.rst |   4 +-
>   drivers/nvme/host/tcp.c                    |  88 +++++++++++++++--
>   drivers/nvme/target/core.c                 |   1 +
>   drivers/nvme/target/tcp.c                  | 104 +++++++++++++++++++--
>   include/net/handshake.h                    |  17 +++-
>   include/uapi/linux/handshake.h             |  14 +++
>   net/handshake/genl.c                       |   5 +-
>   net/handshake/handshake.h                  |   1 -
>   net/handshake/request.c                    |  18 ++++
>   net/handshake/tlshd.c                      |  46 +++++++--
>   net/sunrpc/svcsock.c                       |   3 +-
>   net/sunrpc/xprtsock.c                      |   3 +-
>   13 files changed, 289 insertions(+), 34 deletions(-)
> 

Hey Alistair,
thanks for doing this. While the patchset itself looks okay-ish, there
are some general ideas/concerns for it:

- I have posted a patch for replacing the current 'read_sock()'
interface with a recvmsg() base workflow. That should give us
access to the 'real' control message, so it would be good if you
could fold it in.
- Olga has send a patchset fixing a security issue with control
messages; the gist is that the network code expects a 'kvec' based
msg buffer when receiving a control message. So essentially one
has to receive a message _without_ a control buffer, check for
MSG_CTRUNC, and then read the control message via kvec.
Can you ensure that your patchset follows these guidelines?
- There is no method to trigger a KeyUpdate, making it really hard
to test this feature (eg by writin a blktest for it). Ideally we
should be able to trigger it from both directions, but having just
one (eg on the target side) should be enough for starters.
A possible interface would be to implement write support to the
'tls_key' debugfs attribute; when writing the same key ID as
the one currently in use the KeyUpdate mechanism could be started.

But thanks for doing the work!

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Olga Kornievskaia]

On Mon, Sep 15, 2025 at 7:46 AM Hannes Reinecke <hare@suse.de> wrote:
>
> On 9/5/25 04:46, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > The TLS 1.3 specification allows the TLS client or server to send a
> > KeyUpdate. This is generally used when the sequence is about to
> > overflow or after a certain amount of bytes have been encrypted.
> >
> > The TLS spec doesn't mandate the conditions though, so a KeyUpdate
> > can be sent by the TLS client or server at any time. This includes
> > when running NVMe-OF over a TLS 1.3 connection.
> >
> > As such Linux should be able to handle a KeyUpdate event, as the
> > other NVMe side could initiate a KeyUpdate.
> >
> > Upcoming WD NVMe-TCP hardware controllers implement TLS support
> > and send KeyUpdate requests.
> >
> > This series builds on top of the existing TLS EKEYEXPIRED work,
> > which already detects a KeyUpdate request. We can now pass that
> > information up to the NVMe layer (target and host) and then pass
> > it up to userspace.
> >
> > Userspace (ktls-utils) will need to save the connection state
> > in the keyring during the initial handshake. The kernel then
> > provides the key serial back to userspace when handling a
> > KeyUpdate. Userspace can use this to restore the connection
> > information and then update the keys, this final process
> > is similar to the initial handshake.
> >
> > Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> >
> > v2:
> >   - Change "key-serial" to "session-id"
> >   - Fix reported build failures
> >   - Drop tls_clear_err() function
> >   - Stop keep alive timer during KeyUpdate
> >   - Drop handshake message decoding in the NVMe layer
> >
> > Alistair Francis (7):
> >    net/handshake: Store the key serial number on completion
> >    net/handshake: Make handshake_req_cancel public
> >    net/handshake: Expose handshake_sk_destruct_req publically
> >    nvmet: Expose nvmet_stop_keep_alive_timer publically
> >    net/handshake: Support KeyUpdate message types
> >    nvme-tcp: Support KeyUpdate
> >    nvmet-tcp: Support KeyUpdate
> >
> >   Documentation/netlink/specs/handshake.yaml |  19 +++-
> >   Documentation/networking/tls-handshake.rst |   4 +-
> >   drivers/nvme/host/tcp.c                    |  88 +++++++++++++++--
> >   drivers/nvme/target/core.c                 |   1 +
> >   drivers/nvme/target/tcp.c                  | 104 +++++++++++++++++++--
> >   include/net/handshake.h                    |  17 +++-
> >   include/uapi/linux/handshake.h             |  14 +++
> >   net/handshake/genl.c                       |   5 +-
> >   net/handshake/handshake.h                  |   1 -
> >   net/handshake/request.c                    |  18 ++++
> >   net/handshake/tlshd.c                      |  46 +++++++--
> >   net/sunrpc/svcsock.c                       |   3 +-
> >   net/sunrpc/xprtsock.c                      |   3 +-
> >   13 files changed, 289 insertions(+), 34 deletions(-)
> >
>
> Hey Alistair,
> thanks for doing this. While the patchset itself looks okay-ish, there
> are some general ideas/concerns for it:
>
> - I have posted a patch for replacing the current 'read_sock()'
> interface with a recvmsg() base workflow. That should give us
> access to the 'real' control message, so it would be good if you
> could fold it in.
> - Olga has send a patchset fixing a security issue with control
> messages; the gist is that the network code expects a 'kvec' based
> msg buffer when receiving a control message. So essentially one
> has to receive a message _without_ a control buffer, check for
> MSG_CTRUNC, and then read the control message via kvec.
> Can you ensure that your patchset follows these guidelines?
> - There is no method to trigger a KeyUpdate, making it really hard
> to test this feature (eg by writin a blktest for it). Ideally we
> should be able to trigger it from both directions, but having just
> one (eg on the target side) should be enough for starters.
> A possible interface would be to implement write support to the
> 'tls_key' debugfs attribute; when writing the same key ID as
> the one currently in use the KeyUpdate mechanism could be started.
>
> But thanks for doing the work!

Hi Alistart,

I would like to pingy-pack on this message and ask a few questions as
I'm a bit confused about this implemenation.

NFS is also interested in being able to handle KeyUpdate functionality
of TLS and having NvME doing it serves as an example. But the general
approach confuses me.

All messages go thru a TLS (kernel) layer portion of sock_recvmsg
(kernel_recvmsg). When the TLS kernel layer detects that it's
non-TLS-data payload, it does various things depending on whether or
not control buffer was set up prior to the call to sock_recvmsg.
KeyUpdate message is a type of HANDSHAKE message and thus non-TLS-data
payload. While I was doing my changes to NvME code I noticed that
there are multiple places NvME (target) calls into kernel_recvmsg()
and thus those places would need to handle receiving non-TLS-data
payloads. Previously there was a TLS alert which is non-data but now
there is Handshake (specifically Keyupdate, but not others).

I guess where I'm going is I don't see how NvMe is connecting
receiving KeyUpdate (ie, identifying that it received specifically
that and not other handshake type) and its handling of KeyUpdate from
kernel_recvmsg the when NvME is just normally receiving data.

This patch series reads to me as it is expecting KeyUpdate to be a
part of the Handshake process (ie., there is a patch to "cancel" an
ongoing handshake, there is an upcall to tlshd with the KeyUpdate?).
This doesn't make sense to me. KeyUpdate, while a type of Handshake
message, is not done during the handshake -- it is done after sending
the Finished message which concludes the handshake flow (and
involvement of tlshd) and can happen at any time during normal TLS
encrypted message exchange after the handshake. Here's a snippet from
the TLS spec:

The KeyUpdate handshake message is used to indicate that the sender
   is updating its sending cryptographic keys.  This message can be sent
   by either peer after it has sent a Finished message.  Implementations
   that receive a KeyUpdate message prior to receiving a Finished
   message MUST terminate the connection with an "unexpected_message"
   alert.



>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
>

Re: [PATCH v2 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Alistair Francis]

On Tue, Sep 16, 2025 at 2:31 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Mon, Sep 15, 2025 at 7:46 AM Hannes Reinecke <hare@suse.de> wrote:
> >
> > On 9/5/25 04:46, alistair23@gmail.com wrote:
> > > From: Alistair Francis <alistair.francis@wdc.com>
> > >
> > > The TLS 1.3 specification allows the TLS client or server to send a
> > > KeyUpdate. This is generally used when the sequence is about to
> > > overflow or after a certain amount of bytes have been encrypted.
> > >
> > > The TLS spec doesn't mandate the conditions though, so a KeyUpdate
> > > can be sent by the TLS client or server at any time. This includes
> > > when running NVMe-OF over a TLS 1.3 connection.
> > >
> > > As such Linux should be able to handle a KeyUpdate event, as the
> > > other NVMe side could initiate a KeyUpdate.
> > >
> > > Upcoming WD NVMe-TCP hardware controllers implement TLS support
> > > and send KeyUpdate requests.
> > >
> > > This series builds on top of the existing TLS EKEYEXPIRED work,
> > > which already detects a KeyUpdate request. We can now pass that
> > > information up to the NVMe layer (target and host) and then pass
> > > it up to userspace.
> > >
> > > Userspace (ktls-utils) will need to save the connection state
> > > in the keyring during the initial handshake. The kernel then
> > > provides the key serial back to userspace when handling a
> > > KeyUpdate. Userspace can use this to restore the connection
> > > information and then update the keys, this final process
> > > is similar to the initial handshake.
> > >
> > > Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> > >
> > > v2:
> > >   - Change "key-serial" to "session-id"
> > >   - Fix reported build failures
> > >   - Drop tls_clear_err() function
> > >   - Stop keep alive timer during KeyUpdate
> > >   - Drop handshake message decoding in the NVMe layer
> > >
> > > Alistair Francis (7):
> > >    net/handshake: Store the key serial number on completion
> > >    net/handshake: Make handshake_req_cancel public
> > >    net/handshake: Expose handshake_sk_destruct_req publically
> > >    nvmet: Expose nvmet_stop_keep_alive_timer publically
> > >    net/handshake: Support KeyUpdate message types
> > >    nvme-tcp: Support KeyUpdate
> > >    nvmet-tcp: Support KeyUpdate
> > >
> > >   Documentation/netlink/specs/handshake.yaml |  19 +++-
> > >   Documentation/networking/tls-handshake.rst |   4 +-
> > >   drivers/nvme/host/tcp.c                    |  88 +++++++++++++++--
> > >   drivers/nvme/target/core.c                 |   1 +
> > >   drivers/nvme/target/tcp.c                  | 104 +++++++++++++++++++--
> > >   include/net/handshake.h                    |  17 +++-
> > >   include/uapi/linux/handshake.h             |  14 +++
> > >   net/handshake/genl.c                       |   5 +-
> > >   net/handshake/handshake.h                  |   1 -
> > >   net/handshake/request.c                    |  18 ++++
> > >   net/handshake/tlshd.c                      |  46 +++++++--
> > >   net/sunrpc/svcsock.c                       |   3 +-
> > >   net/sunrpc/xprtsock.c                      |   3 +-
> > >   13 files changed, 289 insertions(+), 34 deletions(-)
> > >
> >
> > Hey Alistair,
> > thanks for doing this. While the patchset itself looks okay-ish, there
> > are some general ideas/concerns for it:
> >
> > - I have posted a patch for replacing the current 'read_sock()'
> > interface with a recvmsg() base workflow. That should give us
> > access to the 'real' control message, so it would be good if you
> > could fold it in.

Thanks for sending that. I'll rebase my changes on top of the patch
and update it all.

> > - Olga has send a patchset fixing a security issue with control
> > messages; the gist is that the network code expects a 'kvec' based
> > msg buffer when receiving a control message. So essentially one
> > has to receive a message _without_ a control buffer, check for
> > MSG_CTRUNC, and then read the control message via kvec.

Oh interesting. I'll see if I can find the patchset and update my
series to follow that.

> > Can you ensure that your patchset follows these guidelines?
> > - There is no method to trigger a KeyUpdate, making it really hard
> > to test this feature (eg by writin a blktest for it). Ideally we

I have some patches that do send a KeyUpdate [1] which is what I'm
using to test. It allows me to send a KeyUpdate from either side.

> > should be able to trigger it from both directions, but having just
> > one (eg on the target side) should be enough for starters.
> > A possible interface would be to implement write support to the
> > 'tls_key' debugfs attribute; when writing the same key ID as
> > the one currently in use the KeyUpdate mechanism could be started.

That's a good point about allowing userspace and blktest to initiate a
KeyUpdate. I'll look at adding support for a debugfs attribute

> >
> > But thanks for doing the work!
>
> Hi Alistart,
>
> I would like to pingy-pack on this message and ask a few questions as
> I'm a bit confused about this implemenation.
>
> NFS is also interested in being able to handle KeyUpdate functionality
> of TLS and having NvME doing it serves as an example. But the general
> approach confuses me.
>
> All messages go thru a TLS (kernel) layer portion of sock_recvmsg
> (kernel_recvmsg). When the TLS kernel layer detects that it's
> non-TLS-data payload, it does various things depending on whether or
> not control buffer was set up prior to the call to sock_recvmsg.
> KeyUpdate message is a type of HANDSHAKE message and thus non-TLS-data
> payload. While I was doing my changes to NvME code I noticed that
> there are multiple places NvME (target) calls into kernel_recvmsg()
> and thus those places would need to handle receiving non-TLS-data
> payloads. Previously there was a TLS alert which is non-data but now
> there is Handshake (specifically Keyupdate, but not others).
>
> I guess where I'm going is I don't see how NvMe is connecting
> receiving KeyUpdate (ie, identifying that it received specifically
> that and not other handshake type) and its handling of KeyUpdate from
> kernel_recvmsg the when NvME is just normally receiving data.

The kernel TLS layer is handling the KeyUpdate [2]. The current
upstream Linux TLS layer will decode a KeyUpdate and mark a
`key_update_pending`. Note that upstream doesn't actually do a
KeyUpdate, hence this series.

>
> This patch series reads to me as it is expecting KeyUpdate to be a
> part of the Handshake process (ie., there is a patch to "cancel" an
> ongoing handshake, there is an upcall to tlshd with the KeyUpdate?).

No, KeyUpdate can't be part of the handshake process

> This doesn't make sense to me. KeyUpdate, while a type of Handshake
> message, is not done during the handshake -- it is done after sending
> the Finished message which concludes the handshake flow (and
> involvement of tlshd) and can happen at any time during normal TLS
> encrypted message exchange after the handshake. Here's a snippet from
> the TLS spec:

Exactly, KeyUpdate can happen at any time after the handshake.

The general idea is that the TLS layer will detect a KeyUpdate [2],
then report EKEYEXPIRED to the NVMe layer which then asks
ktls-utils/gnutls in user space to update the keys. Does that make
more sense?

>
> The KeyUpdate handshake message is used to indicate that the sender
>    is updating its sending cryptographic keys.  This message can be sent
>    by either peer after it has sent a Finished message.  Implementations
>    that receive a KeyUpdate message prior to receiving a Finished
>    message MUST terminate the connection with an "unexpected_message"
>    alert.
>

1: https://github.com/alistair23/linux/commit/714d58a0aed5d49fb24ea22497024c3d958a60b8
2: https://github.com/torvalds/linux/blob/46a51f4f5edade43ba66b3c151f0e25ec8b69cb6/net/tls/tls_sw.c#L1775

Alistair

Re: [PATCH v2 6/7] nvme-tcp: Support KeyUpdate

[Hannes Reinecke]

On 9/5/25 04:46, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> If the nvme_tcp_try_send() or nvme_tcp_try_recv() functions return
> EKEYEXPIRED then the underlying TLS keys need to be updated. This occurs
> on an KeyUpdate event.
> 
> If the NVMe Target (TLS server) initiates a KeyUpdate this patch will
> allow the NVMe layer to process the KeyUpdate request and forward the
> request to userspace. Userspace must then update the key to keep the
> connection alive.
> 
> This patch allows us to handle the NVMe target sending a KeyUpdate
> request without aborting the connection. At this time we don't support
> initiating a KeyUpdate.
> 
> Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
>   - Don't change the state
>   - Use a helper function for KeyUpdates
>   - Continue sending in nvme_tcp_send_all() after a KeyUpdate
>   - Remove command message using recvmsg
>   
>   drivers/nvme/host/tcp.c | 73 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 776047a71436..b6449effc2ac 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -171,6 +171,7 @@ struct nvme_tcp_queue {
>   	bool			tls_enabled;
>   	u32			rcv_crc;
>   	u32			snd_crc;
> +	key_serial_t		user_session_id;
>   	__le32			exp_ddgst;
>   	__le32			recv_ddgst;
>   	struct completion       tls_complete;
> @@ -210,6 +211,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>   			      struct nvme_tcp_queue *queue,
>   			      key_serial_t pskid,
>   			      handshake_key_update_type keyupdate);
> +static void update_tls_keys(struct nvme_tcp_queue *queue);
>   
>   static inline struct nvme_tcp_ctrl *to_tcp_ctrl(struct nvme_ctrl *ctrl)
>   {
> @@ -393,6 +395,14 @@ static inline void nvme_tcp_send_all(struct nvme_tcp_queue *queue)
>   	do {
>   		ret = nvme_tcp_try_send(queue);
>   	} while (ret > 0);
> +
> +	if (ret == -EKEYEXPIRED) {
> +		update_tls_keys(queue);
> +
> +		do {
> +			ret = nvme_tcp_try_send(queue);
> +		} while (ret > 0);
> +	}
>   }
>   
>   static inline bool nvme_tcp_queue_has_pending(struct nvme_tcp_queue *queue)
> @@ -1347,6 +1357,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
>   done:
>   	if (ret == -EAGAIN) {
>   		ret = 0;
> +	} else if (ret == -EKEYEXPIRED) {
> +		goto out;
>   	} else if (ret < 0) {
>   		dev_err(queue->ctrl->ctrl.device,
>   			"failed to send request %d\n", ret);
> @@ -1371,9 +1383,56 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
>   	queue->nr_cqe = 0;
>   	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
>   	release_sock(sk);
> +
> +	/* If we received EINVAL from read_sock then it generally means the
> +	 * other side sent a command message. So let's try to clear it from
> +	 * our queue with a recvmsg, otherwise we get stuck in an infinite
> +	 * loop.
> +	 */
> +	if (consumed == -EINVAL) {
> +		char cbuf[CMSG_LEN(sizeof(char))] = {};
> +		struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> +		struct bio_vec bvec;
> +
> +		bvec_set_virt(&bvec, (void *)cbuf, sizeof(cbuf));
> +		iov_iter_bvec(&msg.msg_iter, ITER_DEST, &bvec, 1, sizeof(cbuf));
> +
> +		msg.msg_control = cbuf;
> +		msg.msg_controllen = sizeof(cbuf);
> +
> +		consumed = sock_recvmsg(sock, &msg, msg.msg_flags);
> +	}
> +
>   	return consumed == -EAGAIN ? 0 : consumed;
>   }
>   
> +static void update_tls_keys(struct nvme_tcp_queue *queue)
> +{
> +	int qid = nvme_tcp_queue_id(queue);
> +	int ret;
> +
> +	dev_dbg(queue->ctrl->ctrl.device,
> +		"updating key for queue %d\n", qid);
> +
> +	cancel_work(&queue->io_work);
> +	handshake_req_cancel(queue->sock->sk);
> +	handshake_sk_destruct_req(queue->sock->sk);
> +
Careful here. The RFC fully expects to have several KeyUpdate requests
pending (eg if both sides decide so initiate a KeyUpdate at the same
time). And cancelling a handshake request would cause tlshd/gnutls
to lose track of the generation counter and generate an invalid
traffic secret.
I would just let it rip and don't bother with other handshake
requests.

> +	nvme_stop_keep_alive(&(queue->ctrl->ctrl));
> +	flush_work(&(queue->ctrl->ctrl).async_event_work);
> +
Oh bugger. Seems like gnutls is generating the KeyUpdate message
itself, and we have to wait for that.
So much for KeyUpdate being transparent without having to stop I/O...

Can't we fix gnutls to make sending the KeyUpdate message and changing
the IV parameters an atomic operation? That would be a far better 
interface, as then we would not need to stop I/O and the handshake
process could run fully asynchronous to normal I/O...

> +	ret = nvme_tcp_start_tls(&(queue->ctrl->ctrl),
> +				 queue, queue->ctrl->ctrl.tls_pskid,
> +				 HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
> +
> +	if (ret < 0) {
> +		dev_err(queue->ctrl->ctrl.device,
> +			"failed to update the keys %d\n", ret);
> +		nvme_tcp_fail_request(queue->request);
> +		nvme_tcp_done_send_req(queue);
> +	}
> +}
> +
>   static void nvme_tcp_io_work(struct work_struct *w)
>   {
>   	struct nvme_tcp_queue *queue =
> @@ -1389,15 +1448,21 @@ static void nvme_tcp_io_work(struct work_struct *w)
>   			mutex_unlock(&queue->send_mutex);
>   			if (result > 0)
>   				pending = true;
> -			else if (unlikely(result < 0))
> +			else if (unlikely(result < 0)) {
> +				if (result == -EKEYEXPIRED)
> +					update_tls_keys(queue);

How exactly can we get -EKEYEXPIRED when _sending_?
To my understanding that would have required userspace to intercept
here trying (or even sending) a KeyUpdate message, right?
So really not something we should see during normal operation.
As mentioned in my previous mail we should rather code the
KeyUpdate process itself here, too.
Namely: Trigger the KeyUpdate via userspace (eg by writing into the 
tls_key attribute for the controller), and then have the kernel side
to call out into tlshd to initiate the KeyUpdate 'handshake'.
That way we have identical flow of control for both the sending
and receiving side.

Incidentally: the RFC has some notion about 'request_update' setting
in the KeyUpdate message. Is that something we have to care about at
this level?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 6/7] nvme-tcp: Support KeyUpdate

[Alistair Francis]

On Tue, Sep 16, 2025 at 11:04 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 9/5/25 04:46, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > If the nvme_tcp_try_send() or nvme_tcp_try_recv() functions return
> > EKEYEXPIRED then the underlying TLS keys need to be updated. This occurs
> > on an KeyUpdate event.
> >
> > If the NVMe Target (TLS server) initiates a KeyUpdate this patch will
> > allow the NVMe layer to process the KeyUpdate request and forward the
> > request to userspace. Userspace must then update the key to keep the
> > connection alive.
> >
> > This patch allows us to handle the NVMe target sending a KeyUpdate
> > request without aborting the connection. At this time we don't support
> > initiating a KeyUpdate.
> >
> > Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > v2:
> >   - Don't change the state
> >   - Use a helper function for KeyUpdates
> >   - Continue sending in nvme_tcp_send_all() after a KeyUpdate
> >   - Remove command message using recvmsg
> >
> >   drivers/nvme/host/tcp.c | 73 +++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 70 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 776047a71436..b6449effc2ac 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -171,6 +171,7 @@ struct nvme_tcp_queue {
> >       bool                    tls_enabled;
> >       u32                     rcv_crc;
> >       u32                     snd_crc;
> > +     key_serial_t            user_session_id;
> >       __le32                  exp_ddgst;
> >       __le32                  recv_ddgst;
> >       struct completion       tls_complete;
> > @@ -210,6 +211,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
> >                             struct nvme_tcp_queue *queue,
> >                             key_serial_t pskid,
> >                             handshake_key_update_type keyupdate);
> > +static void update_tls_keys(struct nvme_tcp_queue *queue);
> >
> >   static inline struct nvme_tcp_ctrl *to_tcp_ctrl(struct nvme_ctrl *ctrl)
> >   {
> > @@ -393,6 +395,14 @@ static inline void nvme_tcp_send_all(struct nvme_tcp_queue *queue)
> >       do {
> >               ret = nvme_tcp_try_send(queue);
> >       } while (ret > 0);
> > +
> > +     if (ret == -EKEYEXPIRED) {
> > +             update_tls_keys(queue);
> > +
> > +             do {
> > +                     ret = nvme_tcp_try_send(queue);
> > +             } while (ret > 0);
> > +     }
> >   }
> >
> >   static inline bool nvme_tcp_queue_has_pending(struct nvme_tcp_queue *queue)
> > @@ -1347,6 +1357,8 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
> >   done:
> >       if (ret == -EAGAIN) {
> >               ret = 0;
> > +     } else if (ret == -EKEYEXPIRED) {
> > +             goto out;
> >       } else if (ret < 0) {
> >               dev_err(queue->ctrl->ctrl.device,
> >                       "failed to send request %d\n", ret);
> > @@ -1371,9 +1383,56 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
> >       queue->nr_cqe = 0;
> >       consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
> >       release_sock(sk);
> > +
> > +     /* If we received EINVAL from read_sock then it generally means the
> > +      * other side sent a command message. So let's try to clear it from
> > +      * our queue with a recvmsg, otherwise we get stuck in an infinite
> > +      * loop.
> > +      */
> > +     if (consumed == -EINVAL) {
> > +             char cbuf[CMSG_LEN(sizeof(char))] = {};
> > +             struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> > +             struct bio_vec bvec;
> > +
> > +             bvec_set_virt(&bvec, (void *)cbuf, sizeof(cbuf));
> > +             iov_iter_bvec(&msg.msg_iter, ITER_DEST, &bvec, 1, sizeof(cbuf));
> > +
> > +             msg.msg_control = cbuf;
> > +             msg.msg_controllen = sizeof(cbuf);
> > +
> > +             consumed = sock_recvmsg(sock, &msg, msg.msg_flags);
> > +     }
> > +
> >       return consumed == -EAGAIN ? 0 : consumed;
> >   }
> >
> > +static void update_tls_keys(struct nvme_tcp_queue *queue)
> > +{
> > +     int qid = nvme_tcp_queue_id(queue);
> > +     int ret;
> > +
> > +     dev_dbg(queue->ctrl->ctrl.device,
> > +             "updating key for queue %d\n", qid);
> > +
> > +     cancel_work(&queue->io_work);
> > +     handshake_req_cancel(queue->sock->sk);
> > +     handshake_sk_destruct_req(queue->sock->sk);
> > +
> Careful here. The RFC fully expects to have several KeyUpdate requests
> pending (eg if both sides decide so initiate a KeyUpdate at the same
> time). And cancelling a handshake request would cause tlshd/gnutls
> to lose track of the generation counter and generate an invalid
> traffic secret.
> I would just let it rip and don't bother with other handshake
> requests.

Unfortunately that doesn't work as future calls to
`handshake_req_hash_add()` will fail.

I now think that's a bug in `handshake_complete()` and I have a better
fix in the next version.

>
> > +     nvme_stop_keep_alive(&(queue->ctrl->ctrl));
> > +     flush_work(&(queue->ctrl->ctrl).async_event_work);
> > +
> Oh bugger. Seems like gnutls is generating the KeyUpdate message
> itself, and we have to wait for that.

Yes, we have gnutls generate the message.

> So much for KeyUpdate being transparent without having to stop I/O...
>
> Can't we fix gnutls to make sending the KeyUpdate message and changing
> the IV parameters an atomic operation? That would be a far better

I'm not sure I follow.

ktls-utils will first restore the gnutls session. Then have gnutls
trigger a KeyUpdate.gnutls will send a KeyUpdate and then tell the
kernel the new keys. The kernel cannot send or encrypt any data after
the KeyUpdate has been sent until the keys are updated.

I don't see how we could make it an atomic operation. We have to stop
the traffic between sending a KeyUpdate and updating the keys.
Otherwise we will send invalid data.

> interface, as then we would not need to stop I/O and the handshake
> process could run fully asynchronous to normal I/O...
>
> > +     ret = nvme_tcp_start_tls(&(queue->ctrl->ctrl),
> > +                              queue, queue->ctrl->ctrl.tls_pskid,
> > +                              HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
> > +
> > +     if (ret < 0) {
> > +             dev_err(queue->ctrl->ctrl.device,
> > +                     "failed to update the keys %d\n", ret);
> > +             nvme_tcp_fail_request(queue->request);
> > +             nvme_tcp_done_send_req(queue);
> > +     }
> > +}
> > +
> >   static void nvme_tcp_io_work(struct work_struct *w)
> >   {
> >       struct nvme_tcp_queue *queue =
> > @@ -1389,15 +1448,21 @@ static void nvme_tcp_io_work(struct work_struct *w)
> >                       mutex_unlock(&queue->send_mutex);
> >                       if (result > 0)
> >                               pending = true;
> > -                     else if (unlikely(result < 0))
> > +                     else if (unlikely(result < 0)) {
> > +                             if (result == -EKEYEXPIRED)
> > +                                     update_tls_keys(queue);
>
> How exactly can we get -EKEYEXPIRED when _sending_?

Good point. You can't with this current patch set. I have patches on
top of this that will generate a KeyUpate as part of the send
operation, which I plan to submit after this series.

So this is a bit of prep work to setup the NVMe layer to handle
sending and receiving KeyUpdate requests. I can drop this change from
the series if that's prefered?

> To my understanding that would have required userspace to intercept
> here trying (or even sending) a KeyUpdate message, right?

Not necessarily. The TLS layer can trigger a KeyUpdate independent of
userspace. This would happen for example if the sequence count was
about to overflow, which is what I use in my testing. Userspace has no
idea of the current sequence number, so it can't be involved. The
kernel will need to start the KeyUpdate send if the rec_seq is about
to overflow.

> So really not something we should see during normal operation.
> As mentioned in my previous mail we should rather code the
> KeyUpdate process itself here, too.
> Namely: Trigger the KeyUpdate via userspace (eg by writing into the
> tls_key attribute for the controller), and then have the kernel side
> to call out into tlshd to initiate the KeyUpdate 'handshake'.

Yeah, I agree about exposing a way for userspace to trigger an update.
That would only be for testing though, as in normal operation
userspace has no insight into the current connection state. In a
production system the kernel TLS layer will need to initiate a
KeyUpdate.

> That way we have identical flow of control for both the sending
> and receiving side.
>
> Incidentally: the RFC has some notion about 'request_update' setting
> in the KeyUpdate message. Is that something we have to care about at
> this level?

It is something we will need to care about. At this stage it isn't
supported as it adds a little bit of complexity, but I should be able
to extend the current approach to support a request_update.

Alistair

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 6/7] nvme-tcp: Support KeyUpdate

[Hannes Reinecke]

On 9/17/25 05:14, Alistair Francis wrote:
> On Tue, Sep 16, 2025 at 11:04 PM Hannes Reinecke <hare@suse.de> wrote:
>>
[ .. ]
>> Oh bugger. Seems like gnutls is generating the KeyUpdate message
>> itself, and we have to wait for that.
> 
> Yes, we have gnutls generate the message.
> 
>> So much for KeyUpdate being transparent without having to stop I/O...
>>
>> Can't we fix gnutls to make sending the KeyUpdate message and changing
>> the IV parameters an atomic operation? That would be a far better
> 
> I'm not sure I follow.
> 
> ktls-utils will first restore the gnutls session. Then have gnutls
> trigger a KeyUpdate.gnutls will send a KeyUpdate and then tell the
> kernel the new keys. The kernel cannot send or encrypt any data after
> the KeyUpdate has been sent until the keys are updated.
> 
> I don't see how we could make it an atomic operation. We have to stop
> the traffic between sending a KeyUpdate and updating the keys.
> Otherwise we will send invalid data.
> 
Fully agree with that.
But thing is, the KeyUpdate message is a unidirectional thing.
Host A initiating a KeyUpdate must only change the _sender_ side
keys after sending a KeyUpdate message to host B; the receiver
side keys on host A can only be update once it received the 
corresponding KeyUpdate from host B. If both keys on host A
are modified at the same time we cannot receive the KeyUpdate
message from host B as that will be encoded with the old
keys ...

I wonder how that can be modeled in gnutls; I only see
gnutls_session_key_update() which apparently will update both
keys at once.
Which would fit perfectly for host B receiving the initial KeyUpdate,
(and is probably the reason why you did that side first :-)
but what to do for host A?

Looking at the code gnutls seem to expect to read the handshake
message from the socket, but that message is already processed by
the in-kernel TLS socket.
So either we need to patch gnutls or push a fake handshake
message onto the socket for gnutls to read. Bah.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 6/7] nvme-tcp: Support KeyUpdate

[Alistair Francis]

On Wed, Sep 17, 2025 at 8:12 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 9/17/25 05:14, Alistair Francis wrote:
> > On Tue, Sep 16, 2025 at 11:04 PM Hannes Reinecke <hare@suse.de> wrote:
> >>
> [ .. ]
> >> Oh bugger. Seems like gnutls is generating the KeyUpdate message
> >> itself, and we have to wait for that.
> >
> > Yes, we have gnutls generate the message.
> >
> >> So much for KeyUpdate being transparent without having to stop I/O...
> >>
> >> Can't we fix gnutls to make sending the KeyUpdate message and changing
> >> the IV parameters an atomic operation? That would be a far better
> >
> > I'm not sure I follow.
> >
> > ktls-utils will first restore the gnutls session. Then have gnutls
> > trigger a KeyUpdate.gnutls will send a KeyUpdate and then tell the
> > kernel the new keys. The kernel cannot send or encrypt any data after
> > the KeyUpdate has been sent until the keys are updated.
> >
> > I don't see how we could make it an atomic operation. We have to stop
> > the traffic between sending a KeyUpdate and updating the keys.
> > Otherwise we will send invalid data.
> >
> Fully agree with that.
> But thing is, the KeyUpdate message is a unidirectional thing.
> Host A initiating a KeyUpdate must only change the _sender_ side
> keys after sending a KeyUpdate message to host B; the receiver
> side keys on host A can only be update once it received the
> corresponding KeyUpdate from host B. If both keys on host A
> are modified at the same time we cannot receive the KeyUpdate
> message from host B as that will be encoded with the old
> keys ...

Correct

>
> I wonder how that can be modeled in gnutls; I only see
> gnutls_session_key_update() which apparently will update both
> keys at once.

gnutls_session_key_update() only updates our keys [1]. You can use the
GNUTLS_KU_PEER flag to set `request_update` to update all keys.

> Which would fit perfectly for host B receiving the initial KeyUpdate,
> (and is probably the reason why you did that side first :-)
> but what to do for host A?

Patch has been sent and reviewed, just hasn't been merged yet:

https://gitlab.com/gnutls/gnutls/-/merge_requests/1965

>
> Looking at the code gnutls seem to expect to read the handshake
> message from the socket, but that message is already processed by
> the in-kernel TLS socket.
> So either we need to patch gnutls or push a fake handshake
> message onto the socket for gnutls to read. Bah.

Correct, patch is pending (see above)

1: https://gitlab.com/gnutls/gnutls/-/blob/master/lib/tls13/key_update.c#L245

Alistair

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich