On 8/15/25 1:02 AM, alistair23@xxxxxxxxx wrote:
> From: Alistair Francis <alistair.francis@xxxxxxx>
>
> 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.
Hi Alistair, thanks for continuing to pursue this functionality.
I'll need some time to go over this series more carefully, but a few
mechanical issues stand out immediately. See below.
> Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx>
> ---
> Documentation/netlink/specs/handshake.yaml | 4 ++++
> drivers/nvme/host/tcp.c | 3 ++-
> drivers/nvme/target/tcp.c | 3 ++-
> include/net/handshake.h | 3 ++-
> include/uapi/linux/handshake.h | 1 +
> net/handshake/genl.c | 5 +++--
> net/handshake/tlshd.c | 15 +++++++++++++--
> net/sunrpc/svcsock.c | 3 ++-
> net/sunrpc/xprtsock.c | 3 ++-
> 9 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
> index 95c3fade7a8d..e76b10ef62f2 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: key-serial
> + type: u32
Let's choose a less generic name for this type. All of the peer IDs are
"key serial numbers", I think? What do you think of "session-id" or
"session-ctx" ?
> operations:
> list:
> @@ -123,6 +126,7 @@ operations:
> - status
> - sockfd
> - remote-auth
> + - key-serial
>
> mcast-groups:
> list:
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index c0fe8cfb7229..bb7317a3f1a9 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1673,7 +1673,8 @@ static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
> qid, queue->io_cpu);
> }
>
> -static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
> +static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid,
> + key_serial_t user_key_serial)
I have a patch series that adds a parameter to ->done as well. I think
it's time to consider defining a struct that carries all of this info
instead of adding more new parameters to ->done. That can be done as a
separate patch, perhaps.
> {
> 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..93fce316267d 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_key_serial)
> {
> struct nvmet_tcp_queue *queue = data;
>
> diff --git a/include/net/handshake.h b/include/net/handshake.h
> index 8ebd4f9ed26e..449bed8c2557 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_key_serial);
>
> struct tls_handshake_args {
> struct socket *ta_sock;
> diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
> index 662e7de46c54..46753116ba43 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_KEY_SERIAL,
As above, KEY_SERIAL is too generic IMO.
I suppose Hannes' "A_KEYRING" is a similar vague, generic sounding
argument name... Though I'm not sure it has as specific a function as
key-serial will have.
> __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..bf64323bb5e1 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_KEY_SERIAL + 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_KEY_SERIAL] = { .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_KEY_SERIAL,
> .flags = GENL_CMD_CAP_DO,
> },
> };
> diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c
> index 081093dfd553..cb1ee8ebf2ea 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_key_serial);
> void *th_consumer_data;
>
> int th_type;
> @@ -39,6 +40,8 @@ struct tls_handshake_req {
>
> unsigned int th_num_peerids;
> key_serial_t th_peerid[5];
> +
> + key_serial_t user_key_serial;
> };
>
> static struct tls_handshake_req *
> @@ -55,6 +58,7 @@ tls_handshake_req_init(struct handshake_req *req,
> treq->th_num_peerids = 0;
> treq->th_certificate = TLS_NO_CERT;
> treq->th_privkey = TLS_NO_PRIVKEY;
> + treq->user_key_serial = TLS_NO_PRIVKEY;
> return treq;
> }
>
> @@ -83,6 +87,13 @@ static void tls_handshake_remote_peerids(struct tls_handshake_req *treq,
> if (i >= treq->th_num_peerids)
> break;
> }
> +
> + nla_for_each_attr(nla, head, len, rem) {
> + if (nla_type(nla) == HANDSHAKE_A_DONE_KEY_SERIAL) {
> + treq->user_key_serial = 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_key_serial);
> }
>
> #if IS_ENABLED(CONFIG_KEYS)
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 46c156b121db..3a325d7f2049 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -423,7 +423,8 @@ static void svc_tcp_kill_temp_xprt(struct svc_xprt *xprt)
> * is present" flag on the xprt and let an upper layer enforce local
> * security policy.
> */
> -static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid)
> +static void svc_tcp_handshake_done(void *data, int status, key_serial_t peerid,
> + key_serial_t user_key_serial)
> {
> struct svc_xprt *xprt = data;
> struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index c5f7bbf5775f..8edd095b3a40 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2591,7 +2591,8 @@ static int xs_tcp_tls_finish_connecting(struct rpc_xprt *lower_xprt,
> * @peerid: serial number of key containing the remote's identity
> *
> */
> -static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid)
> +static void xs_tls_handshake_done(void *data, int status, key_serial_t peerid,
> + key_serial_t user_key_serial)
> {
> struct rpc_xprt *lower_xprt = data;
> struct sock_xprt *lower_transport =
--
Chuck Lever
