[PATCH 0/3] module: make structure definitions always visible

[PATCH 0/3] module: make structure definitions always visible

[Thomas Weißschuh]

Code using IS_ENABLED(CONFIG_MODULES) as a C expression may need access
to the module structure definitions to compile.
Make sure these structure definitions are always visible.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Thomas Weißschuh (3):
      module: move 'struct module_use' to internal.h
      module: make structure definitions always visible
      kunit: test: Drop CONFIG_MODULE ifdeffery

 include/linux/module.h   | 30 ++++++++++++------------------
 kernel/module/internal.h |  7 +++++++
 lib/kunit/test.c         |  8 --------
 3 files changed, 19 insertions(+), 26 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250611-kunit-ifdef-modules-0fefd13ae153

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

[PATCH 1/3] module: move 'struct module_use' to internal.h

[Thomas Weißschuh]

The struct was moved to the public header file in
commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use.").
Back then the structure was used outside of the module core.
Nowadays this is not true anymore, so the structure can be made internal.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 include/linux/module.h   | 7 -------
 kernel/module/internal.h | 7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 92e1420fccdffc9de9f49da9061546cc1e0c89d1..52f7b0487a2733c56e2531a434887e56e1bf45b2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -313,13 +313,6 @@ void *__symbol_get_gpl(const char *symbol);
 		__used __section(".no_trim_symbol") = __stringify(x); \
 	(typeof(&x))(__symbol_get(__stringify(x))); })
 
-/* modules using other modules: kdb wants to see this. */
-struct module_use {
-	struct list_head source_list;
-	struct list_head target_list;
-	struct module *source, *target;
-};
-
 enum module_state {
 	MODULE_STATE_LIVE,	/* Normal state. */
 	MODULE_STATE_COMING,	/* Full formed, running module_init. */
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 8d74b0a21c82b5360977a29736eca78ee6b6be3e..1c2e0b0dc52e72d5ecd2f1b310ce535364b3f33b 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -109,6 +109,13 @@ struct find_symbol_arg {
 	enum mod_license license;
 };
 
+/* modules using other modules */
+struct module_use {
+	struct list_head source_list;
+	struct list_head target_list;
+	struct module *source, *target;
+};
+
 int mod_verify_sig(const void *mod, struct load_info *info);
 int try_to_force_load(struct module *mod, const char *reason);
 bool find_symbol(struct find_symbol_arg *fsa);

-- 
2.49.0

Re: [PATCH 1/3] module: move 'struct module_use' to internal.h

[Petr Pavlu]

On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> The struct was moved to the public header file in
> commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use.").
> Back then the structure was used outside of the module core.
> Nowadays this is not true anymore, so the structure can be made internal.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

Re: [PATCH 1/3] module: move 'struct module_use' to internal.h

[Daniel Gomez]

On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> The struct was moved to the public header file in
> commit c8e21ced08b3 ("module: fix kdb's illicit use of struct module_use.").
> Back then the structure was used outside of the module core.
> Nowadays this is not true anymore, so the structure can be made internal.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

[PATCH 2/3] module: make structure definitions always visible

[Thomas Weißschuh]

To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
The code will still fully typechecked but the unreachable parts are
discarded by the compiler. This prevents accidental breakage when a certain
kconfig combination was not specifically tested by the developer.
This pattern is already supported to some extend by module.h defining
empty stub functions if CONFIG_MODULES=n.
However some users of module.h work on the structured defined by module.h.

Therefore these structure definitions need to be visible, too.

Many structure members are still gated by specific configuration settings.
The assumption for those is that the code using them will be gated behind
the same configuration setting anyways.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 include/linux/module.h | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name		\
 
 struct notifier_block;
 
-#ifdef CONFIG_MODULES
-
-extern int modules_disabled; /* for sysctl */
-/* Get/put a kernel symbol (calls must be symmetric) */
-void *__symbol_get(const char *symbol);
-void *__symbol_get_gpl(const char *symbol);
-#define symbol_get(x)	({ \
-	static const char __notrim[] \
-		__used __section(".no_trim_symbol") = __stringify(x); \
-	(typeof(&x))(__symbol_get(__stringify(x))); })
-
 enum module_state {
 	MODULE_STATE_LIVE,	/* Normal state. */
 	MODULE_STATE_COMING,	/* Full formed, running module_init. */
@@ -598,6 +587,18 @@ struct module {
 	struct _ddebug_info dyndbg_info;
 #endif
 } ____cacheline_aligned __randomize_layout;
+
+#ifdef CONFIG_MODULES
+
+extern int modules_disabled; /* for sysctl */
+/* Get/put a kernel symbol (calls must be symmetric) */
+void *__symbol_get(const char *symbol);
+void *__symbol_get_gpl(const char *symbol);
+#define symbol_get(x)	({ \
+	static const char __notrim[] \
+		__used __section(".no_trim_symbol") = __stringify(x); \
+	(typeof(&x))(__symbol_get(__stringify(x))); })
+
 #ifndef MODULE_ARCH_INIT
 #define MODULE_ARCH_INIT {}
 #endif

