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

[PATCH v4 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.

This series depends on the recvmsg() kernel patch:
https://lore.kernel.org/linux-nvme/2cbe1350-0bf5-4487-be33-1d317cb73acf@suse.de/T/#mf56283228ae6c93e37dfbf1c0f6263910217cd80

ktls-utils (tlshd) userspace patches are available at:
https://lore.kernel.org/kernel-tls-handshake/CAKmqyKNpFhPtM8HAkgRMKQA8_N7AgoeqaSTe2=0spPnb+Oz2ng@mail.gmail.com/T/#mb277f5c998282666d0f41cc02f4abf516fcc4e9c

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

Based-on: 2cbe1350-0bf5-4487-be33-1d317cb73acf@suse.de

v4:
 - Don't stop the keep-alive timer
 - Remove any support for sending a KeyUpdate
 - Add tls_client_keyupdate_psk()' and 'tls_server_keyupdate_psk()'
 - Code cleanups
 - Change order of patches
v3:
 - Rebase on the recvmsg() workflow patch
 - Add debugfs support for the host
 - Don't cancel an ongoing request
 - Ensure a request is destructed on completion
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: Define handshake_sk_destruct_req
  net/handshake: Ensure the request is destructed on completion
  net/handshake: Support KeyUpdate message types
  nvme-tcp: Support KeyUpdate
  nvme-tcp: Allow userspace to trigger a KeyUpdate with debugfs
  nvmet-tcp: Support KeyUpdate

 Documentation/netlink/specs/handshake.yaml |  20 +-
 Documentation/networking/tls-handshake.rst |   2 +
 drivers/nvme/host/tcp.c                    | 150 ++++++++++++--
 drivers/nvme/target/tcp.c                  | 216 ++++++++++++++-------
 include/net/handshake.h                    |  12 +-
 include/uapi/linux/handshake.h             |  14 ++
 net/handshake/genl.c                       |   5 +-
 net/handshake/request.c                    |  18 ++
 net/handshake/tlshd.c                      |  96 ++++++++-
 net/sunrpc/svcsock.c                       |   4 +-
 net/sunrpc/xprtsock.c                      |   4 +-
 11 files changed, 455 insertions(+), 86 deletions(-)

-- 
2.51.0

[PATCH v4 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>
Reviewed-by: Hannes Reincke <hare@suse.de>
---
v4:
 - No change
v3:
 - No change
v2:
 - Change "key-serial" to "session-id"

 Documentation/netlink/specs/handshake.yaml |  4 ++++
 Documentation/networking/tls-handshake.rst |  2 ++
 drivers/nvme/host/tcp.c                    |  3 ++-
 drivers/nvme/target/tcp.c                  |  3 ++-
 include/net/handshake.h                    |  4 +++-
 include/uapi/linux/handshake.h             |  1 +
 net/handshake/genl.c                       |  5 +++--
 net/handshake/tlshd.c                      | 15 +++++++++++++--
 net/sunrpc/svcsock.c                       |  4 +++-
 net/sunrpc/xprtsock.c                      |  4 +++-
 10 files changed, 36 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/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
index 6f5ea1646a47..d7287890056a 100644
--- a/Documentation/networking/tls-handshake.rst
+++ b/Documentation/networking/tls-handshake.rst
@@ -60,6 +60,8 @@ fills in a structure that contains the parameters of the request:
         key_serial_t    ta_my_privkey;
         unsigned int    ta_num_peerids;
         key_serial_t    ta_my_peerids[5];
+        key_serial_t    user_session_id;
+
   };
 
 The @ta_sock field references an open and connected socket. The consumer
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2751c15beed6..611be56f8013 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1691,7 +1691,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..dc2222fd6d99 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;
@@ -31,6 +32,7 @@ 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);
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..2549c5dbccd8 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_session_id;
 };
 
 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_session_id = 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 7b90abc5cf0e..bc9a713c6559 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -444,13 +444,15 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
  * @data: address of xprt to wake
  * @status: status of handshake
  * @peerid: serial number of key containing the remote peer's identity
+ * @user_session_id: serial number of the userspace session ID
  *
  * If a security policy is specified as an export option, we don't
  * have a specific export here to check. So we set a "TLS session
  * 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 3aa987e7f072..bce0f43bef65 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2589,9 +2589,11 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
  * @data: address of xprt to wake
  * @status: status of handshake
  * @peerid: serial number of key containing the remote's identity
+ * @user_session_id: serial number of the userspace session ID
  *
  */
-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.51.0

[PATCH v4 2/7] net/handshake: Define handshake_sk_destruct_req

[alistair23]

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

Define a `handshake_sk_destruct_req()` function to allow the destruction
of the handshake req.

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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
v4:
 - No change
v3:
 - New patch

 net/handshake/request.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/handshake/request.c b/net/handshake/request.c
index 274d2c89b6b2..0d1c91c80478 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -98,6 +98,22 @@ static void handshake_sk_destruct(struct sock *sk)
 		sk_destruct(sk);
 }
 
+/**
+ * handshake_sk_destruct_req - destroy an existing request
+ * @sk: socket on which there is an existing request
+ */
+static 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);
+}
+
 /**
  * handshake_req_alloc - Allocate a handshake request
  * @proto: security protocol
-- 
2.51.0

[PATCH v4 3/7] net/handshake: Ensure the request is destructed on completion

[alistair23]

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

To avoid future handshake_req_hash_add() calls failing with EEXIST when
performing a KeyUpdate let's make sure the old request is destructed
as part of the completion.

Until now a handshake would only be destroyed on a failure or when a
sock is freed (via the sk_destruct function pointer).
handshake_complete() is only called on errors, not a successful
handshake so it doesn't remove the request.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
v4:
 - Improve description in commit message
v3:
 - New patch

 net/handshake/request.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/handshake/request.c b/net/handshake/request.c
index 0d1c91c80478..194725a8aaca 100644
--- a/net/handshake/request.c
+++ b/net/handshake/request.c
@@ -311,6 +311,8 @@ void handshake_complete(struct handshake_req *req, unsigned int status,
 		/* Handshake request is no longer pending */
 		sock_put(sk);
 	}
+
+	handshake_sk_destruct_req(sk);
 }
 EXPORT_SYMBOL_IF_KUNIT(handshake_complete);
 
-- 
2.51.0

[PATCH v4 4/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>
---
v4:
 - Don't overload existing functions, instead create new ones
v3:
 - Fixup yamllint and kernel-doc failures

 Documentation/netlink/specs/handshake.yaml | 16 ++++-
 drivers/nvme/host/tcp.c                    | 15 +++-
 drivers/nvme/target/tcp.c                  | 10 ++-
 include/net/handshake.h                    |  8 +++
 include/uapi/linux/handshake.h             | 13 ++++
 net/handshake/tlshd.c                      | 83 +++++++++++++++++++++-
 6 files changed, 137 insertions(+), 8 deletions(-)

diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
index a273bc74d26f..c72ec8fa7d7a 100644
--- a/Documentation/netlink/specs/handshake.yaml
+++ b/Documentation/netlink/specs/handshake.yaml
@@ -21,12 +21,18 @@ 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 +80,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 +129,7 @@ operations:
             - certificate
             - peername
             - keyring
+            - key-serial
     -
       name: done
       doc: Handler reports handshake completion
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 611be56f8013..2696bf97dfac 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -20,6 +20,7 @@
 #include <linux/iov_iter.h>
 #include <net/busy_poll.h>
 #include <trace/events/sock.h>
+#include <uapi/linux/handshake.h>
 
 #include "nvme.h"
 #include "fabrics.h"
@@ -206,6 +207,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)
 {
@@ -1726,7 +1731,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;
@@ -1748,7 +1754,10 @@ 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);
+	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
+		ret = tls_client_hello_psk(&args, GFP_KERNEL);
+	else
+		ret = tls_client_keyupdate_psk(&args, GFP_KERNEL, keyupdate);
 	if (ret) {
 		dev_err(nctrl->device, "queue %d: failed to start TLS: %d\n",
 			qid, ret);
@@ -1898,7 +1907,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..8aeec4a7f136 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1833,7 +1833,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 +1853,10 @@ 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);
+	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
+		ret = tls_server_hello_psk(&args, GFP_KERNEL);
+	else
+		ret = tls_server_keyupdate_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 +1938,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 dc2222fd6d99..084c92a20b68 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,
@@ -38,8 +42,12 @@ struct tls_handshake_args {
 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_keyupdate_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_keyupdate_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 2549c5dbccd8..c40839977ab9 100644
--- a/net/handshake/tlshd.c
+++ b/net/handshake/tlshd.c
@@ -41,6 +41,7 @@ struct tls_handshake_req {
 	unsigned int		th_num_peerids;
 	key_serial_t		th_peerid[5];
 
+	int			th_key_update_request;
 	key_serial_t		user_session_id;
 };
 
@@ -58,7 +59,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_session_id = TLS_NO_PRIVKEY;
+	treq->user_session_id = args->user_session_id;
+
 	return treq;
 }
 
@@ -265,6 +267,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);
 
@@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
 }
 EXPORT_SYMBOL(tls_client_hello_psk);
 
