| Message ID | zsk54zosv6tht4j4nma4ix7lq2knxi3ylqadd2foznc72nkaa3@xbc7gpozx6ai (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] init: staging: Fix missing warning/taint on builtin code | expand |
Thanks!
Acked-by: Dan Carpenter <dan.carpenter@linaro.org>
regards,
dan carpenter
Hello, On Sat, Jul 06, 2024 at 12:15:01AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote: > Fix the absence of warning message and kernel tainting when initializing > drivers from the `drivers/staging` subtree from initcalls (when > configured as built-in). > > When such a driver is built as module and the module is loaded, the > `load_module()` function taints the kernel to signal code of unknown > quality is loaded, and produces a warning like this: > > [ 8.076352] rts5208: module is from the staging directory, the > quality is unknown, you have been warned. > > The same behaviour is absent, however, when a staging driver is compiled > as built-in on the kernel image, since loading it happens through > initcalls and not through load_module(). > > This might prevent relevant information of being available on a bug > report (i.e. on a panic log) among other possible problems. > > NOTES: > - The patch is written in such a way that all non-staging drivers are > kept the way they were, except for staging drivers built with > `-DSTAGING_CODE`. > - Since it changes some macros related to clang LTO as well, I tested it > and it works properly in kernels compiled with both clang and gcc. > - Some `checkpatch.pl` errors, warnings and checks (with `--strict`) are > present. Some were already there, some I introduced but I think > they're unavoidable. Some IMHO don´t make sense at all, I think they > would apply for most regular macros but initcall macros are just way > different. > > Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code") > Signed-off-by: Ágatha Isabelle Chris Moreira Guedes <code@agatha.dev> I didn't grok the complete patch, the but intention is good. Thanks Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Best regards Uwe
On Sat, Jul 06, 2024 at 12:15:01AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote: > Fix the absence of warning message and kernel tainting when initializing > drivers from the `drivers/staging` subtree from initcalls (when > configured as built-in). > > When such a driver is built as module and the module is loaded, the > `load_module()` function taints the kernel to signal code of unknown > quality is loaded, and produces a warning like this: > > [ 8.076352] rts5208: module is from the staging directory, the > quality is unknown, you have been warned. > > The same behaviour is absent, however, when a staging driver is compiled > as built-in on the kernel image, since loading it happens through > initcalls and not through load_module(). > > This might prevent relevant information of being available on a bug > report (i.e. on a panic log) among other possible problems. > > NOTES: > - The patch is written in such a way that all non-staging drivers are > kept the way they were, except for staging drivers built with > `-DSTAGING_CODE`. That's good! > - Since it changes some macros related to clang LTO as well, I tested it > and it works properly in kernels compiled with both clang and gcc. This is odd, why is it messing with LTO stuff? It should be much more "self contained" than this I feel like. I see what you are doing by trying to use some of the LTO macros again, but in doing so, it makes it really hard to understand the diff and feel comfortable with this. If you want to stick with what you have done here, can you split it up a bit more? Once patch for the LTO header file changes and then another that only adds the staging stuff. That way it's easier to review and justify that nothing is going to be broken with this patch. > - Some `checkpatch.pl` errors, warnings and checks (with `--strict`) are > present. Some were already there, some I introduced but I think > they're unavoidable. Some IMHO don´t make sense at all, I think they > would apply for most regular macros but initcall macros are just way > different. Yeah, checkpatch and macros can get tricky, use your best judgement here and it looks ok. > Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code") I think it really fixes the commit _after_ this one that turns on the taint for the build :) anyway, nice work, I think it's almost there! greg k-h
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile index 5390879b5d1b..7cea13436426 100644 --- a/drivers/staging/Makefile +++ b/drivers/staging/Makefile @@ -1,6 +1,8 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for staging directory +subdir-ccflags-y += -DSTAGING_CODE + obj-y += media/ obj-$(CONFIG_FB_OLPC_DCON) += olpc_dcon/ obj-$(CONFIG_RTL8192E) += rtl8192e/ diff --git a/include/linux/init.h b/include/linux/init.h index 58cef4c2e59a..c7afee978b42 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -188,6 +188,8 @@ extern struct module __this_module; #ifndef __ASSEMBLY__ +extern void staging_taint(const char *code_id, bool module); + /* * initcalls are now grouped by functionality into separate * subsections. Ordering inside the subsections is determined @@ -220,6 +222,14 @@ extern struct module __this_module; __PASTE(__, \ __PASTE(__iid, id)))) +#define __define_stub_fn(___stub, fn, __taintcall) \ + int __init ___stub(void); \ + int __init ___stub(void) \ + { \ + __taintcall(); \ + return fn(); \ + } \ + #ifdef CONFIG_LTO_CLANG /* * With LTO, the compiler doesn't necessarily obey link order for @@ -230,31 +240,75 @@ extern struct module __this_module; #define __initcall_section(__sec, __iid) \ #__sec ".init.." #__iid +#define ___define_initcall_stub(__stub, fn, __taintcall) \ + __define_stub_fn(__stub, fn, __taintcall) \ + __ADDRESSABLE(__stub) +#else +#define __initcall_section(__sec, __iid) \ + #__sec ".init" + +#define ___define_initcall_stub(__stub, fn, __taintcall) \ + __ADDRESSABLE(fn) +#endif /* CONFIG_LTO_CLANG */ + +#ifdef STAGING_CODE + /* * With LTO, the compiler can rename static functions to avoid * global naming collisions. We use a global stub function for * initcalls to create a stable symbol name whose address can be * taken in inline assembly when PREL32 relocations are used. + * + * Moreover, when there's staging code, regardless of LTO, we + * need to wrap the function inside a new one to taint the + * kernel and warn about it in the log. So we need this special + * symbol name for the wrapper regardless of that. */ #define __initcall_stub(fn, __iid, id) \ __initcall_name(initstub, __iid, id) +#define __staging_taint_fn() \ + staging_taint(KBUILD_MODNAME, false) + +#ifdef CONFIG_LTO_CLANG + #define __define_initcall_stub(__stub, fn) \ - int __init __stub(void); \ - int __init __stub(void) \ - { \ - return fn(); \ - } \ - __ADDRESSABLE(__stub) -#else -#define __initcall_section(__sec, __iid) \ - #__sec ".init" + ___define_initcall_stub(__stub, fn, __staging_taint_fn) + +#else /* no CONFIG_LTO_CLANG */ + +#define __define_initcall_stub(_stub, fn) \ + __define_stub_fn(_stub, fn, __staging_taint_fn) \ + __ADDRESSABLE(_stub) + +#endif /* CONFIG_LTO_CLANG */ + +#else /* no STAGING_CODE */ + +#ifdef CONFIG_LTO_CLANG + +/* + * Same case as before, it's not staging code but there's LTO + */ +#define __initcall_stub(fn, __iid, id) \ + __initcall_name(initstub, __iid, id) +#else /* no CONFIG_LTO_CLANG*/ + +/* For no LTO outside staging code, the vast majority of drivers + * can just be built with their regular symbol names as they + * just have been built all the time + */ #define __initcall_stub(fn, __iid, id) fn +#endif /* CONFIG_LTO_CLANG */ + +#define __staging_taint_fn() + #define __define_initcall_stub(__stub, fn) \ - __ADDRESSABLE(fn) -#endif + ___define_initcall_stub(__stub, fn, __staging_taint_fn) + +#endif /* STAGING_CODE */ #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS #define ____define_initcall(fn, __stub, __name, __sec) \ diff --git a/init/main.c b/init/main.c index 206acdde51f5..e35eeec1fbe4 100644 --- a/init/main.c +++ b/init/main.c @@ -1602,3 +1602,21 @@ static noinline void __init kernel_init_freeable(void) integrity_load_keys(); } + +/** + * staging_init_taint() - We need to taint the kernel whenever staging code + * is initialized (from built-in drivers) or loaded (as modules) and issue + * a warning the first time it happens. + */ +void staging_taint(const char *code_id, bool module) +{ + char *code_type = module ? "module" : "builtin driver"; + + pr_warn("%s %s: The kernel contains code from staging directory, " + "the quality is unknown, you have been warned.\n", + code_type, code_id); + + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK); +} +EXPORT_SYMBOL(staging_taint); + diff --git a/kernel/module/main.c b/kernel/module/main.c index d18a94b973e1..d7d33336ab43 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2039,9 +2039,7 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i check_modinfo_retpoline(mod, info); if (get_modinfo(info, "staging")) { - add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK); - pr_warn("%s: module is from the staging directory, the quality " - "is unknown, you have been warned.\n", mod->name); + staging_taint(mod->name, true); } if (is_livepatch_module(mod)) {
Fix the absence of warning message and kernel tainting when initializing drivers from the `drivers/staging` subtree from initcalls (when configured as built-in). When such a driver is built as module and the module is loaded, the `load_module()` function taints the kernel to signal code of unknown quality is loaded, and produces a warning like this: [ 8.076352] rts5208: module is from the staging directory, the quality is unknown, you have been warned. The same behaviour is absent, however, when a staging driver is compiled as built-in on the kernel image, since loading it happens through initcalls and not through load_module(). This might prevent relevant information of being available on a bug report (i.e. on a panic log) among other possible problems. NOTES: - The patch is written in such a way that all non-staging drivers are kept the way they were, except for staging drivers built with `-DSTAGING_CODE`. - Since it changes some macros related to clang LTO as well, I tested it and it works properly in kernels compiled with both clang and gcc. - Some `checkpatch.pl` errors, warnings and checks (with `--strict`) are present. Some were already there, some I introduced but I think they're unavoidable. Some IMHO don´t make sense at all, I think they would apply for most regular macros but initcall macros are just way different. Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code") Signed-off-by: Ágatha Isabelle Chris Moreira Guedes <code@agatha.dev> --- CHANGELOG v3: - Added some missing suggestions from Uwe's suggestions after I detected the oversight (the Fixes: tag). - I also noticed a possible build break, so I removed the `#ifdef CONFIG_STAGING` enclosuring the definition of `staging_taint()`. - Improved again the string details after further explanation of Dan's original suggestion in v2. v2: - Changed the way we hook into the initcalls as suggested by Uwe, and moved the logic from `include/linux/module.h` to `include/linux/init.h`. - Adjusted accordingly to work with both GCC and clang with `CONFIG_LTO_CLANG=y`, since some init scripts required that. - Fixed the missing space pointed out by Dan & other minor string details. - Changed the subject, since it became somewhat more of an init thing than a staging thing. drivers/staging/Makefile | 2 ++ include/linux/init.h | 76 ++++++++++++++++++++++++++++++++++------ init/main.c | 18 ++++++++++ kernel/module/main.c | 4 +-- 4 files changed, 86 insertions(+), 14 deletions(-)