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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9c8b2847-5017-45b8-bbc2-0ba96cc42961@suse.com>
Date: Tue, 8 Jul 2025 11:32:09 +0200
From: Petr Pavlu <petr.pavlu@...e.com>
To: Daniel Gomez <da.gomez@...nel.org>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
 Luis Chamberlain <mcgrof@...nel.org>, Sami Tolvanen
 <samitolvanen@...gle.com>, Daniel Gomez <da.gomez@...sung.com>,
 Brendan Higgins <brendan.higgins@...ux.dev>, David Gow
 <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>,
 linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com
Subject: Re: [PATCH 2/3] module: make structure definitions always visible

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@...utronix.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