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

[Hannes Reinecke <hare@suse.de>]

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@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@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)