| Message ID | 20210319075410.for-stable-4.19.1.I222f801866f71be9f7d85e5b10665cd4506d78ec@changeid (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Backport patches to fix KASAN+LKDTM with recent clang on ARM64 | expand |
On Fri, Mar 19, 2021 at 07:54:15AM +0800, Nicolas Boichat wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > commit 6553896666433e7efec589838b400a2a652b3ffa upstream. > > Some code pathes, especially the low level entry code, must be protected > against instrumentation for various reasons: > > - Low level entry code can be a fragile beast, especially on x86. > > - With NO_HZ_FULL RCU state needs to be established before using it. > > Having a dedicated section for such code allows to validate with tooling > that no unsafe functions are invoked. > > Add the .noinstr.text section and the noinstr attribute to mark > functions. noinstr implies notrace. Kprobes will gain a section check > later. > > Provide also a set of markers: instrumentation_begin()/end() > > These are used to mark code inside a noinstr function which calls > into regular instrumentable text section as safe. > > The instrumentation markers are only active when CONFIG_DEBUG_ENTRY is > enabled as the end marker emits a NOP to prevent the compiler from merging > the annotation points. This means the objtool verification requires a > kernel compiled with this option. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com> > Acked-by: Peter Zijlstra <peterz@infradead.org> > Link: https://lkml.kernel.org/r/20200505134100.075416272@linutronix.de > > [Nicolas: context conflicts in: > arch/powerpc/kernel/vmlinux.lds.S > include/asm-generic/vmlinux.lds.h > include/linux/compiler.h > include/linux/compiler_types.h] > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> Did you build this on x86? I get the following build error: ld:./arch/x86/kernel/vmlinux.lds:20: syntax error And that line looks like: . = ALIGN(8); *(.text.hot .text.hot.*) *(.text .text.fixup) *(.text.unlikely .text.unlikely.*) *(.text.unknown .text.unknown.*) . = ALIGN(8); __noinstr_text_start = .; *(.__attribute__((noinline)) __attribute__((no_instrument_function)) __attribute((__section__(".noinstr.text"))).text) __noinstr_text_end = .; *(.text..refcount) *(.ref.text) *(.meminit.text*) *(.memexit.text*) So I'm going to drop both of these patches from the queue. thanks, greg k-h
On 3/19/21 11:39 AM, Greg Kroah-Hartman wrote: > On Fri, Mar 19, 2021 at 07:54:15AM +0800, Nicolas Boichat wrote: >> From: Thomas Gleixner <tglx@linutronix.de> >> >> commit 6553896666433e7efec589838b400a2a652b3ffa upstream. >> >> Some code pathes, especially the low level entry code, must be protected >> against instrumentation for various reasons: >> >> - Low level entry code can be a fragile beast, especially on x86. >> >> - With NO_HZ_FULL RCU state needs to be established before using it. >> >> Having a dedicated section for such code allows to validate with tooling >> that no unsafe functions are invoked. >> >> Add the .noinstr.text section and the noinstr attribute to mark >> functions. noinstr implies notrace. Kprobes will gain a section check >> later. >> >> Provide also a set of markers: instrumentation_begin()/end() >> >> These are used to mark code inside a noinstr function which calls >> into regular instrumentable text section as safe. >> >> The instrumentation markers are only active when CONFIG_DEBUG_ENTRY is >> enabled as the end marker emits a NOP to prevent the compiler from merging >> the annotation points. This means the objtool verification requires a >> kernel compiled with this option. >> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >> Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com> >> Acked-by: Peter Zijlstra <peterz@infradead.org> >> Link: https://lkml.kernel.org/r/20200505134100.075416272@linutronix.de >> >> [Nicolas: context conflicts in: >> arch/powerpc/kernel/vmlinux.lds.S >> include/asm-generic/vmlinux.lds.h >> include/linux/compiler.h >> include/linux/compiler_types.h] >> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > Did you build this on x86? > > I get the following build error: > > ld:./arch/x86/kernel/vmlinux.lds:20: syntax error > > And that line looks like: > > . = ALIGN(8); *(.text.hot .text.hot.*) *(.text .text.fixup) *(.text.unlikely .text.unlikely.*) *(.text.unknown .text.unknown.*) . = ALIGN(8); __noinstr_text_start = .; *(.__attribute__((noinline)) __attribute__((no_instrument_function)) __attribute((__section__(".noinstr.text"))).text) __noinstr_text_end = .; *(.text..refcount) *(.ref.text) *(.meminit.text*) *(.memexit.text*) > In the NOINSTR_TEXT macro, noinstr is expanded with the value of the noinstr macro from linux/compiler_types.h while it shouldn't. The problem is possibly that the noinstr macro is defined for assembly. Make sure that the macro is not defined for assembly e.g.: #ifndef __ASSEMBLY__ /* Section for code which can't be instrumented at all */ #define noinstr \ noinline notrace __attribute((__section__(".noinstr.text"))) #endif alex.
On Fri, Mar 19, 2021 at 12:20:22PM +0100, Alexandre Chartre wrote: > > On 3/19/21 11:39 AM, Greg Kroah-Hartman wrote: > > On Fri, Mar 19, 2021 at 07:54:15AM +0800, Nicolas Boichat wrote: > > > From: Thomas Gleixner <tglx@linutronix.de> > > > > > > commit 6553896666433e7efec589838b400a2a652b3ffa upstream. > > > > > > Some code pathes, especially the low level entry code, must be protected > > > against instrumentation for various reasons: > > > > > > - Low level entry code can be a fragile beast, especially on x86. > > > > > > - With NO_HZ_FULL RCU state needs to be established before using it. > > > > > > Having a dedicated section for such code allows to validate with tooling > > > that no unsafe functions are invoked. > > > > > > Add the .noinstr.text section and the noinstr attribute to mark > > > functions. noinstr implies notrace. Kprobes will gain a section check > > > later. > > > > > > Provide also a set of markers: instrumentation_begin()/end() > > > > > > These are used to mark code inside a noinstr function which calls > > > into regular instrumentable text section as safe. > > > > > > The instrumentation markers are only active when CONFIG_DEBUG_ENTRY is > > > enabled as the end marker emits a NOP to prevent the compiler from merging > > > the annotation points. This means the objtool verification requires a > > > kernel compiled with this option. > > > > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > > Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com> > > > Acked-by: Peter Zijlstra <peterz@infradead.org> > > > Link: https://lkml.kernel.org/r/20200505134100.075416272@linutronix.de > > > > > > [Nicolas: context conflicts in: > > > arch/powerpc/kernel/vmlinux.lds.S > > > include/asm-generic/vmlinux.lds.h > > > include/linux/compiler.h > > > include/linux/compiler_types.h] > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > Did you build this on x86? > > > > I get the following build error: > > > > ld:./arch/x86/kernel/vmlinux.lds:20: syntax error > > > > And that line looks like: > > > > . = ALIGN(8); *(.text.hot .text.hot.*) *(.text .text.fixup) *(.text.unlikely .text.unlikely.*) *(.text.unknown .text.unknown.*) . = ALIGN(8); __noinstr_text_start = .; *(.__attribute__((noinline)) __attribute__((no_instrument_function)) __attribute((__section__(".noinstr.text"))).text) __noinstr_text_end = .; *(.text..refcount) *(.ref.text) *(.meminit.text*) *(.memexit.text*) > > > > In the NOINSTR_TEXT macro, noinstr is expanded with the value of the noinstr > macro from linux/compiler_types.h while it shouldn't. > > The problem is possibly that the noinstr macro is defined for assembly. Make > sure that the macro is not defined for assembly e.g.: > > #ifndef __ASSEMBLY__ > > /* Section for code which can't be instrumented at all */ > #define noinstr \ > noinline notrace __attribute((__section__(".noinstr.text"))) > > #endif This implies that the backport is incorrect, so I'll wait for an updated version... thanks, greg k-h
On Fri, Mar 19, 2021 at 7:55 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Mar 19, 2021 at 12:20:22PM +0100, Alexandre Chartre wrote: > > > > On 3/19/21 11:39 AM, Greg Kroah-Hartman wrote: > > > On Fri, Mar 19, 2021 at 07:54:15AM +0800, Nicolas Boichat wrote: > > > > From: Thomas Gleixner <tglx@linutronix.de> > > > > > > > > commit 6553896666433e7efec589838b400a2a652b3ffa upstream. > > > > > > > > Some code pathes, especially the low level entry code, must be protected > > > > against instrumentation for various reasons: > > > > > > > > - Low level entry code can be a fragile beast, especially on x86. > > > > > > > > - With NO_HZ_FULL RCU state needs to be established before using it. > > > > > > > > Having a dedicated section for such code allows to validate with tooling > > > > that no unsafe functions are invoked. > > > > > > > > Add the .noinstr.text section and the noinstr attribute to mark > > > > functions. noinstr implies notrace. Kprobes will gain a section check > > > > later. > > > > > > > > Provide also a set of markers: instrumentation_begin()/end() > > > > > > > > These are used to mark code inside a noinstr function which calls > > > > into regular instrumentable text section as safe. > > > > > > > > The instrumentation markers are only active when CONFIG_DEBUG_ENTRY is > > > > enabled as the end marker emits a NOP to prevent the compiler from merging > > > > the annotation points. This means the objtool verification requires a > > > > kernel compiled with this option. > > > > > > > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > > > Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com> > > > > Acked-by: Peter Zijlstra <peterz@infradead.org> > > > > Link: https://lkml.kernel.org/r/20200505134100.075416272@linutronix.de > > > > > > > > [Nicolas: context conflicts in: > > > > arch/powerpc/kernel/vmlinux.lds.S > > > > include/asm-generic/vmlinux.lds.h > > > > include/linux/compiler.h > > > > include/linux/compiler_types.h] > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > > Did you build this on x86? > > > > > > I get the following build error: > > > > > > ld:./arch/x86/kernel/vmlinux.lds:20: syntax error > > > > > > And that line looks like: > > > > > > . = ALIGN(8); *(.text.hot .text.hot.*) *(.text .text.fixup) *(.text.unlikely .text.unlikely.*) *(.text.unknown .text.unknown.*) . = ALIGN(8); __noinstr_text_start = .; *(.__attribute__((noinline)) __attribute__((no_instrument_function)) __attribute((__section__(".noinstr.text"))).text) __noinstr_text_end = .; *(.text..refcount) *(.ref.text) *(.meminit.text*) *(.memexit.text*) > > > > > > > In the NOINSTR_TEXT macro, noinstr is expanded with the value of the noinstr > > macro from linux/compiler_types.h while it shouldn't. > > > > The problem is possibly that the noinstr macro is defined for assembly. Make > > sure that the macro is not defined for assembly e.g.: > > > > #ifndef __ASSEMBLY__ > > > > /* Section for code which can't be instrumented at all */ > > #define noinstr \ > > noinline notrace __attribute((__section__(".noinstr.text"))) > > > > #endif > > This implies that the backport is incorrect, so I'll wait for an updated > version... Yep, sorry about that. I did test on ARM64 only and these patches happily went through our Chrome OS CQ (we don't have gcc coverage though). Guenter has a fixup here with explanation: https://crrev.com/c/2776332, I'll look carefully and resubmit. Thanks, > thanks, > > greg k-h
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S index 695432965f20..9b346f3d2814 100644 --- a/arch/powerpc/kernel/vmlinux.lds.S +++ b/arch/powerpc/kernel/vmlinux.lds.S @@ -99,6 +99,7 @@ SECTIONS #endif /* careful! __ftr_alt_* sections need to be close to .text */ *(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup __ftr_alt_* .ref.text); + NOINSTR_TEXT SCHED_TEXT CPUIDLE_TEXT LOCK_TEXT diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index 849cd8eb5ca0..ea5987bb0b84 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -53,6 +53,9 @@ extern char __ctors_start[], __ctors_end[]; /* Start and end of .opd section - used for function descriptors. */ extern char __start_opd[], __end_opd[]; +/* Start and end of instrumentation protected text section */ +extern char __noinstr_text_start[], __noinstr_text_end[]; + extern __visible const void __nosave_begin, __nosave_end; /* Function descriptor handling (if any). Override in asm/sections.h */ diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 2d632a74cc5e..88484ee023ca 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -482,6 +482,15 @@ __security_initcall_end = .; \ } +/* + * Non-instrumentable text section + */ +#define NOINSTR_TEXT \ + ALIGN_FUNCTION(); \ + __noinstr_text_start = .; \ + *(.noinstr.text) \ + __noinstr_text_end = .; + /* * .text section. Map to function alignment to avoid address changes * during second ld run in second ld pass when generating System.map @@ -496,6 +505,7 @@ *(TEXT_MAIN .text.fixup) \ *(.text.unlikely .text.unlikely.*) \ *(.text.unknown .text.unknown.*) \ + NOINSTR_TEXT \ *(.text..refcount) \ *(.ref.text) \ MEM_KEEP(init.text*) \ diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 6b6505e3b2c7..6a53300cbd1e 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -129,11 +129,65 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, ".pushsection .discard.unreachable\n\t" \ ".long 999b - .\n\t" \ ".popsection\n\t" + +#ifdef CONFIG_DEBUG_ENTRY +/* Begin/end of an instrumentation safe region */ +#define instrumentation_begin() ({ \ + asm volatile("%c0:\n\t" \ + ".pushsection .discard.instr_begin\n\t" \ + ".long %c0b - .\n\t" \ + ".popsection\n\t" : : "i" (__COUNTER__)); \ +}) + +/* + * Because instrumentation_{begin,end}() can nest, objtool validation considers + * _begin() a +1 and _end() a -1 and computes a sum over the instructions. + * When the value is greater than 0, we consider instrumentation allowed. + * + * There is a problem with code like: + * + * noinstr void foo() + * { + * instrumentation_begin(); + * ... + * if (cond) { + * instrumentation_begin(); + * ... + * instrumentation_end(); + * } + * bar(); + * instrumentation_end(); + * } + * + * If instrumentation_end() would be an empty label, like all the other + * annotations, the inner _end(), which is at the end of a conditional block, + * would land on the instruction after the block. + * + * If we then consider the sum of the !cond path, we'll see that the call to + * bar() is with a 0-value, even though, we meant it to happen with a positive + * value. + * + * To avoid this, have _end() be a NOP instruction, this ensures it will be + * part of the condition block and does not escape. + */ +#define instrumentation_end() ({ \ + asm volatile("%c0: nop\n\t" \ + ".pushsection .discard.instr_end\n\t" \ + ".long %c0b - .\n\t" \ + ".popsection\n\t" : : "i" (__COUNTER__)); \ +}) +#endif /* CONFIG_DEBUG_ENTRY */ + #else #define annotate_reachable() #define annotate_unreachable() #endif +#ifndef instrumentation_begin +#define instrumentation_begin() do { } while(0) +#define instrumentation_end() do { } while(0) +#endif + #ifndef ASM_UNREACHABLE # define ASM_UNREACHABLE #endif diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 2b8ed70c4c77..a9b0495051a3 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -234,6 +234,10 @@ struct ftrace_likely_data { #define notrace __attribute__((no_instrument_function)) #endif +/* Section for code which can't be instrumented at all */ +#define noinstr \ + noinline notrace __attribute((__section__(".noinstr.text"))) + /* * it doesn't make sense on ARM (currently the only user of __naked) * to trace naked functions because then mcount is called without diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 91a80036c05d..7c693bd775c1 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -895,7 +895,7 @@ static void check_section(const char *modname, struct elf_info *elf, #define DATA_SECTIONS ".data", ".data.rel" #define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ - ".kprobes.text", ".cpuidle.text" + ".kprobes.text", ".cpuidle.text", ".noinstr.text" #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ ".fixup", ".entry.text", ".exception.text", ".text.*", \ ".coldtext"