+/**
+ * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
+ * @args: socket and handshake parameters for this request
+ * @flags: memory allocation control flags
+ * @keyupdate: specifies the type of KeyUpdate operation
+ *
+ * Return values:
+ *   %0: Handshake request enqueue; ->done will be called when complete
+ *   %-EINVAL: Wrong number of local peer IDs
+ *   %-ESRCH: No user agent is available
+ *   %-ENOMEM: Memory allocation failed
+ */
+int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
+			     handshake_key_update_type keyupdate)
+{
+	struct tls_handshake_req *treq;
+	struct handshake_req *req;
+	unsigned int i;
+
+	if (!args->ta_num_peerids ||
+	    args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
+		return -EINVAL;
+
+	req = handshake_req_alloc(&tls_handshake_proto, flags);
+	if (!req)
+		return -ENOMEM;
+	treq = tls_handshake_req_init(req, args);
+	treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
+	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++)
+		treq->th_peerid[i] = args->ta_my_peerids[i];
+
+	return handshake_req_submit(args->ta_sock, req, flags);
+}
+EXPORT_SYMBOL(tls_client_keyupdate_psk);
+
 /**
  * tls_server_hello_x509 - request a server TLS handshake on a socket
  * @args: socket and handshake parameters for this request
@@ -428,6 +478,37 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
 }
 EXPORT_SYMBOL(tls_server_hello_psk);
 
+/**
+ * tls_server_keyupdate_psk - request a server TLS KeyUpdate on a socket
+ * @args: socket and handshake parameters for this request
+ * @flags: memory allocation control flags
+ * @keyupdate: specifies the type of KeyUpdate operation
+ *
+ * Return values:
+ *   %0: Handshake request enqueue; ->done will be called when complete
+ *   %-ESRCH: No user agent is available
+ *   %-ENOMEM: Memory allocation failed
+ */
+int tls_server_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
+			     handshake_key_update_type keyupdate)
+{
+	struct tls_handshake_req *treq;
+	struct handshake_req *req;
+
+	req = handshake_req_alloc(&tls_handshake_proto, flags);
+	if (!req)
+		return -ENOMEM;
+	treq = tls_handshake_req_init(req, args);
+	treq->th_type = HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE;
+	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];
+
+	return handshake_req_submit(args->ta_sock, req, flags);
+}
+EXPORT_SYMBOL(tls_server_keyupdate_psk);
+
 /**
  * tls_handshake_cancel - cancel a pending handshake
  * @sk: socket on which there is an ongoing handshake
-- 
2.51.0

[PATCH v4 5/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>
---
v4:
 - Remove all support for initiating KeyUpdate
 - Don't call cancel_work() when updating keys
v3:
 - Don't cancel existing handshake requests
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 | 60 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2696bf97dfac..791e0cc91ad8 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -172,6 +172,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;
@@ -858,7 +859,10 @@ static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue,
 static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
 {
 	char *pdu = queue->pdu;
+	char cbuf[CMSG_LEN(sizeof(char))] = {};
 	struct msghdr msg = {
+		.msg_control = cbuf,
+		.msg_controllen = sizeof(cbuf),
 		.msg_flags = MSG_DONTWAIT,
 	};
 	struct kvec iov = {
@@ -873,12 +877,17 @@ static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
 	if (ret <= 0)
 		return ret;
 
+	hdr = queue->pdu;
+	if (hdr->type == TLS_HANDSHAKE_KEYUPDATE) {
+		dev_err(queue->ctrl->ctrl.device, "KeyUpdate message\n");
+		return 1;
+	}
+
 	queue->pdu_remaining -= ret;
 	queue->pdu_offset += ret;
 	if (queue->pdu_remaining)
 		return 0;
 
-	hdr = queue->pdu;
 	if (unlikely(hdr->hlen != sizeof(struct nvme_tcp_rsp_pdu))) {
 		if (!nvme_tcp_recv_pdu_supported(hdr->type))
 			goto unsupported_pdu;
@@ -944,6 +953,7 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
 	struct request *rq =
 		nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
 	struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
+	char cbuf[CMSG_LEN(sizeof(char))] = {};
 
 	if (nvme_tcp_recv_state(queue) != NVME_TCP_RECV_DATA)
 		return 0;
@@ -973,12 +983,14 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
 		memset(&msg, 0, sizeof(msg));
 		msg.msg_iter = req->iter;
 		msg.msg_flags = MSG_DONTWAIT;
+		msg.msg_control = cbuf,
+		msg.msg_controllen = sizeof(cbuf),
 
 		ret = sock_recvmsg(queue->sock, &msg, msg.msg_flags);
 		if (ret < 0) {
-			dev_err(queue->ctrl->ctrl.device,
-				"queue %d failed to receive request %#x data",
-				nvme_tcp_queue_id(queue), rq->tag);
+			dev_dbg(queue->ctrl->ctrl.device,
+				"queue %d failed to receive request %#x data, %d",
+				nvme_tcp_queue_id(queue), rq->tag, ret);
 			return ret;
 		}
 		if (queue->data_digest)
@@ -1381,17 +1393,42 @@ static int nvme_tcp_try_recvmsg(struct nvme_tcp_queue *queue)
 		}
 	} while (result >= 0);
 
-	if (result < 0 && result != -EAGAIN) {
+	if (result == -EKEYEXPIRED) {
+		return -EKEYEXPIRED;
+	} else if (result == -EAGAIN) {
+		return -EAGAIN;
+	} else if (result < 0) {
 		dev_err(queue->ctrl->ctrl.device,
 			"receive failed:  %d\n", result);
 		queue->rd_enabled = false;
 		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
-	} else if (result == -EAGAIN)
-		result = 0;
+	}
 
 	return result < 0 ? result : (queue->nr_cqe = nr_cqe);
 }
 
+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);
+
+	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 =
@@ -1414,8 +1451,11 @@ static void nvme_tcp_io_work(struct work_struct *w)
 		result = nvme_tcp_try_recvmsg(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) &&
@@ -1723,6 +1763,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:
@@ -1752,6 +1793,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);
 	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
-- 
2.51.0

[PATCH v4 6/7] nvme-tcp: Allow userspace to trigger a KeyUpdate with debugfs

[alistair23]

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

Allow userspace to trigger a KeyUpdate via debugfs. This patch exposes a
key_update file that can be written to with the queue number to trigger
a KeyUpdate on that queue.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v4:
 - No change
v3:
 - New patch

 drivers/nvme/host/tcp.c | 72 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 791e0cc91ad8..f5c7b646d002 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -11,6 +11,7 @@
 #include <linux/crc32.h>
 #include <linux/nvme-tcp.h>
 #include <linux/nvme-keyring.h>
+#include <linux/debugfs.h>
 #include <net/sock.h>
 #include <net/tcp.h>
 #include <net/tls.h>
@@ -1429,6 +1430,75 @@ static void update_tls_keys(struct nvme_tcp_queue *queue)
 	}
 }
 
+#ifdef CONFIG_NVME_TCP_TLS
+#define NVME_DEBUGFS_RW_ATTR(field) \
+	static int field##_open(struct inode *inode, struct file *file) \
+	{ return single_open(file, field##_show, inode->i_private); } \
+	\
+	static const struct file_operations field##_fops = { \
+		.open = field##_open, \
+		.read = seq_read, \
+		.write = field##_write, \
+		.release = single_release, \
+	}
+
+static int nvme_ctrl_key_update_show(struct seq_file *m, void *p)
+{
+	seq_printf(m, "0\n");
+
+	return 0;
+}
+
+static ssize_t nvme_ctrl_key_update_write(struct file *file, const char __user *buf,
+					  size_t count, loff_t *ppos)
+{
+	struct seq_file *m = file->private_data;
+	struct nvme_ctrl *nctrl = m->private;
+	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);
+	char kbuf[16] = {0};
+	int queue_nr, rc;
+	struct nvme_tcp_queue *queue;
+
+	if (count > sizeof(kbuf) - 1)
+		return -EINVAL;
+	if (copy_from_user(kbuf, buf, count))
+		return -EFAULT;
+	kbuf[count] = 0;
+
+	rc = kstrtouint(kbuf, 10, &queue_nr);
+	if (rc)
+		return rc;
+
+	if (queue_nr >= nctrl->queue_count)
+		return -EINVAL;
+
+	queue = &ctrl->queues[queue_nr];
+
+	update_tls_keys(queue);
+
+	return count;
+}
+NVME_DEBUGFS_RW_ATTR(nvme_ctrl_key_update);
+#endif
+
+static void nvme_tcp_debugfs_init(struct nvme_ctrl *ctrl,
+			    const char *dev_name)
+{
+	struct dentry *parent;
+
+	/* create debugfs directory and attribute */
+	parent = debugfs_create_dir(dev_name, NULL);
+	if (IS_ERR(parent)) {
+		pr_warn("%s: failed to create debugfs directory\n", dev_name);
+		return;
+	}
+
+#ifdef CONFIG_NVME_TCP_TLS
+	debugfs_create_file("key_update", S_IWUSR, parent, ctrl,
+			    &nvme_ctrl_key_update_fops);
+#endif
+}
+
 static void nvme_tcp_io_work(struct work_struct *w)
 {
 	struct nvme_tcp_queue *queue =
@@ -3065,6 +3135,8 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	list_add_tail(&ctrl->list, &nvme_tcp_ctrl_list);
 	mutex_unlock(&nvme_tcp_ctrl_mutex);
 
+	nvme_tcp_debugfs_init(&ctrl->ctrl, dev_name(dev));
+
 	return &ctrl->ctrl;
 
 out_uninit_ctrl:
-- 
2.51.0

[PATCH v4 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>
---
v4:
 - Restructure code to avoid #ifdefs and forward declarations
 - Use a helper function for checking -EKEYEXPIRED
 - Remove all support for initiating KeyUpdate
 - Use helper function for restoring callbacks
v3:
 - Use a write lock for sk_user_data
 - Fix build with CONFIG_NVME_TARGET_TCP_TLS disabled
 - Remove unused variable
v2:
 - Use a helper function for KeyUpdates
 - Ensure keep alive timer is stopped
 - Wait for TLS KeyUpdate to complete

 drivers/nvme/target/tcp.c | 205 ++++++++++++++++++++++++++------------
 1 file changed, 143 insertions(+), 62 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 8aeec4a7f136..4ef25df2791a 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;
 
@@ -214,6 +217,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)
@@ -832,6 +839,23 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
 	return 1;
 }
 
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static bool nvmet_tls_key_expired(struct nvmet_tcp_queue *queue, int ret)
+{
+	if (ret == -EKEYEXPIRED &&
+	    queue->state != NVMET_TCP_Q_DISCONNECTING &&
+	    queue->state != NVMET_TCP_Q_TLS_HANDSHAKE)
+					return true;
+
+	return false;
+}
+#else
+static bool nvmet_tls_key_expired(struct nvmet_tcp_queue *queue, int ret)
+{
+	return false;
+}
+#endif
+
 static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
 		int budget, int *sends)
 {
@@ -1106,6 +1130,103 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
 	return false;
 }
 
