[PATCH 0/2] [v4] nvme: fixup module compilation

[PATCH 0/2] [v4] nvme: fixup module compilation

[Hannes Reinecke]

Hi all,

Arnd noticed that the module selection between keyring and
host/target code fails under certain combinations.
This patchset addresses this by making 'keyring' into a
'proper' module by adding module_init()/module_exit() functions
and ensure that the module will always be compiled in when
one of the dependent modules are selected.

Arnd Bergmann (1):
  nvme: common: make keyring and auth separate modules

Hannes Reinecke (1):
  nvme: keyring: fix conditional compilation

 drivers/nvme/Makefile         |  2 +-
 drivers/nvme/common/Kconfig   |  5 +----
 drivers/nvme/common/Makefile  |  7 ++++---
 drivers/nvme/common/keyring.c | 11 +++++++----
 drivers/nvme/host/Kconfig     |  4 +---
 drivers/nvme/host/core.c      |  9 +--------
 drivers/nvme/target/Kconfig   |  4 +---
 include/linux/nvme-keyring.h  |  8 --------
 8 files changed, 16 insertions(+), 34 deletions(-)

-- 
2.35.3

[PATCH 1/2] nvme: common: make keyring and auth separate modules

[Hannes Reinecke]

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/Makefile         | 2 +-
 drivers/nvme/common/Kconfig   | 7 ++-----
 drivers/nvme/common/Makefile  | 7 ++++---
 drivers/nvme/common/keyring.c | 2 ++
 drivers/nvme/host/Kconfig     | 2 --
 drivers/nvme/target/Kconfig   | 2 --
 6 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/Makefile b/drivers/nvme/Makefile
index eedca8c72098..74f59ceed3d5 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 06c8df00d1e2..244432e0b73d 100644
--- a/drivers/nvme/common/Kconfig
+++ b/drivers/nvme/common/Kconfig
@@ -1,14 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-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 0cbd0b0b8d49..681514cf2e2f 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 f8d9a208397b..46d7a537dbc2 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 48f7d72de5e9..8fe2dd619e80 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 fa479c9f5c3d..31633da9427c 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
-- 
2.35.3

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

[Hannes Reinecke]

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 moving nvme_auth_init()/nvme_auth_exit() into
module init/exit functions for the keyring module and make sure
that the keyring module is always built in when one of the
dependent modules are selected.

Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/common/Kconfig   | 2 +-
 drivers/nvme/common/keyring.c | 9 +++++----
 drivers/nvme/host/Kconfig     | 2 +-
 drivers/nvme/host/core.c      | 9 +--------
 drivers/nvme/target/Kconfig   | 2 +-
 include/linux/nvme-keyring.h  | 8 --------
 6 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/drivers/nvme/common/Kconfig b/drivers/nvme/common/Kconfig
index 244432e0b73d..96031016079f 100644
--- a/drivers/nvme/common/Kconfig
+++ b/drivers/nvme/common/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 config NVME_KEYRING
-       tristate
+       bool
        select KEYS
 
 config NVME_AUTH
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 46d7a537dbc2..ee341b83eeba 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -151,7 +151,7 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 }
 EXPORT_SYMBOL_GPL(nvme_tls_psk_default);
 
-int nvme_keyring_init(void)
+static int __init nvme_keyring_init(void)
 {
 	int err;
 
@@ -171,14 +171,15 @@ int nvme_keyring_init(void)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_init);
 
-void nvme_keyring_exit(void)
+static void __exit nvme_keyring_exit(void)
 {
 	unregister_key_type(&nvme_tls_psk_key_type);
 	key_revoke(nvme_keyring);
 	key_put(nvme_keyring);
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_exit);
 
 MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Hannes Reinecke <hare@suse.de>");
+module_init(nvme_keyring_init);
+module_exit(nvme_keyring_exit);
diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 8fe2dd619e80..ff6a8af10646 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -95,7 +95,7 @@ config NVME_TCP
 config NVME_TCP_TLS
 	bool "NVMe over Fabrics TCP TLS encryption support"
 	depends on NVME_TCP
-	select NVME_KEYRING
+	select NVME_KEYRING if NVME_TCP_TLS
 	select NET_HANDSHAKE
 	select KEYS
 	help
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f48b4f735d2d..47645a219128 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -25,7 +25,6 @@
 #include "nvme.h"
 #include "fabrics.h"
 #include <linux/nvme-auth.h>
-#include <linux/nvme-keyring.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -4724,16 +4723,11 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_ns_chr_class);
 		goto unregister_generic_ns;
 	}
-	result = nvme_keyring_init();
-	if (result)
-		goto destroy_ns_chr;
 	result = nvme_init_auth();
 	if (result)
