Re: [PATCH v13 2/3] module: Extend module_blacklist parameter to built-in modules

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] init: klp-diff failure on __modname_<fn> with LTO and PREL32 relocations
--

commit 4ae71df0c31c4118e7804e6488565c6666998f3a
Author: Aaron Tomlin <atomlin@atomlin.com>

module: Extend module_blacklist parameter to built-in modules

This patch extends the module_blacklist= command-line parameter to built-in
modules by recording each module_init() function pointer and its
KBUILD_MODNAME in a new .initcall.modnames section. During boot,
do_one_initcall_builtin() checks this table and skips initcalls for
blacklisted built-in modules.

> diff --git a/include/linux/init.h b/include/linux/init.h
> index 6326c61e2332d..08cb8a2943758 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
[ ... ]
> @@ -252,6 +257,7 @@ extern struct module __this_module;
>  #endif
>  
>  #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> +#define __initcall_fn_ptr(fn, __iid, id)	__initcall_stub(fn, __iid, id)
>  #define ____define_initcall(fn, __stub, __name, __sec)		\
>  	__define_initcall_stub(__stub, fn)			\
>  	asm(".section	\"" __sec "\", \"a\"		\n"	\
> @@ -260,6 +266,7 @@ extern struct module __this_module;
>  	    ".previous					\n");	\
>  	static_assert(__same_type(initcall_t, &fn));
>  #else
> +#define __initcall_fn_ptr(fn, __iid, id)	fn
>  #define ____define_initcall(fn, __unused, __name, __sec)	\
>  	static initcall_t __name __used 			\
>  		__attribute__((__section__(__sec))) = fn;
> @@ -271,7 +278,23 @@ extern struct module __this_module;
>  		__initcall_name(initcall, __iid, id),		\
>  		__initcall_section(__sec, __iid))
>  
> -#define ___define_initcall(fn, id, __sec)			\
> +#define ____define_initcall_modname(fn, id, __sec, __iid)		\
> +	__unique_initcall(fn, id, __sec, __iid)				\
> +	static const char __initstr_##fn[] __used __aligned(1)		\
> +		__section(".init.rodata") = KBUILD_MODNAME;		\
> +	static const struct initcall_modname __modname_##fn __used	\
> +		__section(".initcall.modnames")				\
> +		__aligned(__alignof__(struct initcall_modname)) = {	\
> +			.initcall_fn = __initcall_fn_ptr(fn, __iid, id),\
> +			.modname = __initstr_##fn			\
> +		};

[Severity: Medium]
Can this break livepatch builds with objtool klp diff when
CONFIG_HAVE_ARCH_PREL32_RELOCATIONS=y, CONFIG_LTO_CLANG=y, and
CONFIG_KLP_BUILD=y are enabled?

Under those options, ____define_initcall_modname() initializes
__modname_<fn>.initcall_fn to __initcall_fn_ptr(fn, __iid, id), which expands
to __initcall_stub(fn, __iid, id). Because __iid is generated by
__initcall_id(fn), the stub symbol name embeds both __COUNTER__ and __LINE__:

include/linux/init.h:
    #define __initcall_id(fn)                                   \
            __PASTE(kmod_,                                      \
            __PASTE(__KBUILD_MODNAME,                           \
            __PASTE(__,                                         \
            __PASTE(__COUNTER__,                                \
            __PASTE(_,                                          \
            __PASTE(__LINE__,                                   \
            __PASTE(_, fn)))))))

During objtool klp checksum, calculate_checksums() -> checksum_update_object()
hashes target->demangled_name for each relocation on the __modname_<fn>
object:

tools/objtool/klp-checksum.c:checksum_update_object() {
    ...
                __checksum_update_object(sym, sym_offset, "reloc name",
                                         target->demangled_name,
                                         strlen(target->demangled_name));
    ...
}

Because demangle_name() in tools/objtool/elf.c does not strip the embedded
__COUNTER__ and __LINE__ tokens from __initstub__kmod_*, any patch to a
translation unit containing a built-in module_init() that shifts __COUNTER__
(for example by adding or removing min(), max(), guard(), WARN_ON(), or
__UNIQUE_ID()) or __LINE__ before module_init() changes the stub name and
alters the checksum of __modname_<fn>.

In tools/objtool/klp-diff.c, dont_correlate() -> is_initcall_sym() skips
__initcall__ and __initstub__ symbols, but does not skip __modname_:

tools/objtool/klp-diff.c:is_initcall_sym() {
        return strstarts(sym->name, "__initcall__") ||
               strstarts(sym->name, "__initstub__");
}

When correlate_symbols() pairs __modname_<fn> between the original and
patched objects, mark_changed_functions() sees the differing checksum on the
non-function symbol and aborts the build:

tools/objtool/klp-diff.c:mark_changed_functions() {
    ...
                if (orig_sym->csum.checksum != patched_sym->csum.checksum) {
                        if (!is_func_sym(orig_sym)) {
                                ERROR("changed data: %s", orig_sym->name);
                                return -1;
                        }
    ...
}

Should is_initcall_sym() in tools/objtool/klp-diff.c also exclude
__modname_ symbols (or should __modname_<fn> avoid referencing the
__COUNTER__- and __LINE__-dependent stub name)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104154003.43313-1-atomlin@atomlin.com?part=2