+static void nvmet_tcp_release_queue(struct kref *kref)
+{
+	struct nvmet_tcp_queue *queue =
+		container_of(kref, struct nvmet_tcp_queue, kref);
+
+	WARN_ON(queue->state != NVMET_TCP_Q_DISCONNECTING);
+	queue_work(nvmet_wq, &queue->release_work);
+}
+
+static void nvmet_tcp_schedule_release_queue(struct nvmet_tcp_queue *queue)
+{
+	spin_lock_bh(&queue->state_lock);
+	if (queue->state == NVMET_TCP_Q_TLS_HANDSHAKE) {
+		/* Socket closed during handshake */
+		tls_handshake_cancel(queue->sock->sk);
+	}
+	if (queue->state != NVMET_TCP_Q_DISCONNECTING) {
+		queue->state = NVMET_TCP_Q_DISCONNECTING;
+		kref_put(&queue->kref, nvmet_tcp_release_queue);
+	}
+	spin_unlock_bh(&queue->state_lock);
+}
+
+static void nvmet_tcp_restore_socket_callbacks(struct nvmet_tcp_queue *queue)
+{
+	struct socket *sock = queue->sock;
+
+	if (!queue->state_change)
+		return;
+
+	write_lock_bh(&sock->sk->sk_callback_lock);
+	sock->sk->sk_data_ready =  queue->data_ready;
+	sock->sk->sk_state_change = queue->state_change;
+	sock->sk->sk_write_space = queue->write_space;
+	sock->sk->sk_user_data = NULL;
+	write_unlock_bh(&sock->sk->sk_callback_lock);
+}
+
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w)
+{
+	struct nvmet_tcp_queue *queue = container_of(to_delayed_work(w),
+			struct nvmet_tcp_queue, tls_handshake_tmo_work);
+
+	pr_warn("queue %d: TLS handshake timeout\n", queue->idx);
+	/*
+	 * If tls_handshake_cancel() fails we've lost the race with
+	 * nvmet_tcp_tls_handshake_done() */
+	if (!tls_handshake_cancel(queue->sock->sk))
+		return;
+	spin_lock_bh(&queue->state_lock);
+	if (WARN_ON(queue->state != NVMET_TCP_Q_TLS_HANDSHAKE)) {
+		spin_unlock_bh(&queue->state_lock);
+		return;
+	}
+	queue->state = NVMET_TCP_Q_FAILED;
+	spin_unlock_bh(&queue->state_lock);
+	nvmet_tcp_schedule_release_queue(queue);
+	kref_put(&queue->kref, nvmet_tcp_release_queue);
+}
+
+static int update_tls_keys(struct nvmet_tcp_queue *queue)
+{
+	int ret;
+
+	cancel_work(&queue->io_work);
+	queue->state = NVMET_TCP_Q_TLS_HANDSHAKE;
+
+	nvmet_tcp_restore_socket_callbacks(queue);
+
+	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 0;
+}
+#else
+static int update_tls_keys(struct nvmet_tcp_queue *queue)
+{
+	return -EPFNOSUPPORT;
+}
+#endif
+
 static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
 		struct msghdr *msg, char *cbuf)
 {
@@ -1131,6 +1252,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",
@@ -1340,6 +1464,8 @@ 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)) {
+			if (nvmet_tls_key_expired(queue, ret))
+					goto done;
 			nvmet_tcp_socket_error(queue, ret);
 			goto done;
 		} else if (ret == 0) {
@@ -1351,29 +1477,6 @@ static int nvmet_tcp_try_recv(struct nvmet_tcp_queue *queue,
 	return ret;
 }
 
-static void nvmet_tcp_release_queue(struct kref *kref)
-{
-	struct nvmet_tcp_queue *queue =
-		container_of(kref, struct nvmet_tcp_queue, kref);
-
-	WARN_ON(queue->state != NVMET_TCP_Q_DISCONNECTING);
-	queue_work(nvmet_wq, &queue->release_work);
-}
-
-static void nvmet_tcp_schedule_release_queue(struct nvmet_tcp_queue *queue)
-{
-	spin_lock_bh(&queue->state_lock);
-	if (queue->state == NVMET_TCP_Q_TLS_HANDSHAKE) {
-		/* Socket closed during handshake */
-		tls_handshake_cancel(queue->sock->sk);
-	}
-	if (queue->state != NVMET_TCP_Q_DISCONNECTING) {
-		queue->state = NVMET_TCP_Q_DISCONNECTING;
-		kref_put(&queue->kref, nvmet_tcp_release_queue);
-	}
-	spin_unlock_bh(&queue->state_lock);
-}
-
 static inline void nvmet_tcp_arm_queue_deadline(struct nvmet_tcp_queue *queue)
 {
 	queue->poll_end = jiffies + usecs_to_jiffies(idle_poll_period_usecs);
@@ -1404,8 +1507,12 @@ 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)
+		else if (ret < 0) {
+			if (ret == -EKEYEXPIRED)
+				break;
+
 			return;
+		}
 
 		ret = nvmet_tcp_try_send(queue, NVMET_TCP_SEND_BUDGET, &ops);
 		if (ret > 0)
@@ -1415,6 +1522,11 @@ static void nvmet_tcp_io_work(struct work_struct *w)
 
 	} while (pending && ops < NVMET_TCP_IO_WORK_BUDGET);
 
+	if (ret == -EKEYEXPIRED) {
+		update_tls_keys(queue);
+		pending = true;
+	}
+
 	/*
 	 * Requeue the worker if idle deadline period is in progress or any
 	 * ops activity was recorded during the do-while loop above.
@@ -1517,21 +1629,6 @@ static void nvmet_tcp_free_cmds(struct nvmet_tcp_queue *queue)
 	kfree(cmds);
 }
 
-static void nvmet_tcp_restore_socket_callbacks(struct nvmet_tcp_queue *queue)
-{
-	struct socket *sock = queue->sock;
-
-	if (!queue->state_change)
-		return;
-
-	write_lock_bh(&sock->sk->sk_callback_lock);
-	sock->sk->sk_data_ready =  queue->data_ready;
-	sock->sk->sk_state_change = queue->state_change;
-	sock->sk->sk_write_space = queue->write_space;
-	sock->sk->sk_user_data = NULL;
-	write_unlock_bh(&sock->sk->sk_callback_lock);
-}
-
 static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue)
 {
 	struct nvmet_tcp_cmd *cmd = queue->cmds;
@@ -1794,6 +1891,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;
@@ -1809,32 +1907,11 @@ 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);
-}
-
-static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w)
-{
-	struct nvmet_tcp_queue *queue = container_of(to_delayed_work(w),
-			struct nvmet_tcp_queue, tls_handshake_tmo_work);
-
-	pr_warn("queue %d: TLS handshake timeout\n", queue->idx);
-	/*
-	 * If tls_handshake_cancel() fails we've lost the race with
-	 * nvmet_tcp_tls_handshake_done() */
-	if (!tls_handshake_cancel(queue->sock->sk))
-		return;
-	spin_lock_bh(&queue->state_lock);
-	if (WARN_ON(queue->state != NVMET_TCP_Q_TLS_HANDSHAKE)) {
-		spin_unlock_bh(&queue->state_lock);
-		return;
-	}
-	queue->state = NVMET_TCP_Q_FAILED;
-	spin_unlock_bh(&queue->state_lock);
-	nvmet_tcp_schedule_release_queue(queue);
-	kref_put(&queue->kref, nvmet_tcp_release_queue);
+	complete(&queue->tls_complete);
 }
 
 static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue,
-	handshake_key_update_type keyupdate)
+				   handshake_key_update_type keyupdate)
 {
 	int ret = -EOPNOTSUPP;
 	struct tls_handshake_args args;
@@ -1852,11 +1929,15 @@ 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);
 
 	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
 		ret = tls_server_hello_psk(&args, GFP_KERNEL);
 	else
 		ret = tls_server_keyupdate_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);
-- 
2.51.0

Re: [PATCH v4 5/7] nvme-tcp: Support KeyUpdate

[Christoph Hellwig]

On Fri, Oct 17, 2025 at 02:23:10PM +1000, 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

Totally independent of the current Link-tag flamewar, spec reference
should be in the free flowing commit text.

> +	if (result == -EKEYEXPIRED) {
> +		return -EKEYEXPIRED;
> +	} else if (result == -EAGAIN) {
> +		return -EAGAIN;
> +	} else if (result < 0) {

returns do not need and should not be followed by else statements.

>  		dev_err(queue->ctrl->ctrl.device,
>  			"receive failed:  %d\n", result);
>  		queue->rd_enabled = false;
>  		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> +	}
>  
>  	return result < 0 ? result : (queue->nr_cqe = nr_cqe);

Also the overall flow here, but old and newly added feels really odd,
up to the point of intentional obfuscation in the last return line.

I'd expect this to be more something like:

	if (result < 0) {
		if (result != -EKEYEXPIRED && result != -EAGAIN) {
	 		dev_err(queue->ctrl->ctrl.device,
	 			"receive failed:  %d\n", result);
	 		queue->rd_enabled = false;
	  		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
		}
		return result;
	}

	queue->nr_cqe = nr_cqe;
	return nr_cqe;
}

Re: [PATCH v4 6/7] nvme-tcp: Allow userspace to trigger a KeyUpdate with debugfs

[Christoph Hellwig]

On Fri, Oct 17, 2025 at 02:23:11PM +1000, alistair23@gmail.com wrote:
> +#ifdef CONFIG_NVME_TCP_TLS
> +#define NVME_DEBUGFS_RW_ATTR(field) \
> +	static int field##_open(struct inode *inode, struct file *file) \
> +	{ return single_open(file, field##_show, inode->i_private); } \

Please use normal indentation in the macro, but do not add an extra
level of indentation just for being inside a macro.

Then again the macro is only used once anyway, so why add it at all?

> +static ssize_t nvme_ctrl_key_update_write(struct file *file, const char __user *buf,

Please avoid the overly long line.

> +					  size_t count, loff_t *ppos)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct nvme_ctrl *nctrl = m->private;
> +	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);
> +	char kbuf[16] = {0};
> +	int queue_nr, rc;
> +	struct nvme_tcp_queue *queue;
> +
> +	if (count > sizeof(kbuf) - 1)
> +		return -EINVAL;
> +	if (copy_from_user(kbuf, buf, count))
> +		return -EFAULT;
> +	kbuf[count] = 0;
> +
> +	rc = kstrtouint(kbuf, 10, &queue_nr);
> +	if (rc)
> +		return rc;

kstrtoint_from_user?

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

[Chuck Lever]

On 10/17/25 12:23 AM, alistair23@gmail.com wrote:
> 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>
> Reviewed-by: Hannes Reincke <hare@suse.de>
> ---
> v4:
>  - No change
> v3:
>  - No change
> v2:
>  - Change "key-serial" to "session-id"
> 
>  Documentation/netlink/specs/handshake.yaml |  4 ++++
>  Documentation/networking/tls-handshake.rst |  2 ++
>  drivers/nvme/host/tcp.c                    |  3 ++-
>  drivers/nvme/target/tcp.c                  |  3 ++-
>  include/net/handshake.h                    |  4 +++-
>  include/uapi/linux/handshake.h             |  1 +
>  net/handshake/genl.c                       |  5 +++--
>  net/handshake/tlshd.c                      | 15 +++++++++++++--
>  net/sunrpc/svcsock.c                       |  4 +++-
>  net/sunrpc/xprtsock.c                      |  4 +++-
>  10 files changed, 36 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/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
> index 6f5ea1646a47..d7287890056a 100644
> --- a/Documentation/networking/tls-handshake.rst
> +++ b/Documentation/networking/tls-handshake.rst
> @@ -60,6 +60,8 @@ fills in a structure that contains the parameters of the request:
>          key_serial_t    ta_my_privkey;
>          unsigned int    ta_num_peerids;
>          key_serial_t    ta_my_peerids[5];
> +        key_serial_t    user_session_id;
> +
>    };