-		goto keyring_exit;
+		goto destroy_ns_chr;
 	return 0;
 
-keyring_exit:
-	nvme_keyring_exit();
 destroy_ns_chr:
 	class_destroy(nvme_ns_chr_class);
 unregister_generic_ns:
@@ -4757,7 +4751,6 @@ static int __init nvme_core_init(void)
 static void __exit nvme_core_exit(void)
 {
 	nvme_exit_auth();
-	nvme_keyring_exit();
 	class_destroy(nvme_ns_chr_class);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 31633da9427c..9fe74b771fa3 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -87,7 +87,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 NVME_KEYRING if NVME_TARGET_TCP_TLS
 	select NET_HANDSHAKE
 	select KEYS
 	help
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index 4efea9dd967c..2095382de103 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -12,8 +12,6 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn);
 
 key_serial_t nvme_keyring_id(void);
-int nvme_keyring_init(void);
-void nvme_keyring_exit(void);
 
 #else
 
@@ -26,11 +24,5 @@ static inline key_serial_t nvme_keyring_id(void)
 {
 	return 0;
 }
-static inline int nvme_keyring_init(void)
-{
-	return 0;
-}
-static inline void nvme_keyring_exit(void) {}
-
 #endif /* !CONFIG_NVME_KEYRING */
 #endif /* _NVME_KEYRING_H */
-- 
2.35.3

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

[Arnd Bergmann]

On Thu, Oct 26, 2023, at 15:08, Hannes Reinecke wrote:
> diff --git a/drivers/nvme/common/Kconfig b/drivers/nvme/common/Kconfig
> index 244432e0b73d..96031016079f 100644
> --- a/drivers/nvme/common/Kconfig
> +++ b/drivers/nvme/common/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> 
>  config NVME_KEYRING
> -       tristate
> +       bool
>         select KEYS

I guess that's one way to address the link failure ;-)

It feels like cheating to force it built-in even if
both target and host support is in loadable module.

     Arnd

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

[Hannes Reinecke]

On 10/26/23 15:37, Arnd Bergmann wrote:
> On Thu, Oct 26, 2023, at 15:08, Hannes Reinecke wrote:
>> diff --git a/drivers/nvme/common/Kconfig b/drivers/nvme/common/Kconfig
>> index 244432e0b73d..96031016079f 100644
>> --- a/drivers/nvme/common/Kconfig
>> +++ b/drivers/nvme/common/Kconfig
>> @@ -1,7 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>
>>   config NVME_KEYRING
>> -       tristate
>> +       bool
>>          select KEYS
> 
> I guess that's one way to address the link failure ;-)
> 
> It feels like cheating to force it built-in even if
> both target and host support is in loadable module.
> 
Arguably.
But the decision matrix really has only limited choices:

Host Target Keyring
n    n      n
n    m      m
n    y      y
m    n      m
m    m      m
m    y      y
y    n      y
y    m      y
y    y      y

So we're correct in 75% of all cases :-)
And before we trying to figure out some weird complex kconfig syntax
to get all cases correct I prefer the easy solution.
Plus it has the benefit that the keyring is avialable right from the
start, so you can pre-provision keys even before nvme is loaded.

Cheers,

Hannes

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

[Arnd Bergmann]

On Thu, Oct 26, 2023, at 16:20, Hannes Reinecke wrote:
> On 10/26/23 15:37, Arnd Bergmann wrote:
>> On Thu, Oct 26, 2023, at 15:08, Hannes Reinecke wrote:
>>> diff --git a/drivers/nvme/common/Kconfig b/drivers/nvme/common/Kconfig
>>> index 244432e0b73d..96031016079f 100644
>>> --- a/drivers/nvme/common/Kconfig
>>> +++ b/drivers/nvme/common/Kconfig
>>> @@ -1,7 +1,7 @@
>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>
>>>   config NVME_KEYRING
>>> -       tristate
>>> +       bool
>>>          select KEYS
>> 
>> I guess that's one way to address the link failure ;-)
>> 
>> It feels like cheating to force it built-in even if
>> both target and host support is in loadable module.
>> 
> Arguably.
> But the decision matrix really has only limited choices:
>
> Host Target Keyring
> n    n      n
> n    m      m
> n    y      y
> m    n      m
> m    m      m
> m    y      y
> y    n      y
> y    m      y
> y    y      y
>
> So we're correct in 75% of all cases :-)
> And before we trying to figure out some weird complex kconfig syntax
> to get all cases correct I prefer the easy solution.
> Plus it has the benefit that the keyring is avialable right from the
> start, so you can pre-provision keys even before nvme is loaded.

Your original version already had the logic for doing
this part right and always linking the "common" module
as built-in if needed. Maybe just replace the "PATCH 1/2"
with a different approach then.

