| Message ID | yf3ewok3u7fu7f4yqlcovcvedkhyjtibvwzlgbcjwf7ijcsktt@vrk54srlnk2n (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | staging: Fix missing warning/taint on builtin code | expand |
Hello Ágatha, On Tue, Jul 02, 2024 at 02:44:31AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote: > ACKNOWLEDGEMENTS > Thanks for Jookia, heat and ukleinek for the important comments & > suggestions on this patch prior to submission. FTR: That happend in the #kernelnewbies irc channel. > drivers/staging/Makefile | 2 ++ > include/linux/init.h | 6 ++++++ > include/linux/module.h | 33 +++++++++++++++++++++++++++++++++ > init/main.c | 20 ++++++++++++++++++++ > kernel/module/main.c | 4 +--- > 5 files changed, 62 insertions(+), 3 deletions(-) > > 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..68c37600958f 100644 > --- a/include/linux/init.h > +++ b/include/linux/init.h > @@ -397,4 +397,10 @@ void __init parse_early_options(char *cmdline); > #define __exit_p(x) NULL > #endif > > +#ifdef CONFIG_STAGING > +#ifndef __ASSEMBLY__ > +extern void staging_taint(const char *code_id, bool module); > +#endif /* __ASSEMBLY__ */ > +#endif /* CONFIG_STAGING */ You could drop the #ifdef for CONFIG_STAGING here. The obvious downside of this suggestion is that then you have a declaration of a function that isn't available depending on configuration. However the compiler doesn't care and the upside of not having CONFIG_STAGING in <linux/init.h> is that compile units that have nothing to do with CONFIG_STAGING but include <linux/init.h> won't get recompiled if you switch the setting. (OK, maybe a minor issue given that most drivers also include <linux/module.h> where the #ifdef cannot be dropped.) > #endif /* _LINUX_INIT_H */ > diff --git a/include/linux/module.h b/include/linux/module.h > index 330ffb59efe5..ffe58f5d143b 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -76,6 +76,39 @@ extern struct module_attribute module_uevent; > extern int init_module(void); > extern void cleanup_module(void); > > +#ifdef CONFIG_STAGING > + > +#define __lower_define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id) > + > +/** > + * __staging_define_initcall(fn,id) - staging initialization entry point > + * @fn: the function to run at kernel boot time > + * @id: the initcall level > + * > + * __staging_define_initcall() will ensure the drive's init function is always > + * called during initcalls for staging code by producing a wrapper function. > + * It applies if a module from the drivers/staging subtree is builtin to the > + * kernel. It reproduces the behavior in load_module() by tainting the kernel > + * and logging a warning about the code quality. > + */ > + > +#define __staging_define_initcall(fn, id) \ > + static int __init __staging_wrapped_##fn(void) \ > + { \ > + staging_taint(__FILE__, false); \ > + return fn(); \ > + } \ > +__lower_define_initcall(__staging_wrapped_##fn, id) > + > +#ifdef STAGING_CODE > + > +#undef __define_initcall > +#define __define_initcall(fn, id) __staging_define_initcall(fn, id) undefining macros makes the implemented logic hard to understand. While it allows to keep the touched code compact, in the long run it's more important that it's understandable. So I suggest to modify the original definition of __define_initcall(). > +#endif /* STAGING_CODE */ > + > +#endif /* CONFIG_STAGING */ > + > #ifndef MODULE > /** > * module_init() - driver initialization entry point I like the idea and think the missing taint for built-in code is an oversight that went unnoticed for an astonishingly long time (since 2008 if my quick research is right). Maybe add Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code") to the commit log?! Best regards Uwe
On Tue, Jul 02, 2024 at 02:44:31AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote: > diff --git a/init/main.c b/init/main.c > index 206acdde51f5..fca889f3bcc0 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1602,3 +1602,23 @@ static noinline void __init kernel_init_freeable(void) > > integrity_load_keys(); > } > + > +#ifdef CONFIG_STAGING > +/** > + * 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 at"; > + > + pr_warn("%s %s: The kernel contains code from staging directory" Needs a space after directory before the quote. > + "the quality is unknown, you have been warned.\n", > + code_type, code_id); > + > + add_taint(TAINT_CRAP, LOCKDEP_STILL_OK); > +} > +EXPORT_SYMBOL(staging_taint); regards, dan carpenter
On Tue, Jul 02, 2024 at 09:50:49AM GMT, Uwe Kleine-König wrote: > Hello Ágatha, > > On Tue, Jul 02, 2024 at 02:44:31AM -0300, Ágatha Isabelle Chris Moreira Guedes wrote: > > ACKNOWLEDGEMENTS > > Thanks for Jookia, heat and ukleinek for the important comments & > > suggestions on this patch prior to submission. > > FTR: That happend in the #kernelnewbies irc channel. <3 <3 <3 > > diff --git a/include/linux/init.h b/include/linux/init.h > > index 58cef4c2e59a..68c37600958f 100644 > > --- a/include/linux/init.h > > +++ b/include/linux/init.h > > @@ -397,4 +397,10 @@ void __init parse_early_options(char *cmdline); > > #define __exit_p(x) NULL > > #endif > > > > +#ifdef CONFIG_STAGING > > +#ifndef __ASSEMBLY__ > > +extern void staging_taint(const char *code_id, bool module); > > +#endif /* __ASSEMBLY__ */ > > +#endif /* CONFIG_STAGING */ > > You could drop the #ifdef for CONFIG_STAGING here. The obvious downside > of this suggestion is that then you have a declaration of a function > that isn't available depending on configuration. However the compiler > doesn't care and the upside of not having CONFIG_STAGING in > <linux/init.h> is that compile units that have nothing to do with > CONFIG_STAGING but include <linux/init.h> won't get recompiled if you > switch the setting. Thanks a lot for the suggestion, it hasn't ocurred to me (despite the intense task of recompiling it all over several times during development LOL). > > (OK, maybe a minor issue given that most drivers also include > <linux/module.h> where the #ifdef cannot be dropped.) For drivers it would be true, but with a very minimal config I am using, the core code that doesn't include it would get some relative build time improvement. Definetely worth, considering the machine I am using for kernel development here (a core2quad cpu LOL). > > diff --git a/include/linux/module.h b/include/linux/module.h > > index 330ffb59efe5..ffe58f5d143b 100644 > > --- a/include/linux/module.h > > +++ b/include/linux/module.h > > @@ -76,6 +76,39 @@ extern struct module_attribute module_uevent; > > extern int init_module(void); > > extern void cleanup_module(void); > > > > +#ifdef CONFIG_STAGING > > + > > +#define __lower_define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id) > > + > > +/** > > + * __staging_define_initcall(fn,id) - staging initialization entry point > > + * @fn: the function to run at kernel boot time > > + * @id: the initcall level > > + * > > + * __staging_define_initcall() will ensure the drive's init function is always > > + * called during initcalls for staging code by producing a wrapper function. > > + * It applies if a module from the drivers/staging subtree is builtin to the > > + * kernel. It reproduces the behavior in load_module() by tainting the kernel > > + * and logging a warning about the code quality. > > + */ > > + > > +#define __staging_define_initcall(fn, id) \ > > + static int __init __staging_wrapped_##fn(void) \ > > + { \ > > + staging_taint(__FILE__, false); \ > > + return fn(); \ > > + } \ > > +__lower_define_initcall(__staging_wrapped_##fn, id) > > + > > +#ifdef STAGING_CODE > > + > > +#undef __define_initcall > > +#define __define_initcall(fn, id) __staging_define_initcall(fn, id) > > undefining macros makes the implemented logic hard to understand. While > it allows to keep the touched code compact, in the long run it's more > important that it's understandable. > > So I suggest to modify the original definition of __define_initcall(). Yeah, I didn't like the idea of touching it at first, but it makes sense at this time. Thanks for suggesting! > > > +#endif /* STAGING_CODE */ > > + > > +#endif /* CONFIG_STAGING */ > > + > > #ifndef MODULE > > /** > > * module_init() - driver initialization entry point > > I like the idea and think the missing taint for built-in code is an > oversight that went unnoticed for an astonishingly long time (since 2008 > if my quick research is right). > > Maybe add > > Fixes: 061b1bd394ca ("Staging: add TAINT_CRAP for all drivers/staging code") > > to the commit log?! I'll do, thanks a lot so far Uwe! --- Sincerely, Ágatha Isabelle she/her https://agatha.dev
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..68c37600958f 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -397,4 +397,10 @@ void __init parse_early_options(char *cmdline); #define __exit_p(x) NULL #endif +#ifdef CONFIG_STAGING +#ifndef __ASSEMBLY__ +extern void staging_taint(const char *code_id, bool module); +#endif /* __ASSEMBLY__ */ +#endif /* CONFIG_STAGING */ + #endif /* _LINUX_INIT_H */ diff --git a/include/linux/module.h b/include/linux/module.h index 330ffb59efe5..ffe58f5d143b 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -76,6 +76,39 @@ extern struct module_attribute module_uevent; extern int init_module(void); extern void cleanup_module(void); +#ifdef CONFIG_STAGING + +#define __lower_define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id) + +/** + * __staging_define_initcall(fn,id) - staging initialization entry point + * @fn: the function to run at kernel boot time + * @id: the initcall level + * + * __staging_define_initcall() will ensure the drive's init function is always + * called during initcalls for staging code by producing a wrapper function. + * It applies if a module from the drivers/staging subtree is builtin to the + * kernel. It reproduces the behavior in load_module() by tainting the kernel + * and logging a warning about the code quality. + */ + +#define __staging_define_initcall(fn, id) \ + static int __init __staging_wrapped_##fn(void) \ + { \ + staging_taint(__FILE__, false); \ + return fn(); \ + } \ +__lower_define_initcall(__staging_wrapped_##fn, id) + +#ifdef STAGING_CODE + +#undef __define_initcall +#define __define_initcall(fn, id) __staging_define_initcall(fn, id) + +#endif /* STAGING_CODE */ + +#endif /* CONFIG_STAGING */ + #ifndef MODULE /** * module_init() - driver initialization entry point diff --git a/init/main.c b/init/main.c index 206acdde51f5..fca889f3bcc0 100644 --- a/init/main.c +++ b/init/main.c @@ -1602,3 +1602,23 @@ static noinline void __init kernel_init_freeable(void) integrity_load_keys(); } + +#ifdef CONFIG_STAGING +/** + * 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 at"; + + 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); + +#endif /* CONFIG_STAGING */ 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. Signed-off-by: Ágatha Isabelle Chris Moreira Guedes <code@agatha.dev> --- ACKNOWLEDGEMENTS Thanks for Jookia, heat and ukleinek for the important comments & suggestions on this patch prior to submission. drivers/staging/Makefile | 2 ++ include/linux/init.h | 6 ++++++ include/linux/module.h | 33 +++++++++++++++++++++++++++++++++ init/main.c | 20 ++++++++++++++++++++ kernel/module/main.c | 4 +--- 5 files changed, 62 insertions(+), 3 deletions(-)