[PATCH v3 0/3] nvme link failure fixes

[PATCH v3 0/3] nvme link failure fixes

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

There are still a couple of link failures that I tried to address
with a previous patch. I've split up the missing bits into smaller
patches and tried to explain the bugs in more detail.

With these applied, randconfig builds work again. Please either
merge them or treat them as bug reports and find a different fix,
I won't do another version.

Arnd Bergmann (3):
  nvme: target: fix nvme_keyring_id() references
  nvme: target: fix Kconfig select statements
  nvme: tcp: fix compile-time checks for TLS mode

 drivers/nvme/host/tcp.c        | 31 ++++++++++++++-----------------
 drivers/nvme/target/Kconfig    |  4 ++--
 drivers/nvme/target/configfs.c |  2 +-
 3 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.39.2

Cc: Keith Busch <kbusch@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Cc: linux-nvme@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

[PATCH v3 1/3] nvme: target: fix nvme_keyring_id() references

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

In configurations without CONFIG_NVME_TARGET_TCP_TLS, the keyring
code might not be available, or using it will result in a runtime
failure:

x86_64-linux-ld: vmlinux.o: in function `nvmet_ports_make':
configfs.c:(.text+0x100a211): undefined reference to `nvme_keyring_id'

Add a check to ensure we only check the keyring if there is a chance
of it being used, which avoids both the runtime and link-time
problems.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/target/configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 9eed6e6765ea..e307a044b1a1 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1893,7 +1893,7 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	if (nvme_keyring_id()) {
+	if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS) && nvme_keyring_id()) {
 		port->keyring = key_lookup(nvme_keyring_id());
 		if (IS_ERR(port->keyring)) {
 			pr_warn("NVMe keyring not available, disabling TLS\n");
-- 
2.39.2

[PATCH v3 2/3] nvme: target: fix Kconfig select statements

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

When the NVME target code is built-in but its TCP frontend is a loadable
module, enabling keyring support causes a link failure:

x86_64-linux-ld: vmlinux.o: in function `nvmet_ports_make':
configfs.c:(.text+0x100a211): undefined reference to `nvme_keyring_id'

The problem is that CONFIG_NVME_TARGET_TCP_TLS is a 'bool' symbol that
depends on the tristate CONFIG_NVME_TARGET_TCP, so any 'select' from
it inherits the state of the tristate symbol rather than the intended
CONFIG_NVME_TARGET one that contains the actual call.

The same thing is true for CONFIG_KEYS, which itself is required for
NVME_KEYRING.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/target/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 31633da9427c..e1ebc73f3e5e 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -4,6 +4,8 @@ config NVME_TARGET
 	tristate "NVMe Target support"
 	depends on BLOCK
 	depends on CONFIGFS_FS
+	select NVME_KEYRING if NVME_TARGET_TCP_TLS
+	select KEYS if NVME_TARGET_TCP_TLS
 	select BLK_DEV_INTEGRITY_T10 if BLK_DEV_INTEGRITY
 	select SGL_ALLOC
 	help
@@ -87,9 +89,7 @@ config NVME_TARGET_TCP
 config NVME_TARGET_TCP_TLS
 	bool "NVMe over Fabrics TCP target TLS encryption support"
 	depends on NVME_TARGET_TCP
-	select NVME_KEYRING
 	select NET_HANDSHAKE
-	select KEYS
 	help
 	  Enables TLS encryption for the NVMe TCP target using the netlink handshake API.
 
-- 
2.39.2

[PATCH v3 3/3] nvme: tcp: fix compile-time checks for TLS mode

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

When CONFIG_NVME_KEYRING is enabled as a loadable module, but the TCP
host code is built-in, it fails to link:

arm-linux-gnueabi-ld: drivers/nvme/host/tcp.o: in function `nvme_tcp_setup_ctrl':
tcp.c:(.text+0x1940): undefined reference to `nvme_tls_psk_default'

The problem is that the compile-time conditionals are inconsistent here,
using a mix of #ifdef CONFIG_NVME_TCP_TLS, IS_ENABLED(CONFIG_NVME_TCP_TLS)
and IS_ENABLED(CONFIG_NVME_KEYRING) checks, with CONFIG_NVME_KEYRING
controlling whether the implementation is actually built.

Change it to use IS_ENABLED(CONFIG_NVME_KEYRING) checks consistently,
which should help readability and make it less error-prone. Combining
it with the check for the ctrl->opts->tls flag lets the compiler drop
all the TLS code in configurations without this feature, which also
helps runtime behavior in addition to avoiding the link failure.

To make it possible for the compiler to build the dead code, both
the tls_handshake_timeout variable and the TLS specific members
of nvme_tcp_queue need to be moved out of the #ifdef block as well,
but at least the former of these gets optimized out again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/host/tcp.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 89661a9cf850..ee7aa8478cfb 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -36,11 +36,11 @@ static int so_priority;
 module_param(so_priority, int, 0644);
 MODULE_PARM_DESC(so_priority, "nvme tcp socket optimize priority");
 
-#ifdef CONFIG_NVME_TCP_TLS
 /*
  * TLS handshake timeout
  */
 static int tls_handshake_timeout = 10;
+#ifdef CONFIG_NVME_TCP_TLS
 module_param(tls_handshake_timeout, int, 0644);
 MODULE_PARM_DESC(tls_handshake_timeout,
 		 "nvme TLS handshake timeout in seconds (default 10)");
@@ -161,10 +161,8 @@ struct nvme_tcp_queue {
 	struct ahash_request	*snd_hash;
 	__le32			exp_ddgst;
 	__le32			recv_ddgst;
-#ifdef CONFIG_NVME_TCP_TLS
 	struct completion       tls_complete;
 	int                     tls_err;
-#endif
 	struct page_frag_cache	pf_cache;
 
 	void (*state_change)(struct sock *);
@@ -207,6 +205,14 @@ static inline int nvme_tcp_queue_id(struct nvme_tcp_queue *queue)
 	return queue - queue->ctrl->queues;
 }
 
+static inline bool nvme_tcp_tls(struct nvme_ctrl *ctrl)
+{
+	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
+		return 0;
+
+	return ctrl->opts->tls;
+}
+
 static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)
 {
 	u32 queue_idx = nvme_tcp_queue_id(queue);
@@ -1412,7 +1418,7 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 	memset(&msg, 0, sizeof(msg));
 	iov.iov_base = icresp;
 	iov.iov_len = sizeof(*icresp);
-	if (queue->ctrl->ctrl.opts->tls) {
+	if (nvme_tcp_tls(&queue->ctrl->ctrl)) {
 		msg.msg_control = cbuf;
 		msg.msg_controllen = sizeof(cbuf);
 	}
@@ -1424,7 +1430,7 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 		goto free_icresp;
 	}
 	ret = -ENOTCONN;
-	if (queue->ctrl->ctrl.opts->tls) {
+	if (nvme_tcp_tls(&queue->ctrl->ctrl)) {
 		ctype = tls_get_record_type(queue->sock->sk,
 					    (struct cmsghdr *)cbuf);
 		if (ctype != TLS_RECORD_TYPE_DATA) {
@@ -1548,7 +1554,6 @@ static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
 	queue->io_cpu = cpumask_next_wrap(n - 1, cpu_online_mask, -1, false);
 }
 
-#ifdef CONFIG_NVME_TCP_TLS
 static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
 {
 	struct nvme_tcp_queue *queue = data;
@@ -1625,14 +1630,6 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 	}
 	return ret;
 }
-#else
-static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
-			      struct nvme_tcp_queue *queue,
-			      key_serial_t pskid)
-{
-	return -EPROTONOSUPPORT;
-}
-#endif
 
 static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 				key_serial_t pskid)
@@ -1759,7 +1756,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 	}
 
 	/* If PSKs are configured try to start TLS */