My feeling is still that my v1 was the simplest solution,
but that does exactly the right thing in the end, but if
you absolutely want to keep the #if/#else block in the
header instead of the "if (IS_ENABLED())" checks, then
you could also get there by moving the bits that
are actually common (the MODULE_LICENSE, MODULE_AUTHOR,
module_init and module_exit tags) into a third file
that is always part of nvme-common.ko.

     Arnd

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

[Christoph Hellwig]

On Thu, Oct 26, 2023 at 04:20:12PM +0200, Hannes Reinecke wrote:
> Host Target Keyring
> n    n      n
> n    m      m
> n    y      y
> m    n      m
> m    m      m
> m    y      y
> y    n      y
> y    m      y
> y    y      y
>
> So we're correct in 75% of all cases :-)
> And before we trying to figure out some weird complex kconfig syntax
> to get all cases correct I prefer the easy solution.
> Plus it has the benefit that the keyring is avialable right from the
> start, so you can pre-provision keys even before nvme is loaded.

in the 75% of cases that don't really matter, as 99% of all setups
have nvme and nvmet built modular, and for that you now build code
into the kernel for no good reason at all.

FYI, what's I've done a lot in the past for such simple helper is
to not have a Kconfig symbol at all, but let the Makefile handle
it.

A

obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o

will actually do the right thing here without much complicated
boilerplate.

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

[Hannes Reinecke]

On 10/27/23 07:21, Christoph Hellwig wrote:
> On Thu, Oct 26, 2023 at 04:20:12PM +0200, Hannes Reinecke wrote:
>> Host Target Keyring
>> n    n      n
>> n    m      m
>> n    y      y
>> m    n      m
>> m    m      m
>> m    y      y
>> y    n      y
>> y    m      y
>> y    y      y
>>
>> So we're correct in 75% of all cases :-)
>> And before we trying to figure out some weird complex kconfig syntax
>> to get all cases correct I prefer the easy solution.
>> Plus it has the benefit that the keyring is avialable right from the
>> start, so you can pre-provision keys even before nvme is loaded.
> 
> in the 75% of cases that don't really matter, as 99% of all setups
> have nvme and nvmet built modular, and for that you now build code
> into the kernel for no good reason at all.
> 
> FYI, what's I've done a lot in the past for such simple helper is
> to not have a Kconfig symbol at all, but let the Makefile handle
> it.
> 
> A
> 
> obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
> obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o
> 
> will actually do the right thing here without much complicated
> boilerplate.
> 
In principle. Unfortunately I have to initialize the keyring, and that
can be done only once.
I see if I can come up with a different solution.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Arnd Bergmann]

On Fri, Oct 27, 2023, at 08:01, Hannes Reinecke wrote:
> On 10/27/23 07:21, Christoph Hellwig wrote:
>> On Thu, Oct 26, 2023 at 04:20:12PM +0200, Hannes Reinecke wrote:
>>> So we're correct in 75% of all cases :-)
>>> And before we trying to figure out some weird complex kconfig syntax
>>> to get all cases correct I prefer the easy solution.
>>> Plus it has the benefit that the keyring is avialable right from the
>>> start, so you can pre-provision keys even before nvme is loaded.
>> 
>> in the 75% of cases that don't really matter, as 99% of all setups
>> have nvme and nvmet built modular, and for that you now build code
>> into the kernel for no good reason at all.
>> 
>> FYI, what's I've done a lot in the past for such simple helper is
>> to not have a Kconfig symbol at all, but let the Makefile handle
>> it.
>> 
>> A
>> 
>> obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
>> obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o
>> 
>> will actually do the right thing here without much complicated
>> boilerplate.

Right, this is what I meant referring to the usual Kconfig
or Makefile logic. The example above needs an extra conditional
here to deal with the keyring code being turned on or off
altogether in addition to being used from any combination
of built-in or modular host/target modules, but none of this
should require unusual hacks.
 
> In principle. Unfortunately I have to initialize the keyring, and that
> can be done only once.
> I see if I can come up with a different solution.

If the keyring has to be initialized first, I think the safest
way is to move it to a different initcall level, to avoid
relying on link order for the everything-built-in case. The
other cases are handled automatically once you get it to
link properly, as module load order takes care of the
all-modular case, and with the keyring built-in it also comes
before any modules.

     Arnd

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

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 10:12:11AM +0200, Arnd Bergmann wrote:
> > In principle. Unfortunately I have to initialize the keyring, and that
> > can be done only once.
> > I see if I can come up with a different solution.
> 
> If the keyring has to be initialized first, I think the safest
> way is to move it to a different initcall level, to avoid
> relying on link order for the everything-built-in case.

