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 <[email protected]> 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/[email protected]?part=2
