[PATCH 15/18] nvmet-tcp: enable TLS handshake upcall
Sagi Grimberg
sagi at grimberg.me
Mon Apr 3 05:51:08 PDT 2023
> 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 at 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?
More information about the Linux-nvme
mailing list