[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 */
--