Re: [PATCH] staging: Fix missing warning/taint on builtin code

[Uwe Kleine-König]

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

Attachment: signature.asc
Description: PGP signature