[PATCH 15/18] nvmet-tcp: enable TLS handshake upcall
Hannes Reinecke
hare at suse.de
Mon Apr 3 07:05:27 PDT 2023
On 4/3/23 14:51, Sagi Grimberg wrote:
>
>> Add functions to start the TLS handshake upcall when
>> the TCP RSAS sectype is set to 'tls1.3'.
>
> TSAS
>
Ok.
>>
>> 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?
>
Sure.
>> +
>> 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?
>
Strictly speaking, no; it just won't do anything except from having a
different value in the discovery log page.
>> +#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.
>
Ok.
>> + 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.
>
Hmm. Should. I can remove the ifdefs
>> +#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?
>
Wasn't sure if we always want to have it.
But if we do, sure, things will be easier.
>> 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.
>
I see what I can do.
>> +}
>> +
>> +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?
Well, this is a slightly odd case; we get a '0' status but failed
to lookup the key.
_Technically_ we should be able to continue, but wasn't sure if I should.
But in the light of the key rotation discussion I probably should; new
keys (and keyrings) might be provided at any time, so I might hit that
case here.
Will be updating the code.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare at suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg
Managing Directors: I. Totev, A. Myers, A. McDonald, M. B. Moerman
(HRB 36809, AG Nürnberg)
More information about the Linux-nvme
mailing list