I don't really mind the link order, for these cases I usually add a
comment to the Makefile so that people don't accidentally change it.

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

[Hannes Reinecke]

On 10/27/23 10:30, Christoph Hellwig wrote:
> On Fri, Oct 27, 2023 at 10:12:11AM +0200, Arnd Bergmann wrote:
>>> In principle. Unfortunately I have to initialize the keyring, and that
>>> can be done only once.
>>> I see if I can come up with a different solution.
>>
>> If the keyring has to be initialized first, I think the safest
>> way is to move it to a different initcall level, to avoid
>> relying on link order for the everything-built-in case.
> 
> I don't really mind the link order, for these cases I usually add a
> comment to the Makefile so that people don't accidentally change it.

Point is not that it has to be initialized first, point is it has to be 
initialized only _once_.
So when moving it into a separate module we cannot use the Makefile 
trick from Christoph, and when initialized it in nvme-core we have a
dependency on nvme-core from nvmet.
Which would be the easiest way (ie have nvmet dependent on nvme-core),
but no sure if that's the way we want to go.

Cheers,

Hannes

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

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 10:54:42AM +0200, Hannes Reinecke wrote:
>> I don't really mind the link order, for these cases I usually add a
>> comment to the Makefile so that people don't accidentally change it.
>
> Point is not that it has to be initialized first, point is it has to be 
> initialized only _once_.

module_init ensure it is only initialized once except for the case where
the module is unloaded and reloaded, in which case you actually do need
to initialize it again.

> So when moving it into a separate module we cannot use the Makefile trick 
> from Christoph

why?

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

[Hannes Reinecke]

On 10/27/23 10:56, Christoph Hellwig wrote:
> On Fri, Oct 27, 2023 at 10:54:42AM +0200, Hannes Reinecke wrote:
>>> I don't really mind the link order, for these cases I usually add a
>>> comment to the Makefile so that people don't accidentally change it.
>>
>> Point is not that it has to be initialized first, point is it has to be
>> initialized only _once_.
> 
> module_init ensure it is only initialized once except for the case where
> the module is unloaded and reloaded, in which case you actually do need
> to initialize it again.
> 
So if I do this:

obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o

and mod-common contains a 'module_init()' and a 'module_exit()' 
function, what happens if I load mod2 after mod1?
And what happens if I unload mod2, but keep mod1?

Cheers,

Hannes

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

[Arnd Bergmann]

On Fri, Oct 27, 2023, at 11:08, Hannes Reinecke wrote:
> On 10/27/23 10:56, Christoph Hellwig wrote:
>> On Fri, Oct 27, 2023 at 10:54:42AM +0200, Hannes Reinecke wrote:
>>>> I don't really mind the link order, for these cases I usually add a
>>>> comment to the Makefile so that people don't accidentally change it.
>>>
>>> Point is not that it has to be initialized first, point is it has to be
>>> initialized only _once_.
>> 
>> module_init ensure it is only initialized once except for the case where
>> the module is unloaded and reloaded, in which case you actually do need
>> to initialize it again.
>> 
> So if I do this:
>
> obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
> obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o
>
> and mod-common contains a 'module_init()' and a 'module_exit()' 
> function, what happens if I load mod2 after mod1?
> And what happens if I unload mod2, but keep mod1?

In the syntax above, you get three loadable modules:
mod1.ko, mod2.ko and mod-common.ko. Loading either
mod1.ko or mod2.ko leads to mod-common.ko getting loaded
and intialized first, and it remains loaded until both
mod1 and mod2 get removed. If one of the two symbols is
set to =y, then mod-common.o becomes part of vmlinux,
gets initialized first and cannot get removed.

This is different from the invalid example

obj-$(CONFIG_MOD1)	+= mod1_module.o
obj-$(CONFIG_MOD2)	+= mod2_module.o

mod1_module-y += mod1.o mod-common.o
mod2_module-y += mod2.o mod-common.o

which is probably what you were thinking of. In this
case, we'd get two modules mod1_module.ko and mod2_module.ko,
with mod-common.o getting compiled and linked separately
into each one if they are both =m, or a single copy
linked into vmlinux if they are both =y. Kbuild now
warns if you attempt to do this, because it causes
a number of problems.

    Arnd

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

[Christoph Hellwig]

On Fri, Oct 27, 2023 at 11:08:23AM +0200, Hannes Reinecke wrote:
> obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
> obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o
>
> and mod-common contains a 'module_init()' and a 'module_exit()' function, 
> what happens if I load mod2 after mod1?

mod-common is already loaded and won't be loaded again.

> And what happens if I unload mod2, but keep mod1?

mod-common doesn't become available for unloading until all
modules using it are unloaded first.

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

[Keith Busch]