-- 
2.49.0

Re: [PATCH 2/3] module: make structure definitions always visible

[Daniel Gomez]

On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
> The code will still fully typechecked but the unreachable parts are
> discarded by the compiler. This prevents accidental breakage when a certain
> kconfig combination was not specifically tested by the developer.
> This pattern is already supported to some extend by module.h defining
> empty stub functions if CONFIG_MODULES=n.
> However some users of module.h work on the structured defined by module.h.
> 
> Therefore these structure definitions need to be visible, too.

We are missing here which structures are needed. + we are making more things
visible than what we actually need.

> 
> Many structure members are still gated by specific configuration settings.
> The assumption for those is that the code using them will be gated behind
> the same configuration setting anyways.

I think code and kconfig need to reflect the actual dependencies. For example,
if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
Kconfig with depends on, as well as keep the code gated by these 2 configs with
ifdef/IS_ENABLED.

> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  include/linux/module.h | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name		\
>  
>  struct notifier_block;
>  
> -#ifdef CONFIG_MODULES
> -
> -extern int modules_disabled; /* for sysctl */
> -/* Get/put a kernel symbol (calls must be symmetric) */
> -void *__symbol_get(const char *symbol);
> -void *__symbol_get_gpl(const char *symbol);
> -#define symbol_get(x)	({ \
> -	static const char __notrim[] \
> -		__used __section(".no_trim_symbol") = __stringify(x); \
> -	(typeof(&x))(__symbol_get(__stringify(x))); })
> -
>  enum module_state {
>  	MODULE_STATE_LIVE,	/* Normal state. */
>  	MODULE_STATE_COMING,	/* Full formed, running module_init. */
> @@ -598,6 +587,18 @@ struct module {
>  	struct _ddebug_info dyndbg_info;
>  #endif
>  } ____cacheline_aligned __randomize_layout;
> +
> +#ifdef CONFIG_MODULES
> +
> +extern int modules_disabled; /* for sysctl */
> +/* Get/put a kernel symbol (calls must be symmetric) */
> +void *__symbol_get(const char *symbol);
> +void *__symbol_get_gpl(const char *symbol);
> +#define symbol_get(x)	({ \
> +	static const char __notrim[] \
> +		__used __section(".no_trim_symbol") = __stringify(x); \
> +	(typeof(&x))(__symbol_get(__stringify(x))); })
> +

The patch exposes data structures that are not needed. + breaks the
config dependencies.

For example, before this patch:

#ifdef CONFIG_MODULES

{...}

