| Message ID | 20200824105852.v5.1.I4feb11d34ce7a0dd5ee2c3327fb5a1a9a646be30@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] kernel/trace: Add DISALLOW_TRACE_PRINTK make option | expand |
From: Nicolas Boichat > Sent: 24 August 2020 03:59 > > trace_printk is meant as a debugging tool, and should not be > compiled into production code without specific debug Kconfig > options enabled, or source code changes, as indicated by the > warning that shows up on boot if any trace_printk is called: > ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** > ** ** > ** trace_printk() being used. Allocating extra memory. ** > ** ** > ** This means that this is a DEBUG kernel and it is ** > ** unsafe for production use. ** > > If DISALLOW_TRACE_PRINTK=1 is passed on the make command > line, the kernel will generate a build-time error if > trace_printk is used. We expect distributors to set this > option for their production kernels. > > Note that the code to handle trace_printk is still present, > so this does not prevent people from compiling out-of-tree > kernel modules, or BPF programs. > > Also, we are not making this a kernel config option as we > want the developer convenience of being able to reuse a > production kernel config, add trace_printk for debugging, > and rebuild, without any config changes. Since the objective seems to be to ensure there are no calls to trace_printk() in the git tree, but to allow them in uncommitted sources. Why not use a config option and rely on rand-config builds to detect any 'accidental' commits? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, 24 Aug 2020 08:26:13 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > Since the objective seems to be to ensure there are no > calls to trace_printk() in the git tree, but to allow > them in uncommitted sources. Why not use a config option > and rely on rand-config builds to detect any 'accidental' > commits? Because we don't want distros to ship with disallowing trace_printk, where someone finds a bug, sends the config to a developer, who then adds trace_printk() just to find that they can't use it. The point of avoiding a config was to keep the burden of having the developer needing it and having to then modify the config given to them. Also, it would then prevent those developing modules from using trace_printk() in their module if they build against one of these kernels. Finally, there's debug code in the kernel that legitimately uses trace_printk(), and those randconfigs will trigger on them. -- Steve
On Mon, 24 Aug 2020 10:59:13 +0800 Nicolas Boichat <drinkcat@chromium.org> wrote: > --- > > Changes since v4: > - Turns this into a make option, instead of a config > option, as suggested by Steven Rostedt <rostedt@goodmis.org>. > > Changes since v2/v3: > - Rebase only, v3 didn't exist as I just split out the other > necessary patches. > - Added patch 3/3 to fix atomisp_compat_css20.c > > Changes since v1: > - Use static_assert instead of __static_assert (Jason Gunthorpe) > - Fix issues that can be detected by this patch (running some > randconfig in a loop, kernel test robot, or manual inspection), > by: > - Making some debug config options that use trace_printk depend > on the new config option. > - Adding 3 patches before this one. > > Makefile | 14 ++++++++++++++ > include/linux/kernel.h | 17 ++++++++++++++++- > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index c4470a4e131f2ce..fb8b0d7fb4b2df7 100644 > --- a/Makefile > +++ b/Makefile > @@ -100,6 +100,16 @@ endif > > export quiet Q KBUILD_VERBOSE > > +# Disallow use of trace_printk. Can be used by production kernels. > +ifeq ("$(origin DISALLOW_TRACE_PRINTK)", "command line") > + KBUILD_DISALLOW_TRACE_PRINTK = $(DISALLOW_TRACE_PRINTK) > +endif > +ifndef KBUILD_DISALLOW_TRACE_PRINTK > + KBUILD_DISALLOW_TRACE_PRINTK = 0 > +endif > + > +export KBUILD_DISALLOW_TRACE_PRINTK > + > # Kbuild will save output files in the current working directory. > # This does not need to match to the root of the kernel source tree. > # > @@ -838,6 +848,10 @@ KBUILD_AFLAGS += -gz=zlib > KBUILD_LDFLAGS += --compress-debug-sections=zlib > endif > > +ifeq ($(KBUILD_DISALLOW_TRACE_PRINTK),1) > +KBUILD_CFLAGS += -DDISALLOW_TRACE_PRINTK > +endif > + > KBUILD_CFLAGS += $(DEBUG_CFLAGS) > export DEBUG_CFLAGS > There's one more thing we need to do, is if you build without this option then build with it, you should trigger a full kernel rebuild. Otherwise, if you build without the option, then build with it, and it doesn't rebuild the tree, it wont catch anything. -- Steve
On Mon, Aug 24, 2020 at 09:28:28AM -0400, Steven Rostedt wrote: > On Mon, 24 Aug 2020 08:26:13 +0000 > David Laight <David.Laight@ACULAB.COM> wrote: > > > Since the objective seems to be to ensure there are no > > calls to trace_printk() in the git tree, but to allow > > them in uncommitted sources. Why not use a config option > > and rely on rand-config builds to detect any 'accidental' > > commits? > > Because we don't want distros to ship with disallowing trace_printk, > where someone finds a bug, sends the config to a developer, who then > adds trace_printk() just to find that they can't use it. > > The point of avoiding a config was to keep the burden of having the > developer needing it and having to then modify the config given to them. > > Also, it would then prevent those developing modules from using > trace_printk() in their module if they build against one of these > kernels. > > Finally, there's debug code in the kernel that legitimately uses > trace_printk(), and those randconfigs will trigger on them. How making it make's option prevent some "smart" distros to achieve the same? AFAIU any compile-time knob will allow to build a kernel w/o a feature and you are against of such kernel builds in distros. Catch-22?
On Mon, 24 Aug 2020 16:42:01 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > How making it make's option prevent some "smart" distros to achieve the same? > AFAIU any compile-time knob will allow to build a kernel w/o a feature and you > are against of such kernel builds in distros. Catch-22? Because it will fail the build if it finds a trace_printk() in the compiled code, but doesn't touch the config that is shipped, nor does it affect modules being built against this kernel. This patch series is for those that do not want a trace_printk() accidentally left behind in their own work and trigger that big warning and scare their users. But it still gives an option for developers to add a trace_printk. That is, the decision to have trace_printk in a particular output (vmlinux) is done at the compile time, and all it does is to make sure one isn't present at that moment. This series is not about keeping them out completely (test modules, etc), which a config option will. -- Steve
On Mon, Aug 24, 2020 at 9:30 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 24 Aug 2020 10:59:13 +0800 > Nicolas Boichat <drinkcat@chromium.org> wrote: > > > --- > > > > +ifeq ($(KBUILD_DISALLOW_TRACE_PRINTK),1) > > +KBUILD_CFLAGS += -DDISALLOW_TRACE_PRINTK > > +endif > > + > > KBUILD_CFLAGS += $(DEBUG_CFLAGS) > > export DEBUG_CFLAGS > > > > > There's one more thing we need to do, is if you build without this option > then build with it, you should trigger a full kernel rebuild. > Otherwise, if you build without the option, then build with it, and it > doesn't rebuild the tree, it wont catch anything. This already works. I'll be honest, I'm not 100% sure why (and if fully intentional)... The CFLAGS end up in 3 generated assembly files: # grep -R DISALLOW_TRACE_PRINTK * | grep -v ".cmd:" arch/x86/kernel/asm-offsets.s:# -imultiarch x86_64-linux-gnu -D __KERNEL__ -D DISALLOW_TRACE_PRINTK kernel/bounds.s:# -imultiarch x86_64-linux-gnu -D __KERNEL__ -D DISALLOW_TRACE_PRINTK scripts/mod/devicetable-offsets.s:# -D DISALLOW_TRACE_PRINTK (along with all *.cmd files) and I suspect some/all of those force a complete kernel rebuild. > -- Steve
On Tue, 25 Aug 2020 08:38:27 +0800 Nicolas Boichat <drinkcat@chromium.org> wrote: > This already works. I'll be honest, I'm not 100% sure why (and if > fully intentional)... > > The CFLAGS end up in 3 generated assembly files: > # grep -R DISALLOW_TRACE_PRINTK * | grep -v ".cmd:" > arch/x86/kernel/asm-offsets.s:# -imultiarch x86_64-linux-gnu -D > __KERNEL__ -D DISALLOW_TRACE_PRINTK > kernel/bounds.s:# -imultiarch x86_64-linux-gnu -D __KERNEL__ -D > DISALLOW_TRACE_PRINTK > scripts/mod/devicetable-offsets.s:# -D DISALLOW_TRACE_PRINTK > (along with all *.cmd files) > > and I suspect some/all of those force a complete kernel rebuild. Yeah, I believe that the modification of CFLAGS causes a recompile of the entire kernel. Thanks! -- Steve
diff --git a/Makefile b/Makefile index c4470a4e131f2ce..fb8b0d7fb4b2df7 100644 --- a/Makefile +++ b/Makefile @@ -100,6 +100,16 @@ endif export quiet Q KBUILD_VERBOSE +# Disallow use of trace_printk. Can be used by production kernels. +ifeq ("$(origin DISALLOW_TRACE_PRINTK)", "command line") + KBUILD_DISALLOW_TRACE_PRINTK = $(DISALLOW_TRACE_PRINTK) +endif +ifndef KBUILD_DISALLOW_TRACE_PRINTK + KBUILD_DISALLOW_TRACE_PRINTK = 0 +endif + +export KBUILD_DISALLOW_TRACE_PRINTK + # Kbuild will save output files in the current working directory. # This does not need to match to the root of the kernel source tree. # @@ -838,6 +848,10 @@ KBUILD_AFLAGS += -gz=zlib KBUILD_LDFLAGS += --compress-debug-sections=zlib endif +ifeq ($(KBUILD_DISALLOW_TRACE_PRINTK),1) +KBUILD_CFLAGS += -DDISALLOW_TRACE_PRINTK +endif + KBUILD_CFLAGS += $(DEBUG_CFLAGS) export DEBUG_CFLAGS diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 500def620d8f493..7b533b0375596af 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -687,6 +687,12 @@ do { \ ____trace_printk_check_format(fmt, ##args); \ } while (0) +#ifdef DISALLOW_TRACE_PRINTK +#define ALLOW_TRACE_PRINTK 0 +#else +#define ALLOW_TRACE_PRINTK 1 +#endif + /** * trace_printk - printf formatting in the ftrace buffer * @fmt: the printf format for printing @@ -720,10 +726,13 @@ do { \ #define trace_printk(fmt, ...) \ do { \ char _______STR[] = __stringify((__VA_ARGS__)); \ + \ + static_assert(ALLOW_TRACE_PRINTK, "trace_printk called."); \ + \ if (sizeof(_______STR) > 3) \ do_trace_printk(fmt, ##__VA_ARGS__); \ else \ - trace_puts(fmt); \ + do_trace_puts(fmt); \ } while (0) #define do_trace_printk(fmt, args...) \ @@ -772,6 +781,11 @@ int __trace_printk(unsigned long ip, const char *fmt, ...); */ #define trace_puts(str) ({ \ + static_assert(ALLOW_TRACE_PRINTK, "trace_puts called."); \ + do_trace_puts(str); \ +}) + +#define do_trace_puts(str) ({ \ static const char *trace_printk_fmt __used \ __attribute__((section("__trace_printk_fmt"))) = \ __builtin_constant_p(str) ? str : NULL; \ @@ -793,6 +807,7 @@ extern void trace_dump_stack(int skip); */ #define ftrace_vprintk(fmt, vargs) \ do { \ + static_assert(ALLOW_TRACE_PRINTK, "ftrace_vprintk called."); \ if (__builtin_constant_p(fmt)) { \ static const char *trace_printk_fmt __used \ __attribute__((section("__trace_printk_fmt"))) = \
trace_printk is meant as a debugging tool, and should not be compiled into production code without specific debug Kconfig options enabled, or source code changes, as indicated by the warning that shows up on boot if any trace_printk is called: ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** ** ** ** trace_printk() being used. Allocating extra memory. ** ** ** ** This means that this is a DEBUG kernel and it is ** ** unsafe for production use. ** If DISALLOW_TRACE_PRINTK=1 is passed on the make command line, the kernel will generate a build-time error if trace_printk is used. We expect distributors to set this option for their production kernels. Note that the code to handle trace_printk is still present, so this does not prevent people from compiling out-of-tree kernel modules, or BPF programs. Also, we are not making this a kernel config option as we want the developer convenience of being able to reuse a production kernel config, add trace_printk for debugging, and rebuild, without any config changes. Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> --- Changes since v4: - Turns this into a make option, instead of a config option, as suggested by Steven Rostedt <rostedt@goodmis.org>. Changes since v2/v3: - Rebase only, v3 didn't exist as I just split out the other necessary patches. - Added patch 3/3 to fix atomisp_compat_css20.c Changes since v1: - Use static_assert instead of __static_assert (Jason Gunthorpe) - Fix issues that can be detected by this patch (running some randconfig in a loop, kernel test robot, or manual inspection), by: - Making some debug config options that use trace_printk depend on the new config option. - Adding 3 patches before this one. Makefile | 14 ++++++++++++++ include/linux/kernel.h | 17 ++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)