Messages in this thread | | | Date | Fri, 20 Oct 2023 15:50:42 +0200 | Subject | Re: [PATCH 2/2] nvme: keyring: fix conditional compilation | From | Hannes Reinecke <> |
| |
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
| |