Re: [PATCH 2/2] nvme: keyring: fix conditional compilation

Hannes Reinecke
2023-10-20T13:50:54Z

On 10/20/23 15:05, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The keyring and auth functions can be called from both the host and
> the target side and are controlled by Kconfig options for each of the
> combinations, but the declarations are controlled by #ifdef checks
> on the shared Kconfig symbols.
>
> This leads to link failures in combinations where one of the frontends
> is built-in and the other one is a module, and the keyring code
> ends up in a module that is not reachable from the builtin code:
>
> ld: drivers/nvme/host/core.o: in function `nvme_core_exit':
> core.c:(.exit.text+0x4): undefined reference to `nvme_keyring_exit'
> ld: drivers/nvme/host/core.o: in function `nvme_core_init':
> core.c:(.init.text+0x94): undefined reference to `nvme_keyring_init
>
> ld: drivers/nvme/host/tcp.o: in function `nvme_tcp_setup_ctrl':
> tcp.c:(.text+0x4c18): undefined reference to `nvme_tls_psk_default'
>
> Address this by adding compile-time checks around the callers where
> needed, based on whether the functionality is actually used for
> the target and host side, respectively.
>
> In Kconfig, this requires changing the 'select NVME_KEYRING'
> since the keyring calls are done from the host core module,
> which may be built-in even when the tcp front-end is in a loadable
> module.
>
> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/nvme/host/Kconfig | 2 +-
> drivers/nvme/host/core.c | 16 +++++++++++-----
> drivers/nvme/host/tcp.c | 2 +-
> drivers/nvme/target/configfs.c | 2 +-
> 4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 8fe2dd619e80e..2d53c23f0a483 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -2,6 +2,7 @@
> config NVME_CORE
> tristate
> select BLK_DEV_INTEGRITY_T10 if BLK_DEV_INTEGRITY
> + select NVME_KEYRING if NVME_TCP_TLS
>
> config BLK_DEV_NVME
> tristate "NVM Express block device"
> @@ -95,7 +96,6 @@ config NVME_TCP
> config NVME_TCP_TLS
> bool "NVMe over Fabrics TCP TLS encryption support"
> depends on NVME_TCP
> - select NVME_KEYRING
> select NET_HANDSHAKE
> select KEYS
> help
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 62612f87aafa2..ac92534f6da90 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4724,16 +4724,20 @@ static int __init nvme_core_init(void)
> result = PTR_ERR(nvme_ns_chr_class);
> goto unregister_generic_ns;
> }
> - result = nvme_keyring_init();
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + result = nvme_keyring_init();
> if (result)
> goto destroy_ns_chr;
> - result = nvme_init_auth();
> +
> + if (IS_ENABLED(CONFIG_NVME_HOST_AUTH))
> + result = nvme_init_auth();
> if (result)
> goto keyring_exit;
> return 0;
>
> keyring_exit:
> - nvme_keyring_exit();
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + nvme_keyring_exit();
> destroy_ns_chr:
> class_destroy(nvme_ns_chr_class);
> unregister_generic_ns:
> @@ -4756,8 +4760,10 @@ static int __init nvme_core_init(void)
>
> static void __exit nvme_core_exit(void)
> {
> - nvme_exit_auth();
> - nvme_keyring_exit();
> + if (IS_ENABLED(CONFIG_NVME_HOST_AUTH))
> + nvme_exit_auth();
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + nvme_keyring_exit();
> class_destroy(nvme_ns_chr_class);
> class_destroy(nvme_subsys_class);
> class_destroy(nvme_class);

Please add stub calls and avoid sprinkle the code with
IS_ENABLED statements.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4714a902f4caa..e2b90789c0407 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1915,7 +1915,7 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
> int ret;
> key_serial_t pskid = 0;
>
> - if (ctrl->opts->tls) {
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS) && ctrl->opts->tls) {

Why? '->tls' is not protected by a CONFIG options, and should be
available in general ...

> if (ctrl->opts->tls_key)
> pskid = key_serial(ctrl->opts->tls_key);
> else
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 9eed6e6765eaa..e307a044b1a1b 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");
Please make this a stub.

Cheers,

Hannes