From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com [209.85.221.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 66FA35683 for ; Mon, 3 Apr 2023 12:51:12 +0000 (UTC) Received: by mail-wr1-f49.google.com with SMTP id ffacd0b85a97d-2d17b1b21f1so508113f8f.1 for ; Mon, 03 Apr 2023 05:51:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680526270; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=BlkZpGHZPC53irRsKzp6kuuPu4jbfR4ylYoi2Xneehw=; b=gv0CbgYfHq9xtRw4VVUKiCnFX22wQdc08G9a7RPBgQ0qFCdj1988BBlNEPJb62+ttc n/E1itdzEUGunWRVsqehqT2tFHkGscqG/ALphg/65c4C0yLXh5G4k1hYX5xp1NCW/Sim qNxKAHdSHwYrEz8WblJ+2X3/gUuoUHiIwIm4Ot3CWQrbrhA5fIYf6AQNwDY5XRTKD/4f ZS4sYYLyZtaN23jsGiSn8OoeC/JghJ88FCH/jeRWJc63annXtZKc9HwOFRNCfB88/gBs 5lqc5/nDffn3uj6MGStsGuepTkHglOWmgYrTrIygzxj1Fa/pEg9M4/cOW/DRN+JyaSrB c/yQ== X-Gm-Message-State: AAQBX9djsCfgtZ7ICCnJY+Rr8us9ShljbZ/D+Tm7Omyz3Rx5AJMeMRRM 62IPBfJdrdPbx48av/+m9XE= X-Google-Smtp-Source: AKy350bp3m28EaDitddBu8G7sYnthzBcQlgUU16HD+Y7QsuedkUkNrfJNAoM+/bpdYgGtgIP5kbuLA== X-Received: by 2002:a05:600c:1d03:b0:3eb:42f6:ac55 with SMTP id l3-20020a05600c1d0300b003eb42f6ac55mr11911191wms.1.1680526270576; Mon, 03 Apr 2023 05:51:10 -0700 (PDT) Received: from [192.168.64.192] (bzq-219-42-90.isdn.bezeqint.net. [62.219.42.90]) by smtp.gmail.com with ESMTPSA id k22-20020a7bc316000000b003ee20b4b2dasm11958539wmj.46.2023.04.03.05.51.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 03 Apr 2023 05:51:10 -0700 (PDT) Message-ID: Date: Mon, 3 Apr 2023 15:51:08 +0300 Precedence: bulk X-Mailing-List: kernel-tls-handshake@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH 15/18] nvmet-tcp: enable TLS handshake upcall Content-Language: en-US To: Hannes Reinecke , Christoph Hellwig Cc: Keith Busch , linux-nvme@lists.infradead.org, Chuck Lever , kernel-tls-handshake@lists.linux.dev References: <20230329135938.46905-1-hare@suse.de> <20230329135938.46905-16-hare@suse.de> From: Sagi Grimberg In-Reply-To: <20230329135938.46905-16-hare@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit > Add functions to start the TLS handshake upcall when > the TCP RSAS sectype is set to 'tls1.3'. TSAS > > Signed-off-by: Hannes Reincke > --- > 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 > #include > #include > +#ifdef CONFIG_NVME_TLS > +#include Is net/handshake.h under an ifdef? If so, CONFIG_NVME_TLS should select it. > +#include will this include not work if CONFIG_NVME_TLS not work? is not under CONFIG_NVME_AUTH for example. > +#endif > #include > #include > #include > @@ -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?