From: Arnd Bergmann <[email protected]>
When only the keyring module is included but auth is not, modpost
complains about the lack of a module license tag:
ERROR: modpost: missing MODULE_LICENSE() in drivers/nvme/common/nvme-common.o
Address this by making both modules buildable standalone,
removing the now unnecessary CONFIG_NVME_COMMON symbol
in the process.
Fixes: 9d77eb5277849 ("nvme-keyring: register '.nvme' keyring")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/nvme/Makefile | 2 +-
drivers/nvme/common/Kconfig | 4 ++--
drivers/nvme/common/Makefile | 7 ++++---
drivers/nvme/common/keyring.c | 2 ++
drivers/nvme/host/Kconfig | 2 --
drivers/nvme/target/Kconfig | 2 --
include/linux/nvme-keyring.h | 2 +-
7 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/nvme/Makefile b/drivers/nvme/Makefile
index eedca8c720983..74f59ceed3d5a 100644
--- a/drivers/nvme/Makefile
+++ b/drivers/nvme/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_NVME_COMMON) += common/
+obj-y += common/
obj-y += host/
obj-y += target/
diff --git a/drivers/nvme/common/Kconfig b/drivers/nvme/common/Kconfig
index 06c8df00d1e21..63d4fd45999dc 100644
--- a/drivers/nvme/common/Kconfig
+++ b/drivers/nvme/common/Kconfig
@@ -4,11 +4,11 @@ config NVME_COMMON
tristate
config NVME_KEYRING
- bool
+ tristate
select KEYS
config NVME_AUTH
- bool
+ tristate
select CRYPTO
select CRYPTO_HMAC
select CRYPTO_SHA256
diff --git a/drivers/nvme/common/Makefile b/drivers/nvme/common/Makefile
index 0cbd0b0b8d499..681514cf2e2f5 100644
--- a/drivers/nvme/common/Makefile
+++ b/drivers/nvme/common/Makefile
@@ -2,7 +2,8 @@
ccflags-y += -I$(src)
-obj-$(CONFIG_NVME_COMMON) += nvme-common.o
+obj-$(CONFIG_NVME_AUTH) += nvme-auth.o
+obj-$(CONFIG_NVME_KEYRING) += nvme-keyring.o
-nvme-common-$(CONFIG_NVME_AUTH) += auth.o
-nvme-common-$(CONFIG_NVME_KEYRING) += keyring.o
+nvme-auth-y += auth.o
+nvme-keyring-y += keyring.o
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index f8d9a208397b4..46d7a537dbc2e 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -180,3 +180,5 @@ void nvme_keyring_exit(void)
key_put(nvme_keyring);
}
EXPORT_SYMBOL_GPL(nvme_keyring_exit);
+
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 48f7d72de5e9a..8fe2dd619e80e 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -95,7 +95,6 @@ config NVME_TCP
config NVME_TCP_TLS
bool "NVMe over Fabrics TCP TLS encryption support"
depends on NVME_TCP
- select NVME_COMMON
select NVME_KEYRING
select NET_HANDSHAKE
select KEYS
@@ -110,7 +109,6 @@ config NVME_TCP_TLS
config NVME_HOST_AUTH
bool "NVM Express over Fabrics In-Band Authentication"
depends on NVME_CORE
- select NVME_COMMON
select NVME_AUTH
help
This provides support for NVMe over Fabrics In-Band Authentication.
diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index fa479c9f5c3d3..31633da9427c7 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -87,7 +87,6 @@ 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_COMMON
select NVME_KEYRING
select NET_HANDSHAKE
select KEYS
@@ -102,7 +101,6 @@ config NVME_TARGET_TCP_TLS
config NVME_TARGET_AUTH
bool "NVMe over Fabrics In-band Authentication support"
depends on NVME_TARGET
- select NVME_COMMON
select NVME_AUTH
help
This enables support for NVMe over Fabrics In-band Authentication
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index 4efea9dd967c1..6cc0696625f36 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -6,7 +6,7 @@
#ifndef _NVME_KEYRING_H
#define _NVME_KEYRING_H
-#ifdef CONFIG_NVME_KEYRING
+#if IS_ENABLED(CONFIG_NVME_KEYRING)
key_serial_t nvme_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn);
--
2.39.2
From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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);
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) {
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");
--
2.39.2
On 10/20/23 15:05, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> When only the keyring module is included but auth is not, modpost
> complains about the lack of a module license tag:
>
> ERROR: modpost: missing MODULE_LICENSE() in drivers/nvme/common/nvme-common.o
>
> Address this by making both modules buildable standalone,
> removing the now unnecessary CONFIG_NVME_COMMON symbol
> in the process.
>
> Fixes: 9d77eb5277849 ("nvme-keyring: register '.nvme' keyring")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/nvme/Makefile | 2 +-
> drivers/nvme/common/Kconfig | 4 ++--
> drivers/nvme/common/Makefile | 7 ++++---
> drivers/nvme/common/keyring.c | 2 ++
> drivers/nvme/host/Kconfig | 2 --
> drivers/nvme/target/Kconfig | 2 --
> include/linux/nvme-keyring.h | 2 +-
> 7 files changed, 10 insertions(+), 11 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
On 10/20/23 15:05, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Fri, Oct 20, 2023, at 15:50, Hannes Reinecke wrote:
> On 10/20/23 15:05, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> 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.
That seems to add a lot of complexity, but I can try. If I can't
figure it out, someone else might have to try it. Since we need
to check separately for the host and target options, this will
lead to having two extra stubs per function call, right?
key_serial_t nvme_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn);
static inline key_serial_t nvme_host_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn)
{
if (IS_ENABLED(CONFIG_NVME_TCP_TLS)
return nvme_tls_psk_default(keyring, hostnqn, subnqn);
return 0;
}
static inline key_serial_t nvme_target_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn)
{
if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
return nvme_host_tls_psk_default(keyring, hostnqn, subnqn);
return 0;
}
>> 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
It's the nvme_tls_psk_default() call that needs to be protected
here, but I found that the entire code block is dead if tls_key
is false, so it seemed more logical to make the entire block
conditional when we know the condition is always false at
compile time.
Arnd