struct mod_tree_node {

{...}

struct module_memory {
	void *base;
	bool is_rox;
	unsigned int size;

#ifdef CONFIG_MODULES_TREE_LOOKUP
	struct mod_tree_node mtn;
#endif
};

{...}
#endif /* CONFIG_MODULES */

After the patch, mod_tree_node is not needed externally. And the mtn field
in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
+ MODULES_TREE_LOOKUP.

I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros,
and LOOKUP.

>  #define MODULE_ARCH_INIT {}
>  #endif
> 

Re: [PATCH 2/3] module: make structure definitions always visible

[Petr Pavlu]

On 7/7/25 9:11 PM, Daniel Gomez wrote:
> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
>> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
>> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
>> The code will still fully typechecked but the unreachable parts are
>> discarded by the compiler. This prevents accidental breakage when a certain
>> kconfig combination was not specifically tested by the developer.
>> This pattern is already supported to some extend by module.h defining
>> empty stub functions if CONFIG_MODULES=n.
>> However some users of module.h work on the structured defined by module.h.
>>
>> Therefore these structure definitions need to be visible, too.
> 
> We are missing here which structures are needed. + we are making more things
> visible than what we actually need.
> 
>>
>> Many structure members are still gated by specific configuration settings.
>> The assumption for those is that the code using them will be gated behind
>> the same configuration setting anyways.
> 
> I think code and kconfig need to reflect the actual dependencies. For example,
> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
> Kconfig with depends on, as well as keep the code gated by these 2 configs with
> ifdef/IS_ENABLED.
> 
>>
>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>>  include/linux/module.h | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name		\
>>  
>>  struct notifier_block;
>>  
>> -#ifdef CONFIG_MODULES
>> -
>> -extern int modules_disabled; /* for sysctl */
>> -/* Get/put a kernel symbol (calls must be symmetric) */
>> -void *__symbol_get(const char *symbol);
>> -void *__symbol_get_gpl(const char *symbol);
>> -#define symbol_get(x)	({ \
>> -	static const char __notrim[] \
>> -		__used __section(".no_trim_symbol") = __stringify(x); \
>> -	(typeof(&x))(__symbol_get(__stringify(x))); })
>> -
>>  enum module_state {
>>  	MODULE_STATE_LIVE,	/* Normal state. */
>>  	MODULE_STATE_COMING,	/* Full formed, running module_init. */
>> @@ -598,6 +587,18 @@ struct module {
>>  	struct _ddebug_info dyndbg_info;
>>  #endif
>>  } ____cacheline_aligned __randomize_layout;
>> +
>> +#ifdef CONFIG_MODULES
>> +
>> +extern int modules_disabled; /* for sysctl */
>> +/* Get/put a kernel symbol (calls must be symmetric) */
>> +void *__symbol_get(const char *symbol);
>> +void *__symbol_get_gpl(const char *symbol);
>> +#define symbol_get(x)	({ \
>> +	static const char __notrim[] \
>> +		__used __section(".no_trim_symbol") = __stringify(x); \
>> +	(typeof(&x))(__symbol_get(__stringify(x))); })
>> +
> 
> The patch exposes data structures that are not needed. + breaks the
> config dependencies.
> 
> For example, before this patch:
> 
> #ifdef CONFIG_MODULES
> 
> {...}
> 
> struct mod_tree_node {
> 
> {...}
> 
> struct module_memory {
> 	void *base;
> 	bool is_rox;
> 	unsigned int size;
> 
> #ifdef CONFIG_MODULES_TREE_LOOKUP
> 	struct mod_tree_node mtn;
> #endif
> };
> 
> {...}
> #endif /* CONFIG_MODULES */
> 
> After the patch, mod_tree_node is not needed externally. And the mtn field
> in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
> + MODULES_TREE_LOOKUP.
> 
> I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros,
> and LOOKUP.

I think the idea is that having unnecessary structures in header files
isn't particularly harmful, as they won't affect the resulting binary.
On the other hand, they can help with type checking of conditional code
as shown by patch #3.

This is different compared to "extern int modules_disabled;" and
"void *__symbol_get(const char *symbol);" which the patch correctly
still protects by '#ifdef CONFIG_MODULES'. Not hiding them could result
in successful compilation but an error only at link time.

-- 
Thanks,
Petr

Re: [PATCH 2/3] module: make structure definitions always visible

[Thomas Weißschuh]

On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> > To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
> > it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
> > The code will still fully typechecked but the unreachable parts are
> > discarded by the compiler. This prevents accidental breakage when a certain
> > kconfig combination was not specifically tested by the developer.
> > This pattern is already supported to some extend by module.h defining
> > empty stub functions if CONFIG_MODULES=n.
> > However some users of module.h work on the structured defined by module.h.
> > 
> > Therefore these structure definitions need to be visible, too.
> 
> We are missing here which structures are needed. + we are making more things
> visible than what we actually need.
> 
> > 
> > Many structure members are still gated by specific configuration settings.
> > The assumption for those is that the code using them will be gated behind
> > the same configuration setting anyways.
> 
> I think code and kconfig need to reflect the actual dependencies. For example,
> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
> Kconfig with depends on, as well as keep the code gated by these 2 configs with
> ifdef/IS_ENABLED.

If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then
IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically.
There is no need for another explicit IS_ENABLED(CONFIG_MODULES).

> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> >  include/linux/module.h | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name		\
> >  
> >  struct notifier_block;
> >  
> > -#ifdef CONFIG_MODULES
> > -
> > -extern int modules_disabled; /* for sysctl */
> > -/* Get/put a kernel symbol (calls must be symmetric) */
> > -void *__symbol_get(const char *symbol);
> > -void *__symbol_get_gpl(const char *symbol);
> > -#define symbol_get(x)	({ \
> > -	static const char __notrim[] \
> > -		__used __section(".no_trim_symbol") = __stringify(x); \
> > -	(typeof(&x))(__symbol_get(__stringify(x))); })
> > -
> >  enum module_state {
> >  	MODULE_STATE_LIVE,	/* Normal state. */
> >  	MODULE_STATE_COMING,	/* Full formed, running module_init. */
> > @@ -598,6 +587,18 @@ struct module {
> >  	struct _ddebug_info dyndbg_info;
> >  #endif
> >  } ____cacheline_aligned __randomize_layout;
> > +
> > +#ifdef CONFIG_MODULES
> > +
> > +extern int modules_disabled; /* for sysctl */
> > +/* Get/put a kernel symbol (calls must be symmetric) */
> > +void *__symbol_get(const char *symbol);
> > +void *__symbol_get_gpl(const char *symbol);
> > +#define symbol_get(x)	({ \
> > +	static const char __notrim[] \
> > +		__used __section(".no_trim_symbol") = __stringify(x); \
> > +	(typeof(&x))(__symbol_get(__stringify(x))); })
> > +
> 
> The patch exposes data structures that are not needed. + breaks the
> config dependencies.

