Re: [PATCH] module/taint: Automatically increase the buffer size for new taint flags

[Rusty Russell <rusty@rustcorp.com.au>]

Petr Mladek <pmladek@suse.com> writes:
> The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
> add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
> potentially print one more character. But it did not increase the
> size of the corresponding buffers in m_show() and print_modules().

I agree, nice work!

Minor nitpick: the winged ' /* 0 */' comments imply the values matter,
but they don't.  I'd skip that.

I've CC'd Jessica to add to her review pile :)

Thanks,
Rusty.

> We have recently done the same mistake when adding a taint flag
> for livepatching, see
> https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoimboe@redhat.com
>
> Let's convert the taint flags into enum and handle the buffer size
> almost automatically.
>
> It is not optimal because only few taint flags can be printed by
> module_taint_flags(). But better be on the safe side. IMHO, it is
> not worth the optimization and this is a good compromise.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/kernel.h | 44 ++++++++++++++++++++++++--------------------
>  kernel/module.c        |  8 ++++++--
>  kernel/panic.c         |  4 ++--
>  3 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d96a6118d26a..1809bc82b7a5 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
>  	if (panic_timeout == arch_default_timeout)
>  		panic_timeout = timeout;
>  }
> -extern const char *print_tainted(void);
>  enum lockdep_ok {
>  	LOCKDEP_STILL_OK,
>  	LOCKDEP_NOW_UNRELIABLE
>  };
> -extern void add_taint(unsigned flag, enum lockdep_ok);
> -extern int test_taint(unsigned flag);
> -extern unsigned long get_taint(void);
>  extern int root_mountflags;
>  
>  extern bool early_boot_irqs_disabled;
> @@ -493,22 +489,30 @@ extern enum system_states {
>  	SYSTEM_RESTART,
>  } system_state;
>  
> -#define TAINT_PROPRIETARY_MODULE	0
> -#define TAINT_FORCED_MODULE		1
> -#define TAINT_CPU_OUT_OF_SPEC		2
> -#define TAINT_FORCED_RMMOD		3
> -#define TAINT_MACHINE_CHECK		4
> -#define TAINT_BAD_PAGE			5
> -#define TAINT_USER			6
> -#define TAINT_DIE			7
> -#define TAINT_OVERRIDDEN_ACPI_TABLE	8
> -#define TAINT_WARN			9
> -#define TAINT_CRAP			10
> -#define TAINT_FIRMWARE_WORKAROUND	11
> -#define TAINT_OOT_MODULE		12
> -#define TAINT_UNSIGNED_MODULE		13
> -#define TAINT_SOFTLOCKUP		14
> -#define TAINT_LIVEPATCH			15
> +enum taint_flags {
> +	TAINT_PROPRIETARY_MODULE,	/*  0 */
> +	TAINT_FORCED_MODULE,		/*  1 */
> +	TAINT_CPU_OUT_OF_SPEC,		/*  2 */
> +	TAINT_FORCED_RMMOD,		/*  3 */
> +	TAINT_MACHINE_CHECK,		/*  4 */
> +	TAINT_BAD_PAGE,			/*  5 */
> +	TAINT_USER,			/*  6 */
> +	TAINT_DIE,			/*  7 */
> +	TAINT_OVERRIDDEN_ACPI_TABLE,	/*  8 */
> +	TAINT_WARN,			/*  9 */
> +	TAINT_CRAP,			/* 10 */
> +	TAINT_FIRMWARE_WORKAROUND,	/* 11 */
> +	TAINT_OOT_MODULE,		/* 12 */
> +	TAINT_UNSIGNED_MODULE,		/* 13 */
> +	TAINT_SOFTLOCKUP,		/* 14 */
> +	TAINT_LIVEPATCH,		/* 15 */
> +	TAINT_FLAGS_COUNT		/* keep last! */
> +};
> +
> +extern const char *print_tainted(void);
> +extern void add_taint(enum taint_flags flag, enum lockdep_ok);
> +extern int test_taint(enum taint_flags flag);
> +extern unsigned long get_taint(void);
>  
>  extern const char hex_asc[];
>  #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
> diff --git a/kernel/module.c b/kernel/module.c
> index 529efae9f481..fb6c0d425b47 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4036,6 +4036,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
>  }
>  #endif /* CONFIG_KALLSYMS */
>  
> +/* Maximum number of characters written by module_flags() */
> +#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
> +
> +/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
>  static char *module_flags(struct module *mod, char *buf)
>  {
>  	int bx = 0;
> @@ -4080,7 +4084,7 @@ static void m_stop(struct seq_file *m, void *p)
>  static int m_show(struct seq_file *m, void *p)
>  {
>  	struct module *mod = list_entry(p, struct module, list);
> -	char buf[8];
> +	char buf[MODULE_FLAGS_BUF_SIZE];
>  
>  	/* We always ignore unformed modules. */
>  	if (mod->state == MODULE_STATE_UNFORMED)
> @@ -4251,7 +4255,7 @@ EXPORT_SYMBOL_GPL(__module_text_address);
>  void print_modules(void)
>  {
>  	struct module *mod;
> -	char buf[8];
> +	char buf[MODULE_FLAGS_BUF_SIZE];
>  
>  	printk(KERN_DEFAULT "Modules linked in:");
>  	/* Most callers should already have preempt disabled, but make sure */
> diff --git a/kernel/panic.c b/kernel/panic.c
> index ca8cea1ef673..e90125bf9238 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -334,7 +334,7 @@ const char *print_tainted(void)
>  	return buf;
>  }
>  
> -int test_taint(unsigned flag)
> +int test_taint(enum taint_flags flag)
>  {
>  	return test_bit(flag, &tainted_mask);
>  }
> @@ -353,7 +353,7 @@ unsigned long get_taint(void)
>   * If something bad has gone wrong, you'll want @lockdebug_ok = false, but for
>   * some notewortht-but-not-corrupting cases, it can be set to true.
>   */
> -void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
> +void add_taint(enum taint_flags flag, enum lockdep_ok lockdep_ok)
>  {
>  	if (lockdep_ok == LOCKDEP_NOW_UNRELIABLE && __debug_locks_off())
>  		pr_warn("Disabling lock debugging due to kernel taint\n");
> -- 
> 1.8.5.6