No need for the extra blank line here.


>  
>  The @ta_sock field references an open and connected socket. The consumer
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 2751c15beed6..611be56f8013 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1691,7 +1691,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..dc2222fd6d99 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;
> @@ -31,6 +32,7 @@ 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);
> 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..2549c5dbccd8 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_session_id;

Here (and below), "userspace session ID" had me confused. There isn't
a user here; IIRC, it refers to a session ID for tlshd to track?

Something like "handshake session ID" might be more precise?


>  };
>  
>  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_session_id = 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 7b90abc5cf0e..bc9a713c6559 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -444,13 +444,15 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
>   * @data: address of xprt to wake
>   * @status: status of handshake
>   * @peerid: serial number of key containing the remote peer's identity
> + * @user_session_id: serial number of the userspace session ID
>   *
>   * If a security policy is specified as an export option, we don't
>   * have a specific export here to check. So we set a "TLS session
>   * 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 3aa987e7f072..bce0f43bef65 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2589,9 +2589,11 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
>   * @data: address of xprt to wake
>   * @status: status of handshake
>   * @peerid: serial number of key containing the remote's identity
> + * @user_session_id: serial number of the userspace session ID
>   *
>   */
> -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 =


-- 
Chuck Lever

Re: [PATCH v4 4/7] net/handshake: Support KeyUpdate message types

[Hannes Reinecke]

On 10/17/25 06:23, 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>
> ---
> v4:
>   - Don't overload existing functions, instead create new ones
> v3:
>   - Fixup yamllint and kernel-doc failures
> 
>   Documentation/netlink/specs/handshake.yaml | 16 ++++-
>   drivers/nvme/host/tcp.c                    | 15 +++-
>   drivers/nvme/target/tcp.c                  | 10 ++-
>   include/net/handshake.h                    |  8 +++
>   include/uapi/linux/handshake.h             | 13 ++++
>   net/handshake/tlshd.c                      | 83 +++++++++++++++++++++-
>   6 files changed, 137 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> index a273bc74d26f..c72ec8fa7d7a 100644
> --- a/Documentation/netlink/specs/handshake.yaml
> +++ b/Documentation/netlink/specs/handshake.yaml
> @@ -21,12 +21,18 @@ definitions:
>       type: enum
>       name: msg-type
>       value-start: 0
> -    entries: [unspec, clienthello, serverhello]
> +    entries: [unspec, clienthello, serverhello, clientkeyupdate,
> +              clientkeyupdaterequest, serverkeyupdate, serverkeyupdaterequest]
>     -

Why do we need the 'keyupdate' and 'keyupdaterequest' types?
Isn't the 'keyupdate' type enough, and can we specify anything
else via the update type?

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

See above.

>   
>   attribute-sets:
>     -
> @@ -74,6 +80,13 @@ attribute-sets:
>         -
>           name: keyring
>           type: u32
> +      -
> +        name: key-update-request
> +        type: u32
> +        enum: key-update-type
> +      -
> +        name: key-serial
> +        type: u32

Not sure if I like key-serial. Yes, it is a key serial number,
but it's not the serial number of the updated key (rather the serial
number of the key holding the session information).
Maybe 'key-update-serial' ?

>     -
>       name: done
>       attributes:
> @@ -116,6 +129,7 @@ operations:
>               - certificate
>               - peername
>               - keyring
> +            - key-serial
>       -
>         name: done
>         doc: Handler reports handshake completion
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 611be56f8013..2696bf97dfac 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -20,6 +20,7 @@
>   #include <linux/iov_iter.h>
>   #include <net/busy_poll.h>
>   #include <trace/events/sock.h>
> +#include <uapi/linux/handshake.h>
>   
>   #include "nvme.h"
>   #include "fabrics.h"
> @@ -206,6 +207,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)
>   {
> @@ -1726,7 +1731,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;
> @@ -1748,7 +1754,10 @@ 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);
> +	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> +		ret = tls_client_hello_psk(&args, GFP_KERNEL);
> +	else
> +		ret = tls_client_keyupdate_psk(&args, GFP_KERNEL, keyupdate);
>   	if (ret) {
>   		dev_err(nctrl->device, "queue %d: failed to start TLS: %d\n",
>   			qid, ret);
> @@ -1898,7 +1907,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..8aeec4a7f136 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1833,7 +1833,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 +1853,10 @@ 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);
> +	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> +		ret = tls_server_hello_psk(&args, GFP_KERNEL);
> +	else
> +		ret = tls_server_keyupdate_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 +1938,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 dc2222fd6d99..084c92a20b68 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
> +
Huh?
You define it as 'u32' here

>   enum {
>   	TLS_NO_KEYRING = 0,
>   	TLS_NO_PEERID = 0,
> @@ -38,8 +42,12 @@ struct tls_handshake_args {
>   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_keyupdate_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_keyupdate_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,
> +};
> +

and here it's an enum. Please kill the first declaration.

>   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 2549c5dbccd8..c40839977ab9 100644
> --- a/net/handshake/tlshd.c
> +++ b/net/handshake/tlshd.c
> @@ -41,6 +41,7 @@ struct tls_handshake_req {
>   	unsigned int		th_num_peerids;
>   	key_serial_t		th_peerid[5];
>   
> +	int			th_key_update_request;
>   	key_serial_t		user_session_id;
>   };
>   
Why 'int' ? Can it be negative?
If not please make it an 'unsigned int'

> @@ -58,7 +59,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_session_id = TLS_NO_PRIVKEY;
> +	treq->user_session_id = args->user_session_id;
> +
>   	return treq;
>   }
>   
> @@ -265,6 +267,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);
>   
> @@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
>   }
>   EXPORT_SYMBOL(tls_client_hello_psk);
>   
> +/**
> + * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
> + * @args: socket and handshake parameters for this request
> + * @flags: memory allocation control flags
> + * @keyupdate: specifies the type of KeyUpdate operation
> + *
> + * Return values:
> + *   %0: Handshake request enqueue; ->done will be called when complete
> + *   %-EINVAL: Wrong number of local peer IDs
> + *   %-ESRCH: No user agent is available
> + *   %-ENOMEM: Memory allocation failed
> + */
> +int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> +			     handshake_key_update_type keyupdate)
> +{
> +	struct tls_handshake_req *treq;
> +	struct handshake_req *req;
> +	unsigned int i;
> +
> +	if (!args->ta_num_peerids ||
> +	    args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
> +		return -EINVAL;
> +
> +	req = handshake_req_alloc(&tls_handshake_proto, flags);
> +	if (!req)
> +		return -ENOMEM;
> +	treq = tls_handshake_req_init(req, args);
> +	treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
> +	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++)
> +		treq->th_peerid[i] = args->ta_my_peerids[i];
Hmm?
Do we use the 'peerids'?
I thought that the information was encoded in the session, ie
the 'user_session_id' ?

> +
> +	return handshake_req_submit(args->ta_sock, req, flags);
> +}
> +EXPORT_SYMBOL(tls_client_keyupdate_psk);
> +
>   /**
>    * tls_server_hello_x509 - request a server TLS handshake on a socket
>    * @args: socket and handshake parameters for this request
> @@ -428,6 +478,37 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
>   }
>   EXPORT_SYMBOL(tls_server_hello_psk);
>   
> +/**
> + * tls_server_keyupdate_psk - request a server TLS KeyUpdate on a socket
> + * @args: socket and handshake parameters for this request
> + * @flags: memory allocation control flags
> + * @keyupdate: specifies the type of KeyUpdate operation
> + *
> + * Return values:
> + *   %0: Handshake request enqueue; ->done will be called when complete
> + *   %-ESRCH: No user agent is available
> + *   %-ENOMEM: Memory allocation failed
> + */
> +int tls_server_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> +			     handshake_key_update_type keyupdate)
> +{
> +	struct tls_handshake_req *treq;
> +	struct handshake_req *req;
> +
> +	req = handshake_req_alloc(&tls_handshake_proto, flags);
> +	if (!req)
> +		return -ENOMEM;
> +	treq = tls_handshake_req_init(req, args);
> +	treq->th_type = HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE;
> +	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];

Same here. Why do we need to set 'peerid'?

