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