If we want to expose 'struct module' to !CONFIG_MODULES code, all it's
effective member types also need to be included.
With my patch these member types are actually still implictly gated behind
CONFIG_MODULES as they depend on it through kconfig.

> 
> For example, before this patch:
> 
> #ifdef CONFIG_MODULES
> 
> {...}
> 
> struct mod_tree_node {
> 
> {...}
> 
> struct module_memory {
> 	void *base;
> 	bool is_rox;
> 	unsigned int size;
> 
> #ifdef CONFIG_MODULES_TREE_LOOKUP
> 	struct mod_tree_node mtn;
> #endif
> };
> 
> {...}
> #endif /* CONFIG_MODULES */
> 
> After the patch, mod_tree_node is not needed externally.

Can you explain what you mean with "not needed externally"?
'struct mod_tree_node' is only ever used by core module code.
It is only public because it is embedded in the public 'struct module'

> And the mtn field
> in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
> + MODULES_TREE_LOOKUP.

As mentioned above, MODULES_TREE_LOOKUP && !MODULES can never happen.

> I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros,
> and LOOKUP.
> 
> >  #define MODULE_ARCH_INIT {}
> >  #endif
> > 

Re: [PATCH 2/3] module: make structure definitions always visible

[Daniel Gomez]

On 11/07/2025 08.29, Thomas WeiÃschuh wrote:
> On Mon, Jul 07, 2025 at 09:11:05PM +0200, Daniel Gomez wrote:
>> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
>>> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
>>> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
>>> The code will still fully typechecked but the unreachable parts are
>>> discarded by the compiler. This prevents accidental breakage when a certain
>>> kconfig combination was not specifically tested by the developer.
>>> This pattern is already supported to some extend by module.h defining
>>> empty stub functions if CONFIG_MODULES=n.
>>> However some users of module.h work on the structured defined by module.h.
>>>
>>> Therefore these structure definitions need to be visible, too.
>>
>> We are missing here which structures are needed. + we are making more things
>> visible than what we actually need.
>>
>>>
>>> Many structure members are still gated by specific configuration settings.
>>> The assumption for those is that the code using them will be gated behind
>>> the same configuration setting anyways.
>>
>> I think code and kconfig need to reflect the actual dependencies. For example,
>> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
>> Kconfig with depends on, as well as keep the code gated by these 2 configs with
>> ifdef/IS_ENABLED.
> 
> If CONFIG_LIVEPATCH depends on CONFIG_MODULES in kconfig then
> IS_ENABLED(CONFIG_LIVEPATCH) will depend on CONFIG_MODULES automatically.
> There is no need for another explicit IS_ENABLED(CONFIG_MODULES).

This makes sense to me. My assessment before to reflect in code what we have in
kconfig does not scale. Thanks.

> 
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>> ---
>>>  include/linux/module.h | 23 ++++++++++++-----------
>>>  1 file changed, 12 insertions(+), 11 deletions(-)

{...}

>>
>> After the patch, mod_tree_node is not needed externally.
> 
> Can you explain what you mean with "not needed externally"?
> 'struct mod_tree_node' is only ever used by core module code.
> It is only public because it is embedded in the public 'struct module'

But only when MODULES_TREE_LOOKUP is enabled. Now, all kernels (regardless of
that config) will define mod_tree_node data structure.

However, Petr already stated that is harmless to do so. I was trying here to
not be useless.

With that, changes look good to me:

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

Re: [PATCH 2/3] module: make structure definitions always visible

[Petr Pavlu]

On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
> The code will still fully typechecked but the unreachable parts are
> discarded by the compiler. This prevents accidental breakage when a certain
> kconfig combination was not specifically tested by the developer.
> This pattern is already supported to some extend by module.h defining
> empty stub functions if CONFIG_MODULES=n.
> However some users of module.h work on the structured defined by module.h.
> 
> Therefore these structure definitions need to be visible, too.
> 
> Many structure members are still gated by specific configuration settings.
> The assumption for those is that the code using them will be gated behind
> the same configuration setting anyways.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  include/linux/module.h | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name		\
>  
>  struct notifier_block;
>  
> -#ifdef CONFIG_MODULES
> -
> -extern int modules_disabled; /* for sysctl */
> -/* Get/put a kernel symbol (calls must be symmetric) */
> -void *__symbol_get(const char *symbol);
> -void *__symbol_get_gpl(const char *symbol);
> -#define symbol_get(x)	({ \
> -	static const char __notrim[] \
> -		__used __section(".no_trim_symbol") = __stringify(x); \
> -	(typeof(&x))(__symbol_get(__stringify(x))); })
> -
>  enum module_state {
>  	MODULE_STATE_LIVE,	/* Normal state. */
>  	MODULE_STATE_COMING,	/* Full formed, running module_init. */
> @@ -598,6 +587,18 @@ struct module {
>  	struct _ddebug_info dyndbg_info;
>  #endif
>  } ____cacheline_aligned __randomize_layout;
> +
> +#ifdef CONFIG_MODULES
> +
> +extern int modules_disabled; /* for sysctl */
> +/* Get/put a kernel symbol (calls must be symmetric) */
> +void *__symbol_get(const char *symbol);
> +void *__symbol_get_gpl(const char *symbol);
> +#define symbol_get(x)	({ \
> +	static const char __notrim[] \
> +		__used __section(".no_trim_symbol") = __stringify(x); \
> +	(typeof(&x))(__symbol_get(__stringify(x))); })
> +
>  #ifndef MODULE_ARCH_INIT
>  #define MODULE_ARCH_INIT {}
>  #endif
> 