> +
> +	return handshake_req_submit(args->ta_sock, req, flags);
> +}
> +EXPORT_SYMBOL(tls_server_keyupdate_psk);
> +
>   /**
>    * tls_handshake_cancel - cancel a pending handshake
>    * @sk: socket on which there is an ongoing handshake
Nit: we _could_ overload 'peerid' with the user_session_id,then we 
wouldn't need to specify a new field in the handshake
request.
But that's arguably quite hackish.

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 v4 5/7] nvme-tcp: Support KeyUpdate

[Hannes Reinecke]

On 10/17/25 06:23, 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>
> ---
> v4:
>   - Remove all support for initiating KeyUpdate
>   - Don't call cancel_work() when updating keys
> v3:
>   - Don't cancel existing handshake requests
> 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 | 60 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 2696bf97dfac..791e0cc91ad8 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -172,6 +172,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;
> @@ -858,7 +859,10 @@ static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue,
>   static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
>   {
>   	char *pdu = queue->pdu;
> +	char cbuf[CMSG_LEN(sizeof(char))] = {};
>   	struct msghdr msg = {
> +		.msg_control = cbuf,
> +		.msg_controllen = sizeof(cbuf),
>   		.msg_flags = MSG_DONTWAIT,
>   	};
>   	struct kvec iov = {
> @@ -873,12 +877,17 @@ static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
>   	if (ret <= 0)
>   		return ret;
>   
> +	hdr = queue->pdu;
> +	if (hdr->type == TLS_HANDSHAKE_KEYUPDATE) {
> +		dev_err(queue->ctrl->ctrl.device, "KeyUpdate message\n");
> +		return 1;
> +	}
> +
>   	queue->pdu_remaining -= ret;
>   	queue->pdu_offset += ret;
>   	if (queue->pdu_remaining)
>   		return 0;
>   
> -	hdr = queue->pdu;
>   	if (unlikely(hdr->hlen != sizeof(struct nvme_tcp_rsp_pdu))) {
>   		if (!nvme_tcp_recv_pdu_supported(hdr->type))
>   			goto unsupported_pdu;
> @@ -944,6 +953,7 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
>   	struct request *rq =
>   		nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
>   	struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> +	char cbuf[CMSG_LEN(sizeof(char))] = {};
>   
>   	if (nvme_tcp_recv_state(queue) != NVME_TCP_RECV_DATA)
>   		return 0;
> @@ -973,12 +983,14 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
>   		memset(&msg, 0, sizeof(msg));
>   		msg.msg_iter = req->iter;
>   		msg.msg_flags = MSG_DONTWAIT;
> +		msg.msg_control = cbuf,
> +		msg.msg_controllen = sizeof(cbuf),
>   
Watch out. This is the recvmsg bug Olga had been posting patches for.
Thing is, if there is a control message the networking code will place
the control message payload into the message buffer. But in doing so
it expects the message buffer to be an iovec, not a bio vec.

To handle this properly you'd need to _not_ set the control buffer,
but rather check for 'MSG_CTRUNC' in msg_flags upon return.
Then you have to setup a new message with msg_control set and
a suitable msg_len (5 bytes, wasn't it?) and re-issue recvmsg
with that message.

And keep fingers crossed that you don't get MSG_CTRUNC on every
call to recvmsg() ...

>   		ret = sock_recvmsg(queue->sock, &msg, msg.msg_flags);
>   		if (ret < 0) {
> -			dev_err(queue->ctrl->ctrl.device,
> -				"queue %d failed to receive request %#x data",
> -				nvme_tcp_queue_id(queue), rq->tag);
> +			dev_dbg(queue->ctrl->ctrl.device,
> +				"queue %d failed to receive request %#x data, %d",
> +				nvme_tcp_queue_id(queue), rq->tag, ret);
>   			return ret;
>   		}
>   		if (queue->data_digest)
> @@ -1381,17 +1393,42 @@ static int nvme_tcp_try_recvmsg(struct nvme_tcp_queue *queue)
>   		}
>   	} while (result >= 0);
>   
> -	if (result < 0 && result != -EAGAIN) {
> +	if (result == -EKEYEXPIRED) {
> +		return -EKEYEXPIRED;
> +	} else if (result == -EAGAIN) {
> +		return -EAGAIN;
> +	} else if (result < 0) {
>   		dev_err(queue->ctrl->ctrl.device,
>   			"receive failed:  %d\n", result);
>   		queue->rd_enabled = false;
>   		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> -	} else if (result == -EAGAIN)
> -		result = 0;
> +	}
>   
>   	return result < 0 ? result : (queue->nr_cqe = nr_cqe);
>   }
>   
> +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);
> +
> +	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 =
> @@ -1414,8 +1451,11 @@ static void nvme_tcp_io_work(struct work_struct *w)
>   		result = nvme_tcp_try_recvmsg(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) &&
> @@ -1723,6 +1763,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;

Hmm. I wonder, do we need to store the generation number somewhere?
Currently the sysfs interface is completely oblivious that a key update
has happened. I really would like to have _some_ indicator there telling
us that a key update had happened, and the generation number would be
ideal here.

>   	}
>   
>   out_complete:
> @@ -1752,6 +1793,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);
>   	if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)

Chers,
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 v4 7/7] nvmet-tcp: Support KeyUpdate

[Hannes Reinecke]

On 10/17/25 06:23, alistair23@gmail.com 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>
> ---
> v4:
>   - Restructure code to avoid #ifdefs and forward declarations
>   - Use a helper function for checking -EKEYEXPIRED
>   - Remove all support for initiating KeyUpdate
>   - Use helper function for restoring callbacks
> v3:
>   - Use a write lock for sk_user_data
>   - Fix build with CONFIG_NVME_TARGET_TCP_TLS disabled
>   - Remove unused variable
> v2:
>   - Use a helper function for KeyUpdates
>   - Ensure keep alive timer is stopped
>   - Wait for TLS KeyUpdate to complete
> 
>   drivers/nvme/target/tcp.c | 205 ++++++++++++++++++++++++++------------
>   1 file changed, 143 insertions(+), 62 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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 v4 6/7] nvme-tcp: Allow userspace to trigger a KeyUpdate with debugfs

[Hannes Reinecke]

On 10/17/25 06:23, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Allow userspace to trigger a KeyUpdate via debugfs. This patch exposes a
> key_update file that can be written to with the queue number to trigger
> a KeyUpdate on that queue.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v4:
>   - No change
> v3:
>   - New patch
> 
>   drivers/nvme/host/tcp.c | 72 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 791e0cc91ad8..f5c7b646d002 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -11,6 +11,7 @@
>   #include <linux/crc32.h>
>   #include <linux/nvme-tcp.h>
>   #include <linux/nvme-keyring.h>
> +#include <linux/debugfs.h>
>   #include <net/sock.h>
>   #include <net/tcp.h>
>   #include <net/tls.h>
> @@ -1429,6 +1430,75 @@ static void update_tls_keys(struct nvme_tcp_queue *queue)
>   	}
>   }
>   
> +#ifdef CONFIG_NVME_TCP_TLS
> +#define NVME_DEBUGFS_RW_ATTR(field) \
> +	static int field##_open(struct inode *inode, struct file *file) \
> +	{ return single_open(file, field##_show, inode->i_private); } \
> +	\
> +	static const struct file_operations field##_fops = { \
> +		.open = field##_open, \
> +		.read = seq_read, \
> +		.write = field##_write, \
> +		.release = single_release, \
> +	}
> +
> +static int nvme_ctrl_key_update_show(struct seq_file *m, void *p)
> +{
> +	seq_printf(m, "0\n");
> +
> +	return 0;
> +}
> +
> +static ssize_t nvme_ctrl_key_update_write(struct file *file, const char __user *buf,
> +					  size_t count, loff_t *ppos)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct nvme_ctrl *nctrl = m->private;
> +	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);
> +	char kbuf[16] = {0};
> +	int queue_nr, rc;
> +	struct nvme_tcp_queue *queue;
> +
> +	if (count > sizeof(kbuf) - 1)
> +		return -EINVAL;
> +	if (copy_from_user(kbuf, buf, count))
> +		return -EFAULT;
> +	kbuf[count] = 0;
> +
> +	rc = kstrtouint(kbuf, 10, &queue_nr);
> +	if (rc)
> +		return rc;
> +
> +	if (queue_nr >= nctrl->queue_count)
> +		return -EINVAL;
> +
> +	queue = &ctrl->queues[queue_nr];
> +
> +	update_tls_keys(queue);

Are you sure this is correct?

'update_tls_keys' is issuing a handshake request
with 'HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED'.

And the patch introducing it states:
At this time we don't support initiating a KeyUpdate.

So please move this to the patchset implementing support
for initiating KeyUpdates.

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 v4 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Hannes Reinecke]

On 10/17/25 06:23, 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.
> 

I am rather sceptical at the current tlshd implementation.
At which place do you update the sending keys?
I'm only seeing a call to 'gnutls_handhake_update_receiving_key()'.

But I haven't found the matching function 
'gnutls_handshake_update_sending_key()' in current gnutls.
So how does updating of the sending keys 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 v4 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Alistair Francis]

On Tue, Oct 21, 2025 at 3:46 AM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/17/25 06:23, 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.
> >
>
> I am rather sceptical at the current tlshd implementation.
> At which place do you update the sending keys?

The sending keys are updated as part of gnutls_session_key_update().

gnutls_session_key_update() calls update_sending_key() which updates
the sending keys.

The idea is that when the sequence number is about to overflow the
kernel will request userspace to update the sending keys via the
HANDSHAKE_KEY_UPDATE_TYPE_SEND key_update_type. Userspace updates the
keys and initiates a KeyUpdate.

> I'm only seeing a call to 'gnutls_handhake_update_receiving_key()'.
>
> But I haven't found the matching function
> 'gnutls_handshake_update_sending_key()' in current gnutls.
> So how does updating of the sending keys work?

gnutls_session_key_update() calls update_sending_key() which updates
the sending keys.

When updating the sending keys we want to send a KeyUpdate request,
which is why it's a different flow.

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 v4 4/7] net/handshake: Support KeyUpdate message types

[Alistair Francis]

On Mon, Oct 20, 2025 at 4:09 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/17/25 06:23, 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>
> > ---
> > v4:
> >   - Don't overload existing functions, instead create new ones
> > v3:
> >   - Fixup yamllint and kernel-doc failures
> >
> >   Documentation/netlink/specs/handshake.yaml | 16 ++++-
> >   drivers/nvme/host/tcp.c                    | 15 +++-
> >   drivers/nvme/target/tcp.c                  | 10 ++-
> >   include/net/handshake.h                    |  8 +++
> >   include/uapi/linux/handshake.h             | 13 ++++
> >   net/handshake/tlshd.c                      | 83 +++++++++++++++++++++-
> >   6 files changed, 137 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> > index a273bc74d26f..c72ec8fa7d7a 100644
> > --- a/Documentation/netlink/specs/handshake.yaml
> > +++ b/Documentation/netlink/specs/handshake.yaml
> > @@ -21,12 +21,18 @@ definitions:
> >       type: enum
> >       name: msg-type
> >       value-start: 0
> > -    entries: [unspec, clienthello, serverhello]
> > +    entries: [unspec, clienthello, serverhello, clientkeyupdate,
> > +              clientkeyupdaterequest, serverkeyupdate, serverkeyupdaterequest]
> >     -
>
> Why do we need the 'keyupdate' and 'keyupdaterequest' types?

msg-type indicates if it's a client or server and hello or keyupdate,
the idea being

client:
 - Hello
 - KeyUpdate

server:
 - Hello
 - KeyUpdate

I'll drop clientkeyupdaterequest and serverkeyupdaterequest

> Isn't the 'keyupdate' type enough, and can we specify anything
> else via the update type?

Once we know if it's a client or server KeyUpdate we need to know if
we are receiving one, sending one or receiving one with the
request_update flag set, hence key-update-type

>
> >       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]
>
> See above.
>
> >
> >   attribute-sets:
> >     -
> > @@ -74,6 +80,13 @@ attribute-sets:
> >         -
> >           name: keyring
> >           type: u32
> > +      -
> > +        name: key-update-request
> > +        type: u32
> > +        enum: key-update-type
> > +      -
> > +        name: key-serial
> > +        type: u32
>
> Not sure if I like key-serial. Yes, it is a key serial number,
> but it's not the serial number of the updated key (rather the serial
> number of the key holding the session information).
> Maybe 'key-update-serial' ?
>
> >     -
> >       name: done
> >       attributes:
> > @@ -116,6 +129,7 @@ operations:
> >               - certificate
> >               - peername
> >               - keyring
> > +            - key-serial
> >       -
> >         name: done
> >         doc: Handler reports handshake completion
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 611be56f8013..2696bf97dfac 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/iov_iter.h>
> >   #include <net/busy_poll.h>
> >   #include <trace/events/sock.h>
> > +#include <uapi/linux/handshake.h>
> >
> >   #include "nvme.h"
> >   #include "fabrics.h"
> > @@ -206,6 +207,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)
> >   {
> > @@ -1726,7 +1731,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;
> > @@ -1748,7 +1754,10 @@ 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);
> > +     if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> > +             ret = tls_client_hello_psk(&args, GFP_KERNEL);
> > +     else
> > +             ret = tls_client_keyupdate_psk(&args, GFP_KERNEL, keyupdate);
> >       if (ret) {
> >               dev_err(nctrl->device, "queue %d: failed to start TLS: %d\n",
> >                       qid, ret);
> > @@ -1898,7 +1907,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..8aeec4a7f136 100644
> > --- a/drivers/nvme/target/tcp.c
> > +++ b/drivers/nvme/target/tcp.c
> > @@ -1833,7 +1833,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 +1853,10 @@ 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);
> > +     if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> > +             ret = tls_server_hello_psk(&args, GFP_KERNEL);
> > +     else
> > +             ret = tls_server_keyupdate_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 +1938,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 dc2222fd6d99..084c92a20b68 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
> > +
> Huh?
> You define it as 'u32' here
>
> >   enum {
> >       TLS_NO_KEYRING = 0,
> >       TLS_NO_PEERID = 0,
> > @@ -38,8 +42,12 @@ struct tls_handshake_args {
> >   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_keyupdate_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_keyupdate_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,
> > +};
> > +
>
> and here it's an enum. Please kill the first declaration.
>
> >   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 2549c5dbccd8..c40839977ab9 100644
> > --- a/net/handshake/tlshd.c
> > +++ b/net/handshake/tlshd.c
> > @@ -41,6 +41,7 @@ struct tls_handshake_req {
> >       unsigned int            th_num_peerids;
> >       key_serial_t            th_peerid[5];
> >
> > +     int                     th_key_update_request;
> >       key_serial_t            user_session_id;
> >   };
> >
> Why 'int' ? Can it be negative?
> If not please make it an 'unsigned int'
>
> > @@ -58,7 +59,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_session_id = TLS_NO_PRIVKEY;
> > +     treq->user_session_id = args->user_session_id;
> > +
> >       return treq;
> >   }
> >
> > @@ -265,6 +267,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);
> >
> > @@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> >   }
> >   EXPORT_SYMBOL(tls_client_hello_psk);
> >
> > +/**
> > + * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
> > + * @args: socket and handshake parameters for this request
> > + * @flags: memory allocation control flags
> > + * @keyupdate: specifies the type of KeyUpdate operation
> > + *
> > + * Return values:
> > + *   %0: Handshake request enqueue; ->done will be called when complete
> > + *   %-EINVAL: Wrong number of local peer IDs
> > + *   %-ESRCH: No user agent is available
> > + *   %-ENOMEM: Memory allocation failed
> > + */
> > +int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> > +                          handshake_key_update_type keyupdate)
> > +{
> > +     struct tls_handshake_req *treq;
> > +     struct handshake_req *req;
> > +     unsigned int i;
> > +
> > +     if (!args->ta_num_peerids ||
> > +         args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
> > +             return -EINVAL;
> > +
> > +     req = handshake_req_alloc(&tls_handshake_proto, flags);
> > +     if (!req)
> > +             return -ENOMEM;
> > +     treq = tls_handshake_req_init(req, args);
> > +     treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
> > +     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++)
> > +             treq->th_peerid[i] = args->ta_my_peerids[i];
> Hmm?
> Do we use the 'peerids'?

We don't, this is just copied from the
tls_client_hello_psk()/tls_server_hello_psk() to provide the same
information to keep things more consistent.

I can remove setting these

> I thought that the information was encoded in the session, ie
> the 'user_session_id' ?
>
> > +
> > +     return handshake_req_submit(args->ta_sock, req, flags);
> > +}
> > +EXPORT_SYMBOL(tls_client_keyupdate_psk);
> > +
> >   /**
> >    * tls_server_hello_x509 - request a server TLS handshake on a socket
> >    * @args: socket and handshake parameters for this request
> > @@ -428,6 +478,37 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> >   }
> >   EXPORT_SYMBOL(tls_server_hello_psk);
> >
> > +/**
> > + * tls_server_keyupdate_psk - request a server TLS KeyUpdate on a socket
> > + * @args: socket and handshake parameters for this request
> > + * @flags: memory allocation control flags
> > + * @keyupdate: specifies the type of KeyUpdate operation
> > + *
> > + * Return values:
> > + *   %0: Handshake request enqueue; ->done will be called when complete
> > + *   %-ESRCH: No user agent is available
> > + *   %-ENOMEM: Memory allocation failed
> > + */
> > +int tls_server_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> > +                          handshake_key_update_type keyupdate)
> > +{
> > +     struct tls_handshake_req *treq;
> > +     struct handshake_req *req;
> > +
> > +     req = handshake_req_alloc(&tls_handshake_proto, flags);
> > +     if (!req)
> > +             return -ENOMEM;
> > +     treq = tls_handshake_req_init(req, args);
> > +     treq->th_type = HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE;
> > +     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];
>
> Same here. Why do we need to set 'peerid'?
>
> > +
> > +     return handshake_req_submit(args->ta_sock, req, flags);
> > +}
> > +EXPORT_SYMBOL(tls_server_keyupdate_psk);
> > +
> >   /**
> >    * tls_handshake_cancel - cancel a pending handshake
> >    * @sk: socket on which there is an ongoing handshake
> Nit: we _could_ overload 'peerid' with the user_session_id,then we
> wouldn't need to specify a new field in the handshake
> request.
> But that's arguably quite hackish.

Oh no! Let's not do that. That just seems prone to confusion

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 v4 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Hannes Reinecke]

On 10/21/25 03:01, Alistair Francis wrote:
> On Tue, Oct 21, 2025 at 3:46 AM Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 10/17/25 06:23, 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.
>>>
>>
>> I am rather sceptical at the current tlshd implementation.
>> At which place do you update the sending keys?
> 
> The sending keys are updated as part of gnutls_session_key_update().
> 
> gnutls_session_key_update() calls update_sending_key() which updates
> the sending keys.
> 
> The idea is that when the sequence number is about to overflow the
> kernel will request userspace to update the sending keys via the
> HANDSHAKE_KEY_UPDATE_TYPE_SEND key_update_type. Userspace updates the
> keys and initiates a KeyUpdate.
> 
That's also what the spec says.
But in order to do that we would need to get hold of the sequence
number, which currently is internal to gnutls.
Can we extract it from the session information?
And can we display it in sysfs, to give users information
whether a KeyUpdate had happened?

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 v4 5/7] nvme-tcp: Support KeyUpdate

[Alistair Francis]

On Mon, Oct 20, 2025 at 4:22 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/17/25 06:23, 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>
> > ---
> > v4:
> >   - Remove all support for initiating KeyUpdate
> >   - Don't call cancel_work() when updating keys
> > v3:
> >   - Don't cancel existing handshake requests
> > 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 | 60 ++++++++++++++++++++++++++++++++++-------
> >   1 file changed, 51 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 2696bf97dfac..791e0cc91ad8 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -172,6 +172,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;
> > @@ -858,7 +859,10 @@ static void nvme_tcp_handle_c2h_term(struct nvme_tcp_queue *queue,
> >   static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
> >   {
> >       char *pdu = queue->pdu;
> > +     char cbuf[CMSG_LEN(sizeof(char))] = {};
> >       struct msghdr msg = {
> > +             .msg_control = cbuf,
> > +             .msg_controllen = sizeof(cbuf),
> >               .msg_flags = MSG_DONTWAIT,
> >       };
> >       struct kvec iov = {
> > @@ -873,12 +877,17 @@ static int nvme_tcp_recvmsg_pdu(struct nvme_tcp_queue *queue)
> >       if (ret <= 0)
> >               return ret;
> >
> > +     hdr = queue->pdu;
> > +     if (hdr->type == TLS_HANDSHAKE_KEYUPDATE) {
> > +             dev_err(queue->ctrl->ctrl.device, "KeyUpdate message\n");
> > +             return 1;
> > +     }
> > +
> >       queue->pdu_remaining -= ret;
> >       queue->pdu_offset += ret;
> >       if (queue->pdu_remaining)
> >               return 0;
> >
> > -     hdr = queue->pdu;
> >       if (unlikely(hdr->hlen != sizeof(struct nvme_tcp_rsp_pdu))) {
> >               if (!nvme_tcp_recv_pdu_supported(hdr->type))
> >                       goto unsupported_pdu;
> > @@ -944,6 +953,7 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
> >       struct request *rq =
> >               nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
> >       struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> > +     char cbuf[CMSG_LEN(sizeof(char))] = {};
> >
> >       if (nvme_tcp_recv_state(queue) != NVME_TCP_RECV_DATA)
> >               return 0;
> > @@ -973,12 +983,14 @@ static int nvme_tcp_recvmsg_data(struct nvme_tcp_queue *queue)
> >               memset(&msg, 0, sizeof(msg));
> >               msg.msg_iter = req->iter;
> >               msg.msg_flags = MSG_DONTWAIT;
> > +             msg.msg_control = cbuf,
> > +             msg.msg_controllen = sizeof(cbuf),
> >
> Watch out. This is the recvmsg bug Olga had been posting patches for.
> Thing is, if there is a control message the networking code will place
> the control message payload into the message buffer. But in doing so
> it expects the message buffer to be an iovec, not a bio vec.
>
> To handle this properly you'd need to _not_ set the control buffer,
> but rather check for 'MSG_CTRUNC' in msg_flags upon return.
> Then you have to setup a new message with msg_control set and
> a suitable msg_len (5 bytes, wasn't it?) and re-issue recvmsg
> with that message.

Fixed

>
> And keep fingers crossed that you don't get MSG_CTRUNC on every
> call to recvmsg() ...
>
> >               ret = sock_recvmsg(queue->sock, &msg, msg.msg_flags);
> >               if (ret < 0) {
> > -                     dev_err(queue->ctrl->ctrl.device,
> > -                             "queue %d failed to receive request %#x data",
> > -                             nvme_tcp_queue_id(queue), rq->tag);
> > +                     dev_dbg(queue->ctrl->ctrl.device,
> > +                             "queue %d failed to receive request %#x data, %d",
> > +                             nvme_tcp_queue_id(queue), rq->tag, ret);
> >                       return ret;
> >               }
> >               if (queue->data_digest)
> > @@ -1381,17 +1393,42 @@ static int nvme_tcp_try_recvmsg(struct nvme_tcp_queue *queue)
> >               }
> >       } while (result >= 0);
> >
> > -     if (result < 0 && result != -EAGAIN) {
> > +     if (result == -EKEYEXPIRED) {
> > +             return -EKEYEXPIRED;
> > +     } else if (result == -EAGAIN) {
> > +             return -EAGAIN;
> > +     } else if (result < 0) {
> >               dev_err(queue->ctrl->ctrl.device,
> >                       "receive failed:  %d\n", result);
> >               queue->rd_enabled = false;
> >               nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> > -     } else if (result == -EAGAIN)
> > -             result = 0;
> > +     }
> >
> >       return result < 0 ? result : (queue->nr_cqe = nr_cqe);
> >   }
> >
> > +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);
> > +
> > +     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 =
> > @@ -1414,8 +1451,11 @@ static void nvme_tcp_io_work(struct work_struct *w)
> >               result = nvme_tcp_try_recvmsg(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) &&
> > @@ -1723,6 +1763,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;
>
> Hmm. I wonder, do we need to store the generation number somewhere?
> Currently the sysfs interface is completely oblivious that a key update
> has happened. I really would like to have _some_ indicator there telling
> us that a key update had happened, and the generation number would be
> ideal here.

I don't follow.

The TLS layer will report the number of KeyUpdates that have been
received. Userspace also knows that a KeyUpdate happened as we call to
userspace to handle updating the keys.

Alistair

>
> >       }
> >
> >   out_complete:
> > @@ -1752,6 +1793,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);
> >       if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
>
> Chers,
> 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 v4 0/7] nvme-tcp: Support receiving KeyUpdate requests

[Alistair Francis]

On Tue, Oct 21, 2025 at 4:40 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/21/25 03:01, Alistair Francis wrote:
> > On Tue, Oct 21, 2025 at 3:46 AM Hannes Reinecke <hare@suse.de> wrote:
> >>
> >> On 10/17/25 06:23, 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.
> >>>
> >>
> >> I am rather sceptical at the current tlshd implementation.
> >> At which place do you update the sending keys?
> >
> > The sending keys are updated as part of gnutls_session_key_update().
> >
> > gnutls_session_key_update() calls update_sending_key() which updates
> > the sending keys.
> >
> > The idea is that when the sequence number is about to overflow the
> > kernel will request userspace to update the sending keys via the
> > HANDSHAKE_KEY_UPDATE_TYPE_SEND key_update_type. Userspace updates the
> > keys and initiates a KeyUpdate.
> >
> That's also what the spec says.
> But in order to do that we would need to get hold of the sequence
> number, which currently is internal to gnutls.

The sequence number is in the kernel. After the handshake the kernel
takes over the TLS connection, so it's up to the kernel to detect a
sequence number overflow. My sending KeyUpdate patches do this, but
they aren't included in this series.

> Can we extract it from the session information?

gnutls can export the sequence number, but as above it doesn't
actually know the correct value (after the handshake).

> And can we display it in sysfs, to give users information
> whether a KeyUpdate had happened?

I don't think that's a good idea. Writing the sequence number to sysfs
seems like extra overhead in the TLS fast path. On top of that I don't
see why userspace cares or what it can do with the number

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 v4 4/7] net/handshake: Support KeyUpdate message types

[Alistair Francis]

On Tue, Oct 21, 2025 at 1:19 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Oct 20, 2025 at 4:09 PM Hannes Reinecke <hare@suse.de> wrote:
> >
> > On 10/17/25 06:23, 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>
> > > ---
> > > v4:
> > >   - Don't overload existing functions, instead create new ones
> > > v3:
> > >   - Fixup yamllint and kernel-doc failures
> > >
> > >   Documentation/netlink/specs/handshake.yaml | 16 ++++-
> > >   drivers/nvme/host/tcp.c                    | 15 +++-
> > >   drivers/nvme/target/tcp.c                  | 10 ++-
> > >   include/net/handshake.h                    |  8 +++
> > >   include/uapi/linux/handshake.h             | 13 ++++
> > >   net/handshake/tlshd.c                      | 83 +++++++++++++++++++++-
> > >   6 files changed, 137 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> > > index a273bc74d26f..c72ec8fa7d7a 100644
> > > --- a/Documentation/netlink/specs/handshake.yaml
> > > +++ b/Documentation/netlink/specs/handshake.yaml
> > > @@ -21,12 +21,18 @@ definitions:
> > >       type: enum
> > >       name: msg-type
> > >       value-start: 0
> > > -    entries: [unspec, clienthello, serverhello]
> > > +    entries: [unspec, clienthello, serverhello, clientkeyupdate,
> > > +              clientkeyupdaterequest, serverkeyupdate, serverkeyupdaterequest]
> > >     -
> >
> > Why do we need the 'keyupdate' and 'keyupdaterequest' types?
>
> msg-type indicates if it's a client or server and hello or keyupdate,
> the idea being
>
> client:
>  - Hello
>  - KeyUpdate
>
> server:
>  - Hello
>  - KeyUpdate
>
> I'll drop clientkeyupdaterequest and serverkeyupdaterequest
>
> > Isn't the 'keyupdate' type enough, and can we specify anything
> > else via the update type?
>
> Once we know if it's a client or server KeyUpdate we need to know if
> we are receiving one, sending one or receiving one with the
> request_update flag set, hence key-update-type
>
> >
> > >       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]
> >
> > See above.
> >
> > >
> > >   attribute-sets:
> > >     -
> > > @@ -74,6 +80,13 @@ attribute-sets:
> > >         -
> > >           name: keyring
> > >           type: u32
> > > +      -
> > > +        name: key-update-request
> > > +        type: u32
> > > +        enum: key-update-type
> > > +      -
> > > +        name: key-serial
> > > +        type: u32
> >
> > Not sure if I like key-serial. Yes, it is a key serial number,
> > but it's not the serial number of the updated key (rather the serial
> > number of the key holding the session information).
> > Maybe 'key-update-serial' ?
> >
> > >     -
> > >       name: done
> > >       attributes:
> > > @@ -116,6 +129,7 @@ operations:
> > >               - certificate
> > >               - peername
> > >               - keyring
> > > +            - key-serial
> > >       -
> > >         name: done
> > >         doc: Handler reports handshake completion
> > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > > index 611be56f8013..2696bf97dfac 100644
> > > --- a/drivers/nvme/host/tcp.c
> > > +++ b/drivers/nvme/host/tcp.c
> > > @@ -20,6 +20,7 @@
> > >   #include <linux/iov_iter.h>
> > >   #include <net/busy_poll.h>
> > >   #include <trace/events/sock.h>
> > > +#include <uapi/linux/handshake.h>
> > >
> > >   #include "nvme.h"
> > >   #include "fabrics.h"
> > > @@ -206,6 +207,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)
> > >   {
> > > @@ -1726,7 +1731,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;
> > > @@ -1748,7 +1754,10 @@ 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);
> > > +     if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> > > +             ret = tls_client_hello_psk(&args, GFP_KERNEL);
> > > +     else
> > > +             ret = tls_client_keyupdate_psk(&args, GFP_KERNEL, keyupdate);
> > >       if (ret) {
> > >               dev_err(nctrl->device, "queue %d: failed to start TLS: %d\n",
> > >                       qid, ret);
> > > @@ -1898,7 +1907,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..8aeec4a7f136 100644
> > > --- a/drivers/nvme/target/tcp.c
> > > +++ b/drivers/nvme/target/tcp.c
> > > @@ -1833,7 +1833,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 +1853,10 @@ 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);
> > > +     if (keyupdate == HANDSHAKE_KEY_UPDATE_TYPE_UNSPEC)
> > > +             ret = tls_server_hello_psk(&args, GFP_KERNEL);
> > > +     else
> > > +             ret = tls_server_keyupdate_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 +1938,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 dc2222fd6d99..084c92a20b68 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
> > > +
> > Huh?
> > You define it as 'u32' here
> >
> > >   enum {
> > >       TLS_NO_KEYRING = 0,
> > >       TLS_NO_PEERID = 0,
> > > @@ -38,8 +42,12 @@ struct tls_handshake_args {
> > >   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_keyupdate_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_keyupdate_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,
> > > +};
> > > +
> >
> > and here it's an enum. Please kill the first declaration.
> >
> > >   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 2549c5dbccd8..c40839977ab9 100644
> > > --- a/net/handshake/tlshd.c
> > > +++ b/net/handshake/tlshd.c
> > > @@ -41,6 +41,7 @@ struct tls_handshake_req {
> > >       unsigned int            th_num_peerids;
> > >       key_serial_t            th_peerid[5];
> > >
> > > +     int                     th_key_update_request;
> > >       key_serial_t            user_session_id;
> > >   };
> > >
> > Why 'int' ? Can it be negative?
> > If not please make it an 'unsigned int'
> >
> > > @@ -58,7 +59,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_session_id = TLS_NO_PRIVKEY;
> > > +     treq->user_session_id = args->user_session_id;
> > > +
> > >       return treq;
> > >   }
> > >
> > > @@ -265,6 +267,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);
> > >
> > > @@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> > >   }
> > >   EXPORT_SYMBOL(tls_client_hello_psk);
> > >
> > > +/**
> > > + * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
> > > + * @args: socket and handshake parameters for this request
> > > + * @flags: memory allocation control flags
> > > + * @keyupdate: specifies the type of KeyUpdate operation
> > > + *
> > > + * Return values:
> > > + *   %0: Handshake request enqueue; ->done will be called when complete
> > > + *   %-EINVAL: Wrong number of local peer IDs
> > > + *   %-ESRCH: No user agent is available
> > > + *   %-ENOMEM: Memory allocation failed
> > > + */
> > > +int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> > > +                          handshake_key_update_type keyupdate)
> > > +{
> > > +     struct tls_handshake_req *treq;
> > > +     struct handshake_req *req;
> > > +     unsigned int i;
> > > +
> > > +     if (!args->ta_num_peerids ||
> > > +         args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
> > > +             return -EINVAL;
> > > +
> > > +     req = handshake_req_alloc(&tls_handshake_proto, flags);
> > > +     if (!req)
> > > +             return -ENOMEM;
> > > +     treq = tls_handshake_req_init(req, args);
> > > +     treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
> > > +     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++)
> > > +             treq->th_peerid[i] = args->ta_my_peerids[i];
> > Hmm?
> > Do we use the 'peerids'?
>
> We don't, this is just copied from the
> tls_client_hello_psk()/tls_server_hello_psk() to provide the same
> information to keep things more consistent.
>
> I can remove setting these

Actually, ktls-utils (tlshd) expects these to be set, so I think we
should leave them as is

Alistair

>
> > I thought that the information was encoded in the session, ie
> > the 'user_session_id' ?
> >
> > > +
> > > +     return handshake_req_submit(args->ta_sock, req, flags);
> > > +}
> > > +EXPORT_SYMBOL(tls_client_keyupdate_psk);
> > > +
> > >   /**
> > >    * tls_server_hello_x509 - request a server TLS handshake on a socket
> > >    * @args: socket and handshake parameters for this request
> > > @@ -428,6 +478,37 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags)
> > >   }
> > >   EXPORT_SYMBOL(tls_server_hello_psk);
> > >
> > > +/**
> > > + * tls_server_keyupdate_psk - request a server TLS KeyUpdate on a socket
> > > + * @args: socket and handshake parameters for this request
> > > + * @flags: memory allocation control flags
> > > + * @keyupdate: specifies the type of KeyUpdate operation
> > > + *
> > > + * Return values:
> > > + *   %0: Handshake request enqueue; ->done will be called when complete
> > > + *   %-ESRCH: No user agent is available
> > > + *   %-ENOMEM: Memory allocation failed
> > > + */
> > > +int tls_server_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> > > +                          handshake_key_update_type keyupdate)
> > > +{
> > > +     struct tls_handshake_req *treq;
> > > +     struct handshake_req *req;
> > > +
> > > +     req = handshake_req_alloc(&tls_handshake_proto, flags);
> > > +     if (!req)
> > > +             return -ENOMEM;
> > > +     treq = tls_handshake_req_init(req, args);
> > > +     treq->th_type = HANDSHAKE_MSG_TYPE_SERVERKEYUPDATE;
> > > +     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];
> >
> > Same here. Why do we need to set 'peerid'?
> >
> > > +
> > > +     return handshake_req_submit(args->ta_sock, req, flags);
> > > +}
> > > +EXPORT_SYMBOL(tls_server_keyupdate_psk);
> > > +
> > >   /**
> > >    * tls_handshake_cancel - cancel a pending handshake
> > >    * @sk: socket on which there is an ongoing handshake
> > Nit: we _could_ overload 'peerid' with the user_session_id,then we
> > wouldn't need to specify a new field in the handshake
> > request.
> > But that's arguably quite hackish.
>
> Oh no! Let's not do that. That just seems prone to confusion
>
> 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 v4 5/7] nvme-tcp: Support KeyUpdate

[Hannes Reinecke]

On 10/22/25 06:35, Alistair Francis wrote:
> On Mon, Oct 20, 2025 at 4:22 PM Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 10/17/25 06:23, alistair23@gmail.com wrote:
>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>
[ .. ]>>> @@ -1723,6 +1763,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;
>>
>> Hmm. I wonder, do we need to store the generation number somewhere?
>> Currently the sysfs interface is completely oblivious that a key update
>> has happened. I really would like to have _some_ indicator there telling
>> us that a key update had happened, and the generation number would be
>> ideal here.
> 
> I don't follow.
> 
> The TLS layer will report the number of KeyUpdates that have been
> received. Userspace also knows that a KeyUpdate happened as we call to
> userspace to handle updating the keys.
> 
Oh, the tlshd will know that (somehow). But everyone else will not; the
'tls_pskid' contents will stay the the same.
Can we have a sysfs attribute reporting the sequence number of the most
recent KeyUpdate?
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 v4 4/7] net/handshake: Support KeyUpdate message types

[Hannes Reinecke]

On 10/22/25 06:40, Alistair Francis wrote:
> On Tue, Oct 21, 2025 at 1:19 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Mon, Oct 20, 2025 at 4:09 PM Hannes Reinecke <hare@suse.de> wrote:
>>>
>>> On 10/17/25 06:23, alistair23@gmail.com wrote:
>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>
[ .. ]>>>> @@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct 
tls_handshake_args *args, gfp_t flags)
>>>>    }
>>>>    EXPORT_SYMBOL(tls_client_hello_psk);
>>>>
>>>> +/**
>>>> + * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
>>>> + * @args: socket and handshake parameters for this request
>>>> + * @flags: memory allocation control flags
>>>> + * @keyupdate: specifies the type of KeyUpdate operation
>>>> + *
>>>> + * Return values:
>>>> + *   %0: Handshake request enqueue; ->done will be called when complete
>>>> + *   %-EINVAL: Wrong number of local peer IDs
>>>> + *   %-ESRCH: No user agent is available
>>>> + *   %-ENOMEM: Memory allocation failed
>>>> + */
>>>> +int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
>>>> +                          handshake_key_update_type keyupdate)
>>>> +{
>>>> +     struct tls_handshake_req *treq;
>>>> +     struct handshake_req *req;
>>>> +     unsigned int i;
>>>> +
>>>> +     if (!args->ta_num_peerids ||
>>>> +         args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
>>>> +             return -EINVAL;
>>>> +
>>>> +     req = handshake_req_alloc(&tls_handshake_proto, flags);
>>>> +     if (!req)
>>>> +             return -ENOMEM;
>>>> +     treq = tls_handshake_req_init(req, args);
>>>> +     treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
>>>> +     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++)
>>>> +             treq->th_peerid[i] = args->ta_my_peerids[i];
>>> Hmm?
>>> Do we use the 'peerids'?
>>
>> We don't, this is just copied from the
>> tls_client_hello_psk()/tls_server_hello_psk() to provide the same
>> information to keep things more consistent.
>>
>> I can remove setting these
> 
> Actually, ktls-utils (tlshd) expects these to be set, so I think we
> should leave them as is
> 

Can't we rather fix up tlshd?
It feels really pointless, erroring out on values which are completely
irrelevant for the operation...

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 v4 5/7] nvme-tcp: Support KeyUpdate

