Messages in this thread | | | Subject | Re: [PATCH v5] printk: Userspace format enumeration support | From | Rasmus Villemoes <> | Date | Tue, 16 Mar 2021 15:12:01 +0100 |
| |
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@chrisdown.name> > +S: Maintained > +F: kernel/printk/index.c > + > PRISM54 WIRELESS DRIVER > M: Luis Chamberlain <mcgrof@kernel.org> > L: linux-wireless@vger.kernel.org > 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
| |