On Fri, Oct 27, 2023 at 11:21:50AM +0200, Christoph Hellwig wrote:
> On Fri, Oct 27, 2023 at 11:08:23AM +0200, Hannes Reinecke wrote:
> > obj-$(CONFIG_MOD1)	+= mod1.o mod-common.o
> > obj-$(CONFIG_MOD2)	+= mod2.o mod-common.o
> >
> > and mod-common contains a 'module_init()' and a 'module_exit()' function, 
> > what happens if I load mod2 after mod1?
> 
> mod-common is already loaded and won't be loaded again.
> 
> > And what happens if I unload mod2, but keep mod1?
> 
> mod-common doesn't become available for unloading until all
> modules using it are unloaded first.

Looks like this works, atop patch 1:

---
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 46d7a537dbc2e..c35b3a287a910 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -151,7 +151,7 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 }
 EXPORT_SYMBOL_GPL(nvme_tls_psk_default);
 
-int nvme_keyring_init(void)
+static int __init nvme_keyring_init(void)
 {
 	int err;
 
@@ -171,14 +171,14 @@ int nvme_keyring_init(void)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_init);
 
-void nvme_keyring_exit(void)
+static void __exit nvme_keyring_exit(void)
 {
 	unregister_key_type(&nvme_tls_psk_key_type);
 	key_revoke(nvme_keyring);
 	key_put(nvme_keyring);
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_exit);
 
 MODULE_LICENSE("GPL v2");
+module_init(nvme_keyring_init);
+module_exit(nvme_keyring_exit);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 75a1b58a7a436..683b694afdead 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4737,16 +4737,11 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_ns_chr_class);
 		goto unregister_generic_ns;
 	}
-	result = nvme_keyring_init();
-	if (result)
-		goto destroy_ns_chr;
 	result = nvme_init_auth();
 	if (result)
-		goto keyring_exit;
+		goto destroy_ns_chr;
 	return 0;
 
-keyring_exit:
-	nvme_keyring_exit();
 destroy_ns_chr:
 	class_destroy(nvme_ns_chr_class);
 unregister_generic_ns:
@@ -4770,7 +4765,6 @@ static int __init nvme_core_init(void)
 static void __exit nvme_core_exit(void)
 {
 	nvme_exit_auth();
-	nvme_keyring_exit();
 	class_destroy(nvme_ns_chr_class);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index 4efea9dd967c1..f4f6634e85846 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 46d7a537dbc2e..c35b3a287a910 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -151,7 +151,7 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 }
 EXPORT_SYMBOL_GPL(nvme_tls_psk_default);
 
-int nvme_keyring_init(void)
+static int __init nvme_keyring_init(void)
 {
 	int err;
 
@@ -171,14 +171,14 @@ int nvme_keyring_init(void)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_init);
 
-void nvme_keyring_exit(void)
+static void __exit nvme_keyring_exit(void)
 {
 	unregister_key_type(&nvme_tls_psk_key_type);
 	key_revoke(nvme_keyring);
 	key_put(nvme_keyring);
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_exit);
 
 MODULE_LICENSE("GPL v2");
+module_init(nvme_keyring_init);
+module_exit(nvme_keyring_exit);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 75a1b58a7a436..683b694afdead 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4737,16 +4737,11 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_ns_chr_class);
 		goto unregister_generic_ns;
 	}
-	result = nvme_keyring_init();
-	if (result)
-		goto destroy_ns_chr;
 	result = nvme_init_auth();
 	if (result)
-		goto keyring_exit;
+		goto destroy_ns_chr;
 	return 0;
 
-keyring_exit:
-	nvme_keyring_exit();
 destroy_ns_chr:
 	class_destroy(nvme_ns_chr_class);
 unregister_generic_ns:
@@ -4770,7 +4765,6 @@ static int __init nvme_core_init(void)
 static void __exit nvme_core_exit(void)
 {
 	nvme_exit_auth();
-	nvme_keyring_exit();
 	class_destroy(nvme_ns_chr_class);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index 4efea9dd967c1..f4f6634e85846 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -6,14 +6,12 @@
 #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);
 
 key_serial_t nvme_keyring_id(void);
-int nvme_keyring_init(void);
-void nvme_keyring_exit(void);
 
 #else
 
@@ -26,11 +24,6 @@ static inline key_serial_t nvme_keyring_id(void)
 {
 	return 0;
 }
-static inline int nvme_keyring_init(void)
-{
-	return 0;
-}
-static inline void nvme_keyring_exit(void) {}
 
 #endif /* !CONFIG_NVME_KEYRING */
 #endif /* _NVME_KEYRING_H */
@@ -6,14 +6,12 @@
 #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);
 
 key_serial_t nvme_keyring_id(void);
-int nvme_keyring_init(void);
-void nvme_keyring_exit(void);
 
 #else
 
@@ -26,11 +24,6 @@ static inline key_serial_t nvme_keyring_id(void)
 {
 	return 0;
 }
-static inline int nvme_keyring_init(void)
-{
-	return 0;
-}
-static inline void nvme_keyring_exit(void) {}
 
 #endif /* !CONFIG_NVME_KEYRING */
 #endif /* _NVME_KEYRING_H */
--

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

[Christoph Hellwig]

On Wed, Oct 25, 2023 at 05:35:12PM +0200, Arnd Bergmann wrote:
> On Wed, Oct 25, 2023, at 17:00, Hannes Reinecke wrote:
> > On 10/25/23 14:16, Arnd Bergmann wrote:
> >> On Wed, Oct 25, 2023, at 11:11, Hannes Reinecke wrote:
> >>> On 10/25/23 10:20, Arnd Bergmann wrote:
> >
> > 'IS_REACHABLE' is the keyword here; if we use that in nvme-keyring.h
> > things are resolved.
> >
> > Will be sending an updated patch.
> 
> Please don't use IS_REACHABLE(), it's almost never the right
> solution, even if it avoids this particular build failure,
> it tends to cause hard to debug problems when features that
> are enabled as modules end up not working correctly.

Agreed.

> Another option would be to use Kconfig or Makefile logic
> to force it to be built-in if at least one of the two
> callers is built-in and at least one of them tries to
> use it.

That's the only sensible thing to do, a that is what the user
really wants.

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

[Arnd Bergmann]

On Wed, Oct 25, 2023, at 17:00, Hannes Reinecke wrote:
> On 10/25/23 14:16, Arnd Bergmann wrote:
>> On Wed, Oct 25, 2023, at 11:11, Hannes Reinecke wrote:
>>> On 10/25/23 10:20, Arnd Bergmann wrote:
>
> 'IS_REACHABLE' is the keyword here; if we use that in nvme-keyring.h
> things are resolved.
>
> Will be sending an updated patch.

Please don't use IS_REACHABLE(), it's almost never the right
solution, even if it avoids this particular build failure,
it tends to cause hard to debug problems when features that
are enabled as modules end up not working correctly.

I still think my original patch or a variation of it is
the best option since it keys the usage of the module
directly on whether the caller actually wants to use it.

Another option would be to use Kconfig or Makefile logic
to force it to be built-in if at least one of the two
callers is built-in and at least one of them tries to
use it.

     Arnd

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

[Hannes Reinecke]

On 10/25/23 14:16, Arnd Bergmann wrote:
> On Wed, Oct 25, 2023, at 11:11, Hannes Reinecke wrote:
>> On 10/25/23 10:20, Arnd Bergmann wrote:
>>> On Wed, Oct 25, 2023, at 10:12, Hannes Reinecke 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 moving nvme_auth_init()/nvme_auth_exit() into
>>>> module init/exit functions for the keyring module.
>>>>
>>>> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>
>>> Your patch looks good to me, and I think this fixes a
>>> separate problem with missing the initialization of the
>>> keyring when the kernel has only the target driver but
>>> no host support, but it has nothing to do with the changelog
>>> I wrote above and does not fix the build regression
>>> I described.
>>>
>> Hmm. Can you send me the .config file (or the config options for NVMe
>> where there failure occurred)? I tried the configurations mentioned, and
>> it worked for me.
>> But apparently I didn't get the right combination...
> 
> Sorry if my explanations were not clear enough. The two
> broken cases can be triggered using:
> 
> https://pastebin.com/aVQ1UbTv
> CONFIG_NVME_KEYRING=m
> CONFIG_NVME_AUTH=m
> CONFIG_NVME_CORE=y
> CONFIG_NVME_MULTIPATH=y
> CONFIG_NVME_VERBOSE_ERRORS=y
> CONFIG_NVME_FABRICS=y
> CONFIG_NVME_FC=y
> CONFIG_NVME_TCP=y
> # CONFIG_NVME_TCP_TLS is not set
> # CONFIG_NVME_HOST_AUTH is not set
> CONFIG_NVME_TARGET=m
> # CONFIG_NVME_TARGET_PASSTHRU is not set
> # CONFIG_NVME_TARGET_LOOP is not set
> CONFIG_NVME_TARGET_FC=m
> CONFIG_NVME_TARGET_FCLOOP=m
> CONFIG_NVME_TARGET_TCP=m
> CONFIG_NVME_TARGET_TCP_TLS=y
> CONFIG_NVME_TARGET_AUTH=y
> ld: drivers/nvme/host/tcp.o: in function `nvme_tcp_setup_ctrl':
> tcp.c:(.text+0x1099): undefined reference to `nvme_tls_psk_default'
> 
> and
> 
> https://pastebin.com/jXQ71vn9
> CONFIG_NVME_KEYRING=m
> CONFIG_NVME_AUTH=m
> CONFIG_NVME_CORE=m
> CONFIG_NVME_MULTIPATH=y
> CONFIG_NVME_VERBOSE_ERRORS=y
> CONFIG_NVME_FABRICS=m
> CONFIG_NVME_FC=m
> CONFIG_NVME_TCP=m
> CONFIG_NVME_TCP_TLS=y
> CONFIG_NVME_HOST_AUTH=y
> CONFIG_NVME_TARGET=y
> # CONFIG_NVME_TARGET_LOOP is not set
> CONFIG_NVME_TARGET_FC=m
> CONFIG_NVME_TARGET_FCLOOP=m
> CONFIG_NVME_TARGET_TCP=y
> # CONFIG_NVME_TARGET_TCP_TLS is not set
> # CONFIG_NVME_TARGET_AUTH is not set
> ld: drivers/nvme/target/configfs.o: in function `nvmet_ports_make':
> configfs.c:(.text+0x13b8): undefined reference to `nvme_keyring_id'
> 
> The problem is the same in each case: one of the two callers
> (target or host) is set to =m and enables keyring support, while
> the other one is built-in but doesn't use the keyring. This
> leads to the keyring code being built as a loadable module, but
> include/linux/nvme-keyring.h contains broken stub functions
> that lead to it still getting referenced from the built-in
> code.
> 
Ah. RTFM.

'IS_REACHABLE' is the keyword here; if we use that in nvme-keyring.h
things are resolved.

Will be sending an updated patch.

Cheers.

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Arnd Bergmann]