[Alistair Francis]

On Wed, Oct 22, 2025 at 4:56 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/22/25 06:35, Alistair Francis wrote:
> > On Mon, Oct 20, 2025 at 4:22 PM Hannes Reinecke <hare@suse.de> wrote:
> >>
> >> On 10/17/25 06:23, alistair23@gmail.com wrote:
> >>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>
> [ .. ]>>> @@ -1723,6 +1763,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;
> >>
> >> Hmm. I wonder, do we need to store the generation number somewhere?
> >> Currently the sysfs interface is completely oblivious that a key update
> >> has happened. I really would like to have _some_ indicator there telling
> >> us that a key update had happened, and the generation number would be
> >> ideal here.
> >
> > I don't follow.
> >
> > The TLS layer will report the number of KeyUpdates that have been
> > received. Userspace also knows that a KeyUpdate happened as we call to
> > userspace to handle updating the keys.
> >
> Oh, the tlshd will know that (somehow). But everyone else will not; the
> 'tls_pskid' contents will stay the the same.
> Can we have a sysfs attribute reporting the sequence number of the most
> recent KeyUpdate?

Why do we want to reveal that to userspace though?

Realistically it should just be ~2^64 and it'll should remain the same
number, even after multiple updates

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 v4 4/7] net/handshake: Support KeyUpdate message types