-	if (pskid) {
+	if (IS_ENABLED(CONFIG_NVME_TCP_TLS) && pskid) {
 		ret = nvme_tcp_start_tls(nctrl, queue, pskid);
 		if (ret)
 			goto err_init_connect;
@@ -1916,7 +1913,7 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
 	int ret;
 	key_serial_t pskid = 0;
 
-	if (ctrl->opts->tls) {
+	if (nvme_tcp_tls(ctrl)) {
 		if (ctrl->opts->tls_key)
 			pskid = key_serial(ctrl->opts->tls_key);
 		else
@@ -1949,7 +1946,7 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
 {
 	int i, ret;
 
-	if (ctrl->opts->tls && !ctrl->tls_key) {
+	if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
 		dev_err(ctrl->device, "no PSK negotiated\n");
 		return -ENOKEY;
 	}
-- 
2.39.2

Re: [PATCH v3 0/3] nvme link failure fixes

[Jens Axboe]

On 11/22/23 3:47 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are still a couple of link failures that I tried to address
> with a previous patch. I've split up the missing bits into smaller
> patches and tried to explain the bugs in more detail.
> 
> With these applied, randconfig builds work again. Please either
> merge them or treat them as bug reports and find a different fix,
> I won't do another version.

Applied, but had to hand-apply hunk 9 of patch 3 due to a previous
attempt at this:

commit 23441536b63677cb2ed9b1637d8ca70315e44bd0
Author: Hannes Reinecke <hare@suse.de>
Date:   Tue Nov 14 14:18:21 2023 +0100

    nvme-tcp: only evaluate 'tls' option if TLS is selected

-- 
Jens Axboe

Re: [PATCH v3 0/3] nvme link failure fixes

[Keith Busch]

On Wed, Nov 22, 2023 at 11:47:16PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> There are still a couple of link failures that I tried to address
> with a previous patch. I've split up the missing bits into smaller
> patches and tried to explain the bugs in more detail.
> 
> With these applied, randconfig builds work again. Please either
> merge them or treat them as bug reports and find a different fix,
> I won't do another version.

Jens is taking these directly, but for the record: looks good to me and
thanks for the fixes.

Acked-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH v3 0/3] nvme link failure fixes

[Arnd Bergmann]

On Thu, Nov 23, 2023, at 02:42, Jens Axboe wrote:
> On 11/22/23 3:47 PM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> There are still a couple of link failures that I tried to address
>> with a previous patch. I've split up the missing bits into smaller
>> patches and tried to explain the bugs in more detail.
>> 
>> With these applied, randconfig builds work again. Please either
>> merge them or treat them as bug reports and find a different fix,
>> I won't do another version.
>
> Applied, but had to hand-apply hunk 9 of patch 3 due to a previous
> attempt at this:
>
> commit 23441536b63677cb2ed9b1637d8ca70315e44bd0
> Author: Hannes Reinecke <hare@suse.de>
> Date:   Tue Nov 14 14:18:21 2023 +0100
>
>     nvme-tcp: only evaluate 'tls' option if TLS is selected

Ok, thanks for merging my changes! The commit from Hannes
is what I had in my v1 for this, and it was a correct fix
as well, my patch 3/3 was just a more elaborate way to do
the same thing that I did since Hannes did not like my
version at first.

The 23441536b6 commit was not in linux-next yesterday, so
it looks like our patches crossed paths on the same day.

     Arnd

Re: [PATCH v3 0/3] nvme link failure fixes

[Jens Axboe]

On Wed, 22 Nov 2023 23:47:16 +0100, Arnd Bergmann wrote:
> There are still a couple of link failures that I tried to address
> with a previous patch. I've split up the missing bits into smaller
> patches and tried to explain the bugs in more detail.
> 
> With these applied, randconfig builds work again. Please either
> merge them or treat them as bug reports and find a different fix,
> I won't do another version.
> 
> [...]

Applied, thanks!

[1/3] nvme: target: fix nvme_keyring_id() references
      commit: d78abcbabe7e98bb4baa4dea87550806944790ed
[2/3] nvme: target: fix Kconfig select statements
      commit: 65e2a74c44ddfa174b700f5da2d1d29b4ba6639b
[3/3] nvme: tcp: fix compile-time checks for TLS mode
      commit: 0e6c4fe782e683ff55a27fbb10e9c6b5c241533b

Best regards,
-- 
Jens Axboe

Re: [PATCH v3 0/3] nvme link failure fixes

[Jens Axboe]

On 11/22/23 11:25 PM, Arnd Bergmann wrote:
> On Thu, Nov 23, 2023, at 02:42, Jens Axboe wrote:
>> On 11/22/23 3:47 PM, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> There are still a couple of link failures that I tried to address
>>> with a previous patch. I've split up the missing bits into smaller
>>> patches and tried to explain the bugs in more detail.
>>>
>>> With these applied, randconfig builds work again. Please either
>>> merge them or treat them as bug reports and find a different fix,
>>> I won't do another version.
>>
>> Applied, but had to hand-apply hunk 9 of patch 3 due to a previous
>> attempt at this:
>>
>> commit 23441536b63677cb2ed9b1637d8ca70315e44bd0
>> Author: Hannes Reinecke <hare@suse.de>
>> Date:   Tue Nov 14 14:18:21 2023 +0100
>>
>>     nvme-tcp: only evaluate 'tls' option if TLS is selected
> 
> Ok, thanks for merging my changes! The commit from Hannes
> is what I had in my v1 for this, and it was a correct fix
> as well, my patch 3/3 was just a more elaborate way to do
> the same thing that I did since Hannes did not like my
> version at first.
> 
> The 23441536b6 commit was not in linux-next yesterday, so
> it looks like our patches crossed paths on the same day.

yeah, it was just recently merged, which is probably why it wasn't there
yet and you didn't see it. Not a big deal, easy enough to resolve - just
wanted to make sure you knew about it. It should all be heading upstream
shortly, just sent out the pull.

-- 
Jens Axboe