Nit: I suggest keeping MODULE_ARCH_INIT in its current position,
immediately after the 'struct module' declaration, because the macro is
directly tied to that structure.

-- 
Thanks,
Petr

[PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Thomas Weißschuh]

The function stubs exposed by module.h allow the code to compile properly
without the ifdeffery. The generated object code stays the same, as the
compiler can optimize away all the dead code.
As the code is still typechecked developer errors can be detected faster.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 lib/kunit/test.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 146d1b48a0965e8aaddb6162928f408bbb542645..019b2ac9c8469021542b610278f8842e100d57ad 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -759,7 +759,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
 }
 EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
 
-#ifdef CONFIG_MODULES
 static void kunit_module_init(struct module *mod)
 {
 	struct kunit_suite_set suite_set, filtered_set;
@@ -847,7 +846,6 @@ static struct notifier_block kunit_mod_nb = {
 	.notifier_call = kunit_module_notify,
 	.priority = 0,
 };
-#endif
 
 KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
 
@@ -938,20 +936,14 @@ static int __init kunit_init(void)
 	kunit_debugfs_init();
 
 	kunit_bus_init();
-#ifdef CONFIG_MODULES
 	return register_module_notifier(&kunit_mod_nb);
-#else
-	return 0;
-#endif
 }
 late_initcall(kunit_init);
 
 static void __exit kunit_exit(void)
 {
 	memset(&kunit_hooks, 0, sizeof(kunit_hooks));
-#ifdef CONFIG_MODULES
 	unregister_module_notifier(&kunit_mod_nb);
-#endif
 
 	kunit_bus_shutdown();
 

-- 
2.49.0

Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Petr Pavlu]

