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 <[email protected]> > > --- > > 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 > >
