Re: [PATCH v5] printk: Userspace format enumeration support

[Rasmus Villemoes]

On 10/03/2021 03.30, Chris Down wrote:

> ---
>  MAINTAINERS                          |   5 +
>  arch/arm/kernel/entry-v7m.S          |   2 +-
>  arch/arm/lib/backtrace-clang.S       |   2 +-
>  arch/arm/lib/backtrace.S             |   2 +-
>  arch/arm/mach-rpc/io-acorn.S         |   2 +-
>  arch/arm/vfp/vfphw.S                 |   6 +-
>  arch/ia64/include/uapi/asm/cmpxchg.h |   4 +-
>  arch/openrisc/kernel/entry.S         |   6 +-
>  arch/powerpc/kernel/head_fsl_booke.S |   2 +-
>  arch/um/include/shared/user.h        |   3 +-
>  arch/x86/kernel/head_32.S            |   2 +-
>  fs/seq_file.c                        |  21 +++
>  include/asm-generic/vmlinux.lds.h    |  13 ++
>  include/linux/module.h               |   6 +
>  include/linux/printk.h               |  72 ++++++++++-
>  include/linux/seq_file.h             |   1 +
>  include/linux/string_helpers.h       |   2 +
>  init/Kconfig                         |  14 ++
>  kernel/module.c                      |  14 +-
>  kernel/printk/Makefile               |   1 +
>  kernel/printk/index.c                | 183 +++++++++++++++++++++++++++
>  kernel/printk/printk.c               |  20 ++-
>  lib/string_helpers.c                 |  29 ++++-
>  lib/test-string_helpers.c            |   6 +
>  24 files changed, 386 insertions(+), 32 deletions(-)
>  create mode 100644 kernel/printk/index.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3353de0c4bc8..328b3e822223 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14314,6 +14314,11 @@ S:	Maintained
>  F:	include/linux/printk.h
>  F:	kernel/printk/
>  
> +PRINTK INDEXING
> +R:	Chris Down <chris@xxxxxxxxxxxxxx>
> +S:	Maintained
> +F:	kernel/printk/index.c
> +
>  PRISM54 WIRELESS DRIVER
>  M:	Luis Chamberlain <mcgrof@xxxxxxxxxx>
>  L:	linux-wireless@xxxxxxxxxxxxxxx
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index d0e898608d30..7bde93c10962 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -23,7 +23,7 @@ __invalid_entry:
>  	adr	r0, strerr
>  	mrs	r1, ipsr
>  	mov	r2, lr
> -	bl	printk
> +	bl	_printk

I think it's pointless renaming the symbol to _printk, with all the
churn and reduced readability that involves (especially when reading
assembly "why are we calling _printk and not printk here?"). There's
nothing wrong with providing a macro wrapper by the same name

#define printk(bla bla) ({ do_stuff; printk(bla bla); })

Only two places would need to be updated to surround the word printk in
parentheses to suppress macro expansion: The declaration and the
definition of printk. I.e.

int (printk)(const char *s, ...)

>  
> +struct module;
> +
> +#ifdef CONFIG_PRINTK_INDEX
> +extern void pi_sec_store(struct module *mod);
> +extern void pi_sec_remove(struct module *mod);
> +
> +struct pi_object {
> +	const char *fmt;
> +	const char *func;
> +	const char *file;
> +	unsigned int line;
> +};
> +
> +extern struct pi_object __start_printk_index[];
> +extern struct pi_object __stop_printk_index[];

Do you need these declarations to be visible to the whole kernel? Can't
they live in printk/index.c?

> +
> +#define pi_sec_elf_embed(_p_func, _fmt, ...)				       \
> +	({								       \
> +		int _p_ret;						       \
> +									       \
> +		if (__builtin_constant_p(_fmt)) {			       \
> +			/*
> +			 * The compiler may not be able to eliminate this, so
> +			 * we need to make sure that it doesn't see any
> +			 * hypothetical assignment for non-constants even
> +			 * though this is already inside the
> +			 * __builtin_constant_p guard.
> +			 */						       \
> +			static struct pi_object _pi			       \

static const struct pi_object?

> +			__section(".printk_index") = {			       \
> +				.fmt = __builtin_constant_p(_fmt) ? (_fmt) : NULL, \
> +				.func = __func__,			       \
> +				.file = __FILE__,			       \
> +				.line = __LINE__,			       \
> +			};						       \
> +			_p_ret = _p_func(_pi.fmt, ##__VA_ARGS__);	       \

Is the use of _pi.fmt here a trick to prevent gcc from eliding the _pi
object, so it is seen as "used"? That seems a bit fragile, especially if
the compiler ends up generating the same code in .text - that means gcc
does not load the format string from the _pi object (which it
shouldn't), but then I don't see why it (or the next version of gcc)
couldn't realize that _pi is indeed unused.

There's the __used attribute precisely for this kind of thing. Then you
could also eliminate

> +		} else							       \
> +			_p_ret = _p_func(_fmt, ##__VA_ARGS__);		       \
> +									       \

this and the _p_ret variable

> +		_p_ret;							       \

and just end the ({}) with _p_func(_fmt, ##__VA_ARGS__);

That would also allow you to more easily wrap, say, dev_printk(), which
returns void - it seems that by not handling dev_printk and friends
you're missing quite a few format strings.

Rasmus