On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> The function stubs exposed by module.h allow the code to compile properly
> without the ifdeffery. The generated object code stays the same, as the
> compiler can optimize away all the dead code.
> As the code is still typechecked developer errors can be detected faster.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

I'm worried that patches #2 and #3 make the code harder to understand
because they hide what is compiled and when.

Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
surrounds functionality that should be conditional. This makes it clear
what is used and when.

The patches obscure whether, for instance, kunit_module_notify() in
lib/kunit/test.c is actually used and present in the resulting binary
behind several steps. Understanding its usage requires tracing the code
from kunit_module_notify() to the definition of kunit_mod_nb, then to
the register_module_notifier() call, and finally depends on an ifdef in
another file, include/linux/module.h.

Is this really better? Are there places where this pattern is already
used? Does it actually help in practice, considering that CONFIG_MODULES
is enabled in most cases?

-- 
Thanks,
Petr

Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Thomas Weißschuh]

On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
> On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> > The function stubs exposed by module.h allow the code to compile properly
> > without the ifdeffery. The generated object code stays the same, as the
> > compiler can optimize away all the dead code.
> > As the code is still typechecked developer errors can be detected faster.
> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> 
> I'm worried that patches #2 and #3 make the code harder to understand
> because they hide what is compiled and when.
> 
> Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
> surrounds functionality that should be conditional. This makes it clear
> what is used and when.

#ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work
(here) without patch #2.

> The patches obscure whether, for instance, kunit_module_notify() in
> lib/kunit/test.c is actually used and present in the resulting binary
> behind several steps. Understanding its usage requires tracing the code
> from kunit_module_notify() to the definition of kunit_mod_nb, then to
> the register_module_notifier() call, and finally depends on an ifdef in
> another file, include/linux/module.h.

I agree that it is not completely clear what will end up in the binary.
On the other hand we can program the happy path and the compiler will take care
of all the corner cases.
We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change
anything but would be clearer to read.

> Is this really better? Are there places where this pattern is already
> used? Does it actually help in practice, considering that CONFIG_MODULES
> is enabled in most cases?

This came up for me when refactoring some KUnit internal code.
I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes.
But some callers of changed functions were not updated and this wasn't reported.

The stub functions are a standard pattern and already implemented by module.h.
I have not found a header which hides structure definitions.

Documentation/process/coding-style.rst:

	21) Conditional Compilation
	---------------------------

	Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
	files; doing so makes code harder to read and logic harder to follow.  Instead,
	use such conditionals in a header file defining functions for use in those .c
	files, providing no-op stub versions in the #else case, and then call those
	functions unconditionally from .c files.  The compiler will avoid generating
	any code for the stub calls, producing identical results, but the logic will
	remain easy to follow.

I should add the documentation reference to patch #2.

Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Petr Pavlu]