[Alistair Francis]

On Wed, Oct 22, 2025 at 5:03 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 10/22/25 06:40, Alistair Francis wrote:
> > On Tue, Oct 21, 2025 at 1:19 PM Alistair Francis <alistair23@gmail.com> wrote:
> >>
> >> On Mon, Oct 20, 2025 at 4:09 PM Hannes Reinecke <hare@suse.de> wrote:
> >>>
> >>> On 10/17/25 06:23, alistair23@gmail.com wrote:
> >>>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>>
> [ .. ]>>>> @@ -372,6 +384,44 @@ int tls_client_hello_psk(const struct
> tls_handshake_args *args, gfp_t flags)
> >>>>    }
> >>>>    EXPORT_SYMBOL(tls_client_hello_psk);
> >>>>
> >>>> +/**
> >>>> + * tls_client_keyupdate_psk - request a PSK-based TLS handshake on a socket
> >>>> + * @args: socket and handshake parameters for this request
> >>>> + * @flags: memory allocation control flags
> >>>> + * @keyupdate: specifies the type of KeyUpdate operation
> >>>> + *
> >>>> + * Return values:
> >>>> + *   %0: Handshake request enqueue; ->done will be called when complete
> >>>> + *   %-EINVAL: Wrong number of local peer IDs
> >>>> + *   %-ESRCH: No user agent is available
> >>>> + *   %-ENOMEM: Memory allocation failed
> >>>> + */
> >>>> +int tls_client_keyupdate_psk(const struct tls_handshake_args *args, gfp_t flags,
> >>>> +                          handshake_key_update_type keyupdate)
> >>>> +{
> >>>> +     struct tls_handshake_req *treq;
> >>>> +     struct handshake_req *req;
> >>>> +     unsigned int i;
> >>>> +
> >>>> +     if (!args->ta_num_peerids ||
> >>>> +         args->ta_num_peerids > ARRAY_SIZE(treq->th_peerid))
> >>>> +             return -EINVAL;
> >>>> +
> >>>> +     req = handshake_req_alloc(&tls_handshake_proto, flags);
> >>>> +     if (!req)
> >>>> +             return -ENOMEM;
> >>>> +     treq = tls_handshake_req_init(req, args);
> >>>> +     treq->th_type = HANDSHAKE_MSG_TYPE_CLIENTKEYUPDATE;
> >>>> +     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++)
> >>>> +             treq->th_peerid[i] = args->ta_my_peerids[i];
> >>> Hmm?
> >>> Do we use the 'peerids'?
> >>
> >> We don't, this is just copied from the
> >> tls_client_hello_psk()/tls_server_hello_psk() to provide the same
> >> information to keep things more consistent.
> >>
> >> I can remove setting these
> >
> > Actually, ktls-utils (tlshd) expects these to be set, so I think we
> > should leave them as is
> >
>
> Can't we rather fix up tlshd?
> It feels really pointless, erroring out on values which are completely
> irrelevant for the operation...

It's not that simple.

For example when we call "done" for a handshake or KeyUpdate we call
tls_handshake_done() in the kernel, which calls
tls_handshake_remote_peerids(). So the kernel expects the remote
peerids to be set.

I think there's a lot of value in re-using the existing flows (as a
KeyUpdate is similar to a handshake), but the existing flows expect
remote peerids to be set. We could duplicate everything just to remove
that requirement, but I don't think that's the right approach.

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