Re: [PATCH 02/16] nvme-tcp: sanitize TLS key handling

[Sagi Grimberg <sagi@grimberg.me>]

On 17/07/2024 12:10, Hannes Reinecke wrote:
> There is a difference between TLS configured (ie the user has
> provisioned/requested a key) and TLS enabled (ie the connection
> is encrypted with TLS). This becomes important for secure concatenation,
> where the initial authentication is run unencrypted (ie with
> TLS configured, but not enabled), and then the queue is reset to
> run over TLS (ie TLS configured _and_ enabled).
> So to differentiate between those two states store the provisioned
> key in opts->tls_key (as we're using the same TLS key for all queues)
> and only the key serial of the key negotiated by the TLS handshake
> in queue->tls_pskid.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
>   drivers/nvme/host/core.c  |  1 -
>   drivers/nvme/host/nvme.h  |  2 +-
>   drivers/nvme/host/sysfs.c |  4 ++--
>   drivers/nvme/host/tcp.c   | 47 ++++++++++++++++++++++++++++-----------
>   4 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8d8e7a3549c6..947f1e631ee5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4641,7 +4641,6 @@ static void nvme_free_ctrl(struct device *dev)
>   
>   	if (!subsys || ctrl->instance != subsys->instance)
>   		ida_free(&nvme_instance_ida, ctrl->instance);
> -	key_put(ctrl->tls_key);
>   	nvme_free_cels(ctrl);
>   	nvme_mpath_uninit(ctrl);
>   	cleanup_srcu_struct(&ctrl->srcu);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index c63f2b452369..cdb53323f4eb 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -370,7 +370,7 @@ struct nvme_ctrl {
>   	struct nvme_dhchap_key *ctrl_key;
>   	u16 transaction;
>   #endif
> -	struct key *tls_key;
> +	key_serial_t tls_pskid;
>   
>   	/* Power saving configuration */
>   	u64 ps_max_latency_us;
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 3c55f7edd181..5b1dee8a66ef 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -671,9 +671,9 @@ static ssize_t tls_key_show(struct device *dev,
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>   
> -	if (!ctrl->tls_key)
> +	if (!ctrl->tls_pskid)
>   		return 0;
> -	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
> +	return sysfs_emit(buf, "%08x", ctrl->tls_pskid);
>   }
>   static DEVICE_ATTR_RO(tls_key);
>   #endif
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a2a47d3ab99f..92ad5b8cc1b4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -165,6 +165,7 @@ struct nvme_tcp_queue {
>   
>   	bool			hdr_digest;
>   	bool			data_digest;
> +	bool			tls_enabled;

I swear I'll ask this every single time that I don't understand it. Why 
is this per-queue and
not per controller?