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: <20250711081047-ea2c1e83-1b87-4331-acad-cbbfe6be67d8@linutronix.de>
Date: Fri, 11 Jul 2025 08:29:06 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Daniel Gomez <da.gomez@...nel.org>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Petr Pavlu <petr.pavlu@...e.com>, 
	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 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@...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.

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