On 6/17/25 10:39 AM, Thomas Weißschuh wrote:
> On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
>> On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
>>> The function stubs exposed by module.h allow the code to compile properly
>>> without the ifdeffery. The generated object code stays the same, as the
>>> compiler can optimize away all the dead code.
>>> As the code is still typechecked developer errors can be detected faster.
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>
>> I'm worried that patches #2 and #3 make the code harder to understand
>> because they hide what is compiled and when.
>>
>> Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
>> surrounds functionality that should be conditional. This makes it clear
>> what is used and when.
> 
> #ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work
> (here) without patch #2.
> 
>> The patches obscure whether, for instance, kunit_module_notify() in
>> lib/kunit/test.c is actually used and present in the resulting binary
>> behind several steps. Understanding its usage requires tracing the code
>> from kunit_module_notify() to the definition of kunit_mod_nb, then to
>> the register_module_notifier() call, and finally depends on an ifdef in
>> another file, include/linux/module.h.
> 
> I agree that it is not completely clear what will end up in the binary.
> On the other hand we can program the happy path and the compiler will take care
> of all the corner cases.
> We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change
> anything but would be clearer to read.
> 
>> Is this really better? Are there places where this pattern is already
>> used? Does it actually help in practice, considering that CONFIG_MODULES
>> is enabled in most cases?
> 
> This came up for me when refactoring some KUnit internal code.
> I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes.
> But some callers of changed functions were not updated and this wasn't reported.

I see.

> 
> The stub functions are a standard pattern and already implemented by module.h.

Stub functions are ok, I have no concerns about that pattern.

> I have not found a header which hides structure definitions.

It seems you're right. I think that makes patch #2 acceptable, it is
consistent with other kernel code.

> 
> Documentation/process/coding-style.rst:
> 
> 	21) Conditional Compilation
> 	---------------------------
> 
> 	Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> 	files; doing so makes code harder to read and logic harder to follow.  Instead,
> 	use such conditionals in a header file defining functions for use in those .c
> 	files, providing no-op stub versions in the #else case, and then call those
> 	functions unconditionally from .c files.  The compiler will avoid generating
> 	any code for the stub calls, producing identical results, but the logic will
> 	remain easy to follow.
> 
> I should add the documentation reference to patch #2.

This part discusses stub functions. I feel patch #3 stretches the
intention of the coding style described here. As discussed, the patch
somewhat hides when the functions are actually used, which might not be
desirable, but I'll leave it to the kunit folks to decide what they
prefer.

-- 
Thanks,
Petr

Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

On Thu, 12 Jun 2025 at 22:54, Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> The function stubs exposed by module.h allow the code to compile properly
> without the ifdeffery. The generated object code stays the same, as the
> compiler can optimize away all the dead code.
> As the code is still typechecked developer errors can be detected faster.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---

Acked-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/test.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 146d1b48a0965e8aaddb6162928f408bbb542645..019b2ac9c8469021542b610278f8842e100d57ad 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -759,7 +759,6 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
>  }
>  EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
>
> -#ifdef CONFIG_MODULES
>  static void kunit_module_init(struct module *mod)
>  {
>         struct kunit_suite_set suite_set, filtered_set;
> @@ -847,7 +846,6 @@ static struct notifier_block kunit_mod_nb = {
>         .notifier_call = kunit_module_notify,
>         .priority = 0,
>  };
> -#endif
>
>  KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *)
>
> @@ -938,20 +936,14 @@ static int __init kunit_init(void)
>         kunit_debugfs_init();
>
>         kunit_bus_init();
> -#ifdef CONFIG_MODULES
>         return register_module_notifier(&kunit_mod_nb);
> -#else
> -       return 0;
> -#endif
>  }
>  late_initcall(kunit_init);
>
>  static void __exit kunit_exit(void)
>  {
>         memset(&kunit_hooks, 0, sizeof(kunit_hooks));
> -#ifdef CONFIG_MODULES
>         unregister_module_notifier(&kunit_mod_nb);
> -#endif
>
>         kunit_bus_shutdown();
>
>
> --
> 2.49.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]

Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Daniel Gomez]

On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
> The function stubs exposed by module.h allow the code to compile properly
> without the ifdeffery. The generated object code stays the same, as the
> compiler can optimize away all the dead code.
> As the code is still typechecked developer errors can be detected faster.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>