Re: [PATCH 15/18] nvmet-tcp: enable TLS handshake upcall

[Sagi Grimberg <sagi@grimberg.me>]

> Add functions to start the TLS handshake upcall when
> the TCP RSAS sectype is set to 'tls1.3'.

TSAS

> 
> Signed-off-by: Hannes Reincke <hare@suse.de>
> ---
>   drivers/nvme/target/configfs.c |  32 +++++++-
>   drivers/nvme/target/tcp.c      | 135 ++++++++++++++++++++++++++++++++-
>   2 files changed, 163 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index ca66ee6dc153..36fbf6a22d09 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -159,10 +159,12 @@ static const struct nvmet_type_name_map nvmet_addr_treq[] = {
>   	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
>   };
>   
> +#define NVMET_PORT_TREQ(port) ((port)->disc_addr.treq & NVME_TREQ_SECURE_CHANNEL_MASK)

Can you make it a static inline?

> +
>   static ssize_t nvmet_addr_treq_show(struct config_item *item, char *page)
>   {
> -	u8 treq = to_nvmet_port(item)->disc_addr.treq &
> -		NVME_TREQ_SECURE_CHANNEL_MASK;
> +	struct nvmet_port *port = to_nvmet_port(item);
> +	u8 treq = NVMET_PORT_TREQ(port);
>   	int i;
>   
>   	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> @@ -193,6 +195,17 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> +#ifdef CONFIG_NVME_TLS
> +	if (port->disc_addr.trtype == NVMF_TRTYPE_TCP) {
> +		if (port->disc_addr.tsas.tcp.sectype != NVMF_TCP_SECTYPE_TLS13) {
> +			pr_warn("cannot change TREQ when TLS is not enabled\n");
> +			return -EINVAL;
> +		} else if (nvmet_addr_treq[i].type == NVMF_TREQ_NOT_SPECIFIED) {
> +			pr_warn("cannot set TREQ to 'not specified' when TLS is enabled\n");
> +			return -EINVAL;
> +		}
> +	}

Is this code wrong if CONFIG_NVME_TLS is not enabled?

> +#endif
>   	treq |= nvmet_addr_treq[i].type;
>   	port->disc_addr.treq = treq;
>   	return count;
> @@ -373,6 +386,7 @@ static ssize_t nvmet_addr_tsas_store(struct config_item *item,
>   		const char *page, size_t count)
>   {
>   	struct nvmet_port *port = to_nvmet_port(item);
> +	u8 treq = port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK;
>   	int i;
>   
>   	if (nvmet_is_port_enabled(port, __func__))
> @@ -391,6 +405,20 @@ static ssize_t nvmet_addr_tsas_store(struct config_item *item,
>   
>   found:
>   	nvmet_port_init_tsas_tcp(port, nvmet_addr_tsas_tcp[i].type);
> +	if (nvmet_addr_tsas_tcp[i].type == NVMF_TCP_SECTYPE_TLS13) {
> +#ifdef CONFIG_NVME_TLS

Maybe in the start of the function just do:

	if (!IS_ENABLED(CONFIG_NVME_TLS)) {
		pr_err("TLS not supported\n");
		return -EINVAL;
	}

Instead of incorporating it here.

> +		if (NVMET_PORT_TREQ(port) == NVMF_TREQ_NOT_SPECIFIED)
> +			treq |= NVMF_TREQ_REQUIRED;
> +		else
> +			treq |= NVMET_PORT_TREQ(port);
> +#else
> +		pr_err("TLS not supported\n");
> +		return -EINVAL;
> +#endif
> +	} else {
> +		/* Set to 'not specified' if TLS is not enabled */
> +		treq |= NVMF_TREQ_NOT_SPECIFIED;
> +	}
>   	return count;
>   }
>   
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 5931971d715f..ebec882120fd 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -11,6 +11,10 @@
>   #include <linux/nvme-tcp.h>
>   #include <net/sock.h>
>   #include <net/tcp.h>
> +#ifdef CONFIG_NVME_TLS
> +#include <net/handshake.h>

Is net/handshake.h under an ifdef? If so, CONFIG_NVME_TLS should
select it.

> +#include <linux/nvme-keyring.h>

will this include not work if CONFIG_NVME_TLS not work?
<linux/nvme-auth.h> is not under CONFIG_NVME_AUTH for example.

> +#endif
>   #include <linux/inet.h>
>   #include <linux/llist.h>
>   #include <crypto/hash.h>
> @@ -40,6 +44,16 @@ module_param(idle_poll_period_usecs, int, 0644);
>   MODULE_PARM_DESC(idle_poll_period_usecs,
>   		"nvmet tcp io_work poll till idle time period in usecs");
>   
> +#ifdef CONFIG_NVME_TLS
> +/*
> + * TLS handshake timeout
> + */
> +static int tls_handshake_timeout = 30;
> +module_param(tls_handshake_timeout, int, 0644);
> +MODULE_PARM_DESC(tls_handshake_timeout,
> +		 "nvme TLS handshake timeout in seconds (default 30)");
> +#endif
> +
>   #define NVMET_TCP_RECV_BUDGET		8
>   #define NVMET_TCP_SEND_BUDGET		8
>   #define NVMET_TCP_IO_WORK_BUDGET	64
> @@ -130,6 +144,10 @@ struct nvmet_tcp_queue {
>   	bool			data_digest;
>   	struct ahash_request	*snd_hash;
>   	struct ahash_request	*rcv_hash;
> +#ifdef CONFIG_NVME_TLS
> +	struct key		*tls_psk;
> +	struct delayed_work	tls_handshake_work;
> +#endif

If these won't be under CONFIG_NVME_TLS will it save a lot of the other
ifdefs in the code?

>   
>   	unsigned long           poll_end;
>   
> @@ -1474,6 +1492,10 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>   	nvmet_tcp_free_cmds(queue);
>   	if (queue->hdr_digest || queue->data_digest)
>   		nvmet_tcp_free_crypto(queue);
> +#ifdef CONFIG_NVME_TLS
> +	if (queue->tls_psk)
> +		key_put(queue->tls_psk);

key_put is NULL safe.

> +#endif
>   	ida_free(&nvmet_tcp_queue_ida, queue->idx);
>   	page = virt_to_head_page(queue->pf_cache.va);
>   	__page_frag_cache_drain(page, queue->pf_cache.pagecnt_bias);
> @@ -1488,8 +1510,12 @@ static void nvmet_tcp_data_ready(struct sock *sk)
>   
>   	read_lock_bh(&sk->sk_callback_lock);
>   	queue = sk->sk_user_data;
> -	if (likely(queue))
> -		queue_work_on(queue_cpu(queue), nvmet_tcp_wq, &queue->io_work);
> +	if (queue->data_ready)
> +		queue->data_ready(sk);
> +	if (likely(queue) &&
> +	    queue->state != NVMET_TCP_Q_TLS_HANDSHAKE)
> +		queue_work_on(queue_cpu(queue), nvmet_tcp_wq,
> +			      &queue->io_work);
>   	read_unlock_bh(&sk->sk_callback_lock);
>   }
>   
> @@ -1597,6 +1623,89 @@ static int nvmet_tcp_set_queue_sock(struct nvmet_tcp_queue *queue)
>   	return ret;
>   }
>   
> +#ifdef CONFIG_NVME_TLS
> +static void nvmet_tcp_tls_queue_restart(struct nvmet_tcp_queue *queue)
> +{
> +	spin_lock(&queue->state_lock);
> +	if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
> +		pr_warn("queue %d: TLS handshake already completed\n",
> +			queue->idx);
> +		spin_unlock(&queue->state_lock);
> +		return;
> +	}
> +	queue->state = NVMET_TCP_Q_CONNECTING;
> +	spin_unlock(&queue->state_lock);
> +
> +	pr_debug("queue %d: restarting queue after TLS handshake\n",
> +		 queue->idx);
> +	/*
> +	 * Set callbacks after handshake; TLS implementation
> +	 * might have changed the socket callbacks.
> +	 */
> +	nvmet_tcp_set_queue_sock(queue);

Maybe fold it into the caller? The name is confusing anyways.
The queue is not restarted, it is post-configured for lack of
a better term.

> +}
> +
> +static void nvmet_tcp_tls_handshake_done(void *data, int status,
> +					 key_serial_t peerid)

					pskid.

> +{
> +	struct nvmet_tcp_queue *queue = data;
> +
> +	pr_debug("queue %d: TLS handshake done, key %x, status %d\n",
> +		 queue->idx, peerid, status);
> +	if (!status) {
> +		spin_lock(&queue->state_lock);
> +		queue->tls_psk = key_lookup(peerid);
> +		if (IS_ERR(queue->tls_psk)) {
> +			pr_warn("queue %d: TLS key %x not found\n",
> +				queue->idx, peerid);
> +			queue->tls_psk = NULL;

Here you let the timeout take care of it later?