On Wed, Oct 25, 2023, at 11:11, Hannes Reinecke wrote:
> On 10/25/23 10:20, Arnd Bergmann wrote:
>> On Wed, Oct 25, 2023, at 10:12, Hannes Reinecke 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 moving nvme_auth_init()/nvme_auth_exit() into
>>> module init/exit functions for the keyring module.
>>>
>>> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> 
>> Your patch looks good to me, and I think this fixes a
>> separate problem with missing the initialization of the
>> keyring when the kernel has only the target driver but
>> no host support, but it has nothing to do with the changelog
>> I wrote above and does not fix the build regression
>> I described.
>> 
> Hmm. Can you send me the .config file (or the config options for NVMe 
> where there failure occurred)? I tried the configurations mentioned, and 
> it worked for me.
> But apparently I didn't get the right combination...

Sorry if my explanations were not clear enough. The two
broken cases can be triggered using:

https://pastebin.com/aVQ1UbTv
CONFIG_NVME_KEYRING=m
CONFIG_NVME_AUTH=m
CONFIG_NVME_CORE=y
CONFIG_NVME_MULTIPATH=y
CONFIG_NVME_VERBOSE_ERRORS=y
CONFIG_NVME_FABRICS=y
CONFIG_NVME_FC=y
CONFIG_NVME_TCP=y
# CONFIG_NVME_TCP_TLS is not set
# CONFIG_NVME_HOST_AUTH is not set
CONFIG_NVME_TARGET=m
# CONFIG_NVME_TARGET_PASSTHRU is not set
# CONFIG_NVME_TARGET_LOOP is not set
CONFIG_NVME_TARGET_FC=m
CONFIG_NVME_TARGET_FCLOOP=m
CONFIG_NVME_TARGET_TCP=m
CONFIG_NVME_TARGET_TCP_TLS=y
CONFIG_NVME_TARGET_AUTH=y
ld: drivers/nvme/host/tcp.o: in function `nvme_tcp_setup_ctrl':
tcp.c:(.text+0x1099): undefined reference to `nvme_tls_psk_default'

and

https://pastebin.com/jXQ71vn9
CONFIG_NVME_KEYRING=m
CONFIG_NVME_AUTH=m
CONFIG_NVME_CORE=m
CONFIG_NVME_MULTIPATH=y
CONFIG_NVME_VERBOSE_ERRORS=y
CONFIG_NVME_FABRICS=m
CONFIG_NVME_FC=m
CONFIG_NVME_TCP=m
CONFIG_NVME_TCP_TLS=y
CONFIG_NVME_HOST_AUTH=y
CONFIG_NVME_TARGET=y
# CONFIG_NVME_TARGET_LOOP is not set
CONFIG_NVME_TARGET_FC=m
CONFIG_NVME_TARGET_FCLOOP=m
CONFIG_NVME_TARGET_TCP=y
# CONFIG_NVME_TARGET_TCP_TLS is not set
# CONFIG_NVME_TARGET_AUTH is not set
ld: drivers/nvme/target/configfs.o: in function `nvmet_ports_make':
configfs.c:(.text+0x13b8): undefined reference to `nvme_keyring_id'

The problem is the same in each case: one of the two callers
(target or host) is set to =m and enables keyring support, while
the other one is built-in but doesn't use the keyring. This
leads to the keyring code being built as a loadable module, but
include/linux/nvme-keyring.h contains broken stub functions
that lead to it still getting referenced from the built-in
code.

     Arnd

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

[Hannes Reinecke]

On 10/25/23 10:20, Arnd Bergmann wrote:
> On Wed, Oct 25, 2023, at 10:12, Hannes Reinecke 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 moving nvme_auth_init()/nvme_auth_exit() into
>> module init/exit functions for the keyring module.
>>
>> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> Your patch looks good to me, and I think this fixes a
> separate problem with missing the initialization of the
> keyring when the kernel has only the target driver but
> no host support, but it has nothing to do with the changelog
> I wrote above and does not fix the build regression
> I described.
> 
Hmm. Can you send me the .config file (or the config options for NVMe 
where there failure occurred)? I tried the configurations mentioned, and 
it worked for me.
But apparently I didn't get the right combination...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Arnd Bergmann]

On Wed, Oct 25, 2023, at 10:12, Hannes Reinecke 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 moving nvme_auth_init()/nvme_auth_exit() into
> module init/exit functions for the keyring module.
>
> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Your patch looks good to me, and I think this fixes a
separate problem with missing the initialization of the
keyring when the kernel has only the target driver but
no host support, but it has nothing to do with the changelog
I wrote above and does not fix the build regression
I described.

     Arnd

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

[Hannes Reinecke]

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 moving nvme_auth_init()/nvme_auth_exit() into
module init/exit functions for the keyring module.

Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/keyring.c | 9 +++++----
 drivers/nvme/host/core.c      | 9 +--------
 include/linux/nvme-keyring.h  | 8 --------
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 46d7a537dbc2..ee341b83eeba 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -151,7 +151,7 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 }
 EXPORT_SYMBOL_GPL(nvme_tls_psk_default);
 
-int nvme_keyring_init(void)
+static int __init nvme_keyring_init(void)
 {
 	int err;
 
@@ -171,14 +171,15 @@ int nvme_keyring_init(void)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_init);
 
-void nvme_keyring_exit(void)
+static void __exit nvme_keyring_exit(void)
 {
 	unregister_key_type(&nvme_tls_psk_key_type);
 	key_revoke(nvme_keyring);
 	key_put(nvme_keyring);
 }
-EXPORT_SYMBOL_GPL(nvme_keyring_exit);
 
 MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Hannes Reinecke <hare@suse.de>");
+module_init(nvme_keyring_init);
+module_exit(nvme_keyring_exit);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f48b4f735d2d..47645a219128 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -25,7 +25,6 @@
 #include "nvme.h"
 #include "fabrics.h"
 #include <linux/nvme-auth.h>
-#include <linux/nvme-keyring.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -4724,16 +4723,11 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_ns_chr_class);
 		goto unregister_generic_ns;
 	}
-	result = nvme_keyring_init();
-	if (result)
-		goto destroy_ns_chr;
 	result = nvme_init_auth();
 	if (result)
-		goto keyring_exit;
+		goto destroy_ns_chr;
 	return 0;
 
-keyring_exit:
-	nvme_keyring_exit();
 destroy_ns_chr:
 	class_destroy(nvme_ns_chr_class);
 unregister_generic_ns:
@@ -4757,7 +4751,6 @@ static int __init nvme_core_init(void)
 static void __exit nvme_core_exit(void)
 {
 	nvme_exit_auth();
-	nvme_keyring_exit();
 	class_destroy(nvme_ns_chr_class);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index 6cc0696625f3..e10333d78dbb 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -12,8 +12,6 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn);
 
 key_serial_t nvme_keyring_id(void);
-int nvme_keyring_init(void);
-void nvme_keyring_exit(void);
 
 #else
 
@@ -26,11 +24,5 @@ static inline key_serial_t nvme_keyring_id(void)
 {
 	return 0;
 }
-static inline int nvme_keyring_init(void)
-{
-	return 0;
-}
-static inline void nvme_keyring_exit(void) {}
-
 #endif /* !CONFIG_NVME_KEYRING */
 #endif /* _NVME_KEYRING_H */
-- 
2.35.3

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

[Arnd Bergmann]

On Fri, Oct 20, 2023, at 15:50, Hannes Reinecke wrote:
> On 10/20/23 15:05, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>   
>>   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

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

[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

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

[Arnd Bergmann]

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);
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