* [PATCH v2 0/2] jump_label: Robustify jump label patching @ 2018-02-16 16:31 Josh Poimboeuf 2018-02-16 16:31 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Josh Poimboeuf 2018-02-16 16:31 ` [PATCH v2 2/2] jump_label: Warn on failed jump_label patch Josh Poimboeuf 0 siblings, 2 replies; 11+ messages in thread From: Josh Poimboeuf @ 2018-02-16 16:31 UTC (permalink / raw) To: x86 Cc: linux-kernel, Steven Rostedt, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov v2: - Refine the warning so that it doesn't warn about __init entries - (Do so by explicitly disabling __init entries) - Drop v1 patches which removed __init tracepoints Josh Poimboeuf (2): jump_label: Explicitly disable jump labels in __init code jump_label: Warn on failed jump_label patch include/linux/jump_label.h | 3 +++ init/main.c | 2 ++ kernel/jump_label.c | 31 ++++++++++++++++++++++++------- 3 files changed, 29 insertions(+), 7 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-16 16:31 [PATCH v2 0/2] jump_label: Robustify jump label patching Josh Poimboeuf @ 2018-02-16 16:31 ` Josh Poimboeuf 2018-02-16 16:55 ` Jason Baron 2018-02-17 10:38 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Ingo Molnar 2018-02-16 16:31 ` [PATCH v2 2/2] jump_label: Warn on failed jump_label patch Josh Poimboeuf 1 sibling, 2 replies; 11+ messages in thread From: Josh Poimboeuf @ 2018-02-16 16:31 UTC (permalink / raw) To: x86 Cc: linux-kernel, Steven Rostedt, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov After initmem has been freed, any jump label entries in __init code are prevented from being written to by the kernel_text_address() check in __jump_label_update(). However, this check is quite broad. If kernel_text_address() were to return false for any other reason, the jump label write would fail silently with no warning. For jump label entrieds in module init code, entry->code is set to zero to indicate that the entry is disabled. Do the same thing for core kernel init code. This makes the behavior more consistent, and will also make it more straightforward to detect non-init jump label write failures in the next patch. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- include/linux/jump_label.h | 3 +++ init/main.c | 2 ++ kernel/jump_label.c | 18 ++++++++++++++++-- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h index b6a29c126cc4..2168cc6b8b30 100644 --- a/include/linux/jump_label.h +++ b/include/linux/jump_label.h @@ -151,6 +151,7 @@ extern struct jump_entry __start___jump_table[]; extern struct jump_entry __stop___jump_table[]; extern void jump_label_init(void); +extern void jump_label_invalidate_init(void); extern void jump_label_lock(void); extern void jump_label_unlock(void); extern void arch_jump_label_transform(struct jump_entry *entry, @@ -198,6 +199,8 @@ static __always_inline void jump_label_init(void) static_key_initialized = true; } +static inline void jump_label_invalidate_init(void) {} + static __always_inline bool static_key_false(struct static_key *key) { if (unlikely(static_key_count(key) > 0)) diff --git a/init/main.c b/init/main.c index a8100b954839..969eaf140ef0 100644 --- a/init/main.c +++ b/init/main.c @@ -89,6 +89,7 @@ #include <linux/io.h> #include <linux/cache.h> #include <linux/rodata_test.h> +#include <linux/jump_label.h> #include <asm/io.h> #include <asm/bugs.h> @@ -1000,6 +1001,7 @@ static int __ref kernel_init(void *unused) /* need to finish all async __init code before freeing the memory */ async_synchronize_full(); ftrace_free_init_mem(); + jump_label_invalidate_init(); free_initmem(); mark_readonly(); system_state = SYSTEM_RUNNING; diff --git a/kernel/jump_label.c b/kernel/jump_label.c index b4517095db6a..96274c6d3511 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -16,6 +16,7 @@ #include <linux/jump_label_ratelimit.h> #include <linux/bug.h> #include <linux/cpu.h> +#include <asm/sections.h> #ifdef HAVE_JUMP_LABEL @@ -633,16 +634,29 @@ static void jump_label_del_module(struct module *mod) } } +/* Disable any jump label entries in __init code */ +void __init jump_label_invalidate_init(void) +{ + struct jump_entry *iter_start = __start___jump_table; + struct jump_entry *iter_stop = __stop___jump_table; + struct jump_entry *iter; + + for (iter = iter_start; iter < iter_stop; iter++) + if (iter->code >= (unsigned long)_sinittext && + iter->code < (unsigned long)_einittext) + iter->code = 0; +} + +/* Disable any jump label entries in module init code */ static void jump_label_invalidate_module_init(struct module *mod) { struct jump_entry *iter_start = mod->jump_entries; struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; struct jump_entry *iter; - for (iter = iter_start; iter < iter_stop; iter++) { + for (iter = iter_start; iter < iter_stop; iter++) if (within_module_init(iter->code, mod)) iter->code = 0; - } } static int -- 2.14.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-16 16:31 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Josh Poimboeuf @ 2018-02-16 16:55 ` Jason Baron 2018-02-16 17:57 ` [PATCH] extable: Make init_kernel_text() global Josh Poimboeuf 2018-02-17 10:38 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Ingo Molnar 1 sibling, 1 reply; 11+ messages in thread From: Jason Baron @ 2018-02-16 16:55 UTC (permalink / raw) To: Josh Poimboeuf, x86 Cc: linux-kernel, Steven Rostedt, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Borislav Petkov On 02/16/2018 11:31 AM, Josh Poimboeuf wrote: > After initmem has been freed, any jump label entries in __init code are > prevented from being written to by the kernel_text_address() check in > __jump_label_update(). However, this check is quite broad. If > kernel_text_address() were to return false for any other reason, the > jump label write would fail silently with no warning. > > For jump label entrieds in module init code, entry->code is set to zero > to indicate that the entry is disabled. Do the same thing for core > kernel init code. This makes the behavior more consistent, and will > also make it more straightforward to detect non-init jump label write > failures in the next patch. > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > --- > include/linux/jump_label.h | 3 +++ > init/main.c | 2 ++ > kernel/jump_label.c | 18 ++++++++++++++++-- > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h > index b6a29c126cc4..2168cc6b8b30 100644 > --- a/include/linux/jump_label.h > +++ b/include/linux/jump_label.h > @@ -151,6 +151,7 @@ extern struct jump_entry __start___jump_table[]; > extern struct jump_entry __stop___jump_table[]; > > extern void jump_label_init(void); > +extern void jump_label_invalidate_init(void); > extern void jump_label_lock(void); > extern void jump_label_unlock(void); > extern void arch_jump_label_transform(struct jump_entry *entry, > @@ -198,6 +199,8 @@ static __always_inline void jump_label_init(void) > static_key_initialized = true; > } > > +static inline void jump_label_invalidate_init(void) {} > + > static __always_inline bool static_key_false(struct static_key *key) > { > if (unlikely(static_key_count(key) > 0)) > diff --git a/init/main.c b/init/main.c > index a8100b954839..969eaf140ef0 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -89,6 +89,7 @@ > #include <linux/io.h> > #include <linux/cache.h> > #include <linux/rodata_test.h> > +#include <linux/jump_label.h> > > #include <asm/io.h> > #include <asm/bugs.h> > @@ -1000,6 +1001,7 @@ static int __ref kernel_init(void *unused) > /* need to finish all async __init code before freeing the memory */ > async_synchronize_full(); > ftrace_free_init_mem(); > + jump_label_invalidate_init(); > free_initmem(); > mark_readonly(); > system_state = SYSTEM_RUNNING; > diff --git a/kernel/jump_label.c b/kernel/jump_label.c > index b4517095db6a..96274c6d3511 100644 > --- a/kernel/jump_label.c > +++ b/kernel/jump_label.c > @@ -16,6 +16,7 @@ > #include <linux/jump_label_ratelimit.h> > #include <linux/bug.h> > #include <linux/cpu.h> > +#include <asm/sections.h> > > #ifdef HAVE_JUMP_LABEL > > @@ -633,16 +634,29 @@ static void jump_label_del_module(struct module *mod) > } > } > > +/* Disable any jump label entries in __init code */ > +void __init jump_label_invalidate_init(void) > +{ > + struct jump_entry *iter_start = __start___jump_table; > + struct jump_entry *iter_stop = __stop___jump_table; > + struct jump_entry *iter; > + > + for (iter = iter_start; iter < iter_stop; iter++) > + if (iter->code >= (unsigned long)_sinittext && > + iter->code < (unsigned long)_einittext) > + iter->code = 0; > +} Seems like this wants to use init_kernel_text() but i see its marked 'static', perhaps it can be moved to a header? Thanks, -Jason > + > +/* Disable any jump label entries in module init code */ > static void jump_label_invalidate_module_init(struct module *mod) > { > struct jump_entry *iter_start = mod->jump_entries; > struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; > struct jump_entry *iter; > > - for (iter = iter_start; iter < iter_stop; iter++) { > + for (iter = iter_start; iter < iter_stop; iter++) > if (within_module_init(iter->code, mod)) > iter->code = 0; > - } > } > > static int > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] extable: Make init_kernel_text() global 2018-02-16 16:55 ` Jason Baron @ 2018-02-16 17:57 ` Josh Poimboeuf 2018-02-16 18:03 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Josh Poimboeuf @ 2018-02-16 17:57 UTC (permalink / raw) To: Jason Baron Cc: x86, linux-kernel, Steven Rostedt, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Borislav Petkov On Fri, Feb 16, 2018 at 11:55:54AM -0500, Jason Baron wrote: > > +/* Disable any jump label entries in __init code */ > > +void __init jump_label_invalidate_init(void) > > +{ > > + struct jump_entry *iter_start = __start___jump_table; > > + struct jump_entry *iter_stop = __stop___jump_table; > > + struct jump_entry *iter; > > + > > + for (iter = iter_start; iter < iter_stop; iter++) > > + if (iter->code >= (unsigned long)_sinittext && > > + iter->code < (unsigned long)_einittext) > > + iter->code = 0; > > +} > > Seems like this wants to use init_kernel_text() but i see its marked > 'static', perhaps it can be moved to a header? How about this patch on top? --------- From: Josh Poimboeuf <jpoimboe@redhat.com> Subject: [PATCH] extable: Make init_kernel_text() global Convert init_kernel_text() to a global function and use it in a few places instead of manually comparing _sinittext and _einittext. Note that kallsyms.h has a very similar function called is_kernel_inittext(), but its end check is inclusive. I'm not sure whether that's intentional behavior, so I didn't touch it. Suggested-by: Jason Baron <jbaron@akamai.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- arch/x86/kernel/unwind_orc.c | 3 +-- include/linux/kernel.h | 1 + kernel/extable.c | 2 +- kernel/jump_label.c | 4 +--- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c index 1f9188f5357c..feb28fee6cea 100644 --- a/arch/x86/kernel/unwind_orc.c +++ b/arch/x86/kernel/unwind_orc.c @@ -5,7 +5,6 @@ #include <asm/unwind.h> #include <asm/orc_types.h> #include <asm/orc_lookup.h> -#include <asm/sections.h> #define orc_warn(fmt, ...) \ printk_deferred_once(KERN_WARNING pr_fmt("WARNING: " fmt), ##__VA_ARGS__) @@ -148,7 +147,7 @@ static struct orc_entry *orc_find(unsigned long ip) } /* vmlinux .init slow lookup: */ - if (ip >= (unsigned long)_sinittext && ip < (unsigned long)_einittext) + if (init_kernel_text(ip)) return __orc_find(__start_orc_unwind_ip, __start_orc_unwind, __stop_orc_unwind_ip - __start_orc_unwind_ip, ip); diff --git a/include/linux/kernel.h b/include/linux/kernel.h index ce51455e2adf..3fd291503576 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -472,6 +472,7 @@ extern bool parse_option_str(const char *str, const char *option); extern char *next_arg(char *args, char **param, char **val); extern int core_kernel_text(unsigned long addr); +extern int init_kernel_text(unsigned long addr); extern int core_kernel_data(unsigned long addr); extern int __kernel_text_address(unsigned long addr); extern int kernel_text_address(unsigned long addr); diff --git a/kernel/extable.c b/kernel/extable.c index a17fdb63dc3e..6a5b61ebc66c 100644 --- a/kernel/extable.c +++ b/kernel/extable.c @@ -64,7 +64,7 @@ const struct exception_table_entry *search_exception_tables(unsigned long addr) return e; } -static inline int init_kernel_text(unsigned long addr) +int init_kernel_text(unsigned long addr) { if (addr >= (unsigned long)_sinittext && addr < (unsigned long)_einittext) diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 08a02ae54997..6cd43460cadc 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -16,7 +16,6 @@ #include <linux/jump_label_ratelimit.h> #include <linux/bug.h> #include <linux/cpu.h> -#include <asm/sections.h> #ifdef HAVE_JUMP_LABEL @@ -645,8 +644,7 @@ void __init jump_label_invalidate_init(void) struct jump_entry *iter; for (iter = iter_start; iter < iter_stop; iter++) - if (iter->code >= (unsigned long)_sinittext && - iter->code < (unsigned long)_einittext) + if (init_kernel_text(iter->code)) iter->code = 0; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] extable: Make init_kernel_text() global 2018-02-16 17:57 ` [PATCH] extable: Make init_kernel_text() global Josh Poimboeuf @ 2018-02-16 18:03 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2018-02-16 18:03 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jason Baron, x86, linux-kernel, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Borislav Petkov On Fri, 16 Feb 2018 11:57:44 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Fri, Feb 16, 2018 at 11:55:54AM -0500, Jason Baron wrote: > > > +/* Disable any jump label entries in __init code */ > > > +void __init jump_label_invalidate_init(void) > > > +{ > > > + struct jump_entry *iter_start = __start___jump_table; > > > + struct jump_entry *iter_stop = __stop___jump_table; > > > + struct jump_entry *iter; > > > + > > > + for (iter = iter_start; iter < iter_stop; iter++) > > > + if (iter->code >= (unsigned long)_sinittext && > > > + iter->code < (unsigned long)_einittext) > > > + iter->code = 0; > > > +} > > > > Seems like this wants to use init_kernel_text() but i see its marked > > 'static', perhaps it can be moved to a header? > > How about this patch on top? Yes please. > > --------- > > From: Josh Poimboeuf <jpoimboe@redhat.com> > Subject: [PATCH] extable: Make init_kernel_text() global > > Convert init_kernel_text() to a global function and use it in a few > places instead of manually comparing _sinittext and _einittext. > > Note that kallsyms.h has a very similar function called > is_kernel_inittext(), but its end check is inclusive. I'm not sure > whether that's intentional behavior, so I didn't touch it. > > Suggested-by: Jason Baron <jbaron@akamai.com> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > --- Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-16 16:31 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Josh Poimboeuf 2018-02-16 16:55 ` Jason Baron @ 2018-02-17 10:38 ` Ingo Molnar 2018-02-17 13:40 ` Josh Poimboeuf 1 sibling, 1 reply; 11+ messages in thread From: Ingo Molnar @ 2018-02-17 10:38 UTC (permalink / raw) To: Josh Poimboeuf Cc: x86, linux-kernel, Steven Rostedt, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > +/* Disable any jump label entries in __init code */ > +void __init jump_label_invalidate_init(void) > +{ > + struct jump_entry *iter_start = __start___jump_table; > + struct jump_entry *iter_stop = __stop___jump_table; > + struct jump_entry *iter; > + > + for (iter = iter_start; iter < iter_stop; iter++) > + if (iter->code >= (unsigned long)_sinittext && > + iter->code < (unsigned long)_einittext) > + iter->code = 0; > +} > + > +/* Disable any jump label entries in module init code */ > static void jump_label_invalidate_module_init(struct module *mod) > { > struct jump_entry *iter_start = mod->jump_entries; > struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; > struct jump_entry *iter; > > - for (iter = iter_start; iter < iter_stop; iter++) { > + for (iter = iter_start; iter < iter_stop; iter++) > if (within_module_init(iter->code, mod)) > iter->code = 0; > - } Why did you remove the curly braces? They are canonical kernel style for multi-line statements. The new jump_label_invalidate_init() function has that problem too. Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-17 10:38 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Ingo Molnar @ 2018-02-17 13:40 ` Josh Poimboeuf 2018-02-17 20:13 ` Thomas Gleixner 0 siblings, 1 reply; 11+ messages in thread From: Josh Poimboeuf @ 2018-02-17 13:40 UTC (permalink / raw) To: Ingo Molnar Cc: x86, linux-kernel, Steven Rostedt, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov On Sat, Feb 17, 2018 at 11:38:48AM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > +/* Disable any jump label entries in __init code */ > > +void __init jump_label_invalidate_init(void) > > +{ > > + struct jump_entry *iter_start = __start___jump_table; > > + struct jump_entry *iter_stop = __stop___jump_table; > > + struct jump_entry *iter; > > + > > + for (iter = iter_start; iter < iter_stop; iter++) > > + if (iter->code >= (unsigned long)_sinittext && > > + iter->code < (unsigned long)_einittext) > > + iter->code = 0; > > +} > > + > > +/* Disable any jump label entries in module init code */ > > static void jump_label_invalidate_module_init(struct module *mod) > > { > > struct jump_entry *iter_start = mod->jump_entries; > > struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; > > struct jump_entry *iter; > > > > - for (iter = iter_start; iter < iter_stop; iter++) { > > + for (iter = iter_start; iter < iter_stop; iter++) > > if (within_module_init(iter->code, mod)) > > iter->code = 0; > > - } > > Why did you remove the curly braces? They are canonical kernel style for > multi-line statements. > > The new jump_label_invalidate_init() function has that problem too. I'd say the jury is still out: Without braces: $ find . -name "*.[ch]" | xargs awk '/for \(.*[^{]$/ { line1=$0; f=1; next } f == 1 && /if \(.*[^{]$/ { f=0; line2=$0; i=1; next } i == 1 { i=0; line3=$0; j=1; next } j == 1 && /^$/ {j=0; print line1; print line2; print line3; print; next} { f=0; i=0; j=0; }' |grep 'for (' |wc -l 1389 With braces: $ find . -name "*.[ch]" | xargs awk '/for \(.*{$/ { line1=$0; f=1; next } f == 1 && /if \(.*[^{]$/ { f=0; line2=$0; i=1; next } i == 1 { i=0; line3=$0; j=1; next } j == 1 && /}/ {j=0; print line1; print line2; print line3; print } { f=0; i=0; j=0; }' |grep 'for (' |wc -l 2292 And I see no mention of it in coding-style.rst. Personally I prefer the more compact version, but I have no problem changing it. -- Josh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-17 13:40 ` Josh Poimboeuf @ 2018-02-17 20:13 ` Thomas Gleixner 2018-02-18 13:05 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Thomas Gleixner @ 2018-02-17 20:13 UTC (permalink / raw) To: Josh Poimboeuf Cc: Ingo Molnar, x86, linux-kernel, Steven Rostedt, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov On Sat, 17 Feb 2018, Josh Poimboeuf wrote: > On Sat, Feb 17, 2018 at 11:38:48AM +0100, Ingo Molnar wrote: > > > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > +/* Disable any jump label entries in __init code */ > > > +void __init jump_label_invalidate_init(void) > > > +{ > > > + struct jump_entry *iter_start = __start___jump_table; > > > + struct jump_entry *iter_stop = __stop___jump_table; > > > + struct jump_entry *iter; > > > + > > > + for (iter = iter_start; iter < iter_stop; iter++) > > > + if (iter->code >= (unsigned long)_sinittext && > > > + iter->code < (unsigned long)_einittext) > > > + iter->code = 0; > > > +} > > > + > > > +/* Disable any jump label entries in module init code */ > > > static void jump_label_invalidate_module_init(struct module *mod) > > > { > > > struct jump_entry *iter_start = mod->jump_entries; > > > struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; > > > struct jump_entry *iter; > > > > > > - for (iter = iter_start; iter < iter_stop; iter++) { > > > + for (iter = iter_start; iter < iter_stop; iter++) > > > if (within_module_init(iter->code, mod)) > > > iter->code = 0; > > > - } > > > > Why did you remove the curly braces? They are canonical kernel style for > > multi-line statements. > > Personally I prefer the more compact version, but I have no problem > changing it. Yes, it's certainly a matter of taste. Here is the reason why myself and others prefer the version with braces: https://marc.info/?l=linux-kernel&m=148467980905537&w=2 Thanks, tglx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-17 20:13 ` Thomas Gleixner @ 2018-02-18 13:05 ` Ingo Molnar 2018-02-18 13:15 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Ingo Molnar @ 2018-02-18 13:05 UTC (permalink / raw) To: Thomas Gleixner Cc: Josh Poimboeuf, x86, linux-kernel, Steven Rostedt, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov * Thomas Gleixner <tglx@linutronix.de> wrote: > On Sat, 17 Feb 2018, Josh Poimboeuf wrote: > > On Sat, Feb 17, 2018 at 11:38:48AM +0100, Ingo Molnar wrote: > > > > > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > > +/* Disable any jump label entries in __init code */ > > > > +void __init jump_label_invalidate_init(void) > > > > +{ > > > > + struct jump_entry *iter_start = __start___jump_table; > > > > + struct jump_entry *iter_stop = __stop___jump_table; > > > > + struct jump_entry *iter; > > > > + > > > > + for (iter = iter_start; iter < iter_stop; iter++) > > > > + if (iter->code >= (unsigned long)_sinittext && > > > > + iter->code < (unsigned long)_einittext) > > > > + iter->code = 0; > > > > +} > > > > + > > > > +/* Disable any jump label entries in module init code */ > > > > static void jump_label_invalidate_module_init(struct module *mod) > > > > { > > > > struct jump_entry *iter_start = mod->jump_entries; > > > > struct jump_entry *iter_stop = iter_start + mod->num_jump_entries; > > > > struct jump_entry *iter; > > > > > > > > - for (iter = iter_start; iter < iter_stop; iter++) { > > > > + for (iter = iter_start; iter < iter_stop; iter++) > > > > if (within_module_init(iter->code, mod)) > > > > iter->code = 0; > > > > - } > > > > > > Why did you remove the curly braces? They are canonical kernel style for > > > multi-line statements. > > > > Personally I prefer the more compact version, but I have no problem > > changing it. > > Yes, it's certainly a matter of taste. Here is the reason why myself and > others prefer the version with braces: > > https://marc.info/?l=linux-kernel&m=148467980905537&w=2 Easier visual parsing is indeed one of the primary reasons, but there's two other reasons as well: 2) code robustness For example: for (i = 0; i < 10; i++) if (foo) bar(i); baz(i); Is probably buggy code, although technically it's valid syntax and will compile just fine. If all multi-line statements have curly braces then this type of bug cannot occur: for (i = 0; i < 10; i++) { if (foo) bar(i); baz(i); } 3) style consistency Nothing is worse than randomly inconsistent coding style, and in arch/x86/ and core kernel code using curly braces is certainly the dominant style: # multi-line C statements without braces: $ find arch/x86/ kernel mm -name "*.[ch]" | xargs awk '/for \(.*[^{]$/ { line1=$0; f=1; next } f == 1 && /if \(.*[^{]$/ { f=0; line2=$0; i=1; next } i == 1 { i=0; line3=$0; j=1; next } j == 1 && /^$/{j=0; print line1; print line2; print line3; print; next} { f=0; i=0; j=0; }' |grep 'for (' |wc -l 55 # multi-line C statements with braces: $ find arch/x86 kernel mm -name "*.[ch]" | xargs awk '/for \(.*{$/ { line1=$0; f=1; next } f == 1 && /if \(.*[^{]$/ { f=0; line2=$0; i=1; next } i == 1 { i=0; line3=$0; j=1; next } j == 1 && /}/{j=0; print line1; print line2; print line3; print } { f=0; i=0; j=0; }' |grep 'for (' |wc -l 116 Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code 2018-02-18 13:05 ` Ingo Molnar @ 2018-02-18 13:15 ` Ingo Molnar 0 siblings, 0 replies; 11+ messages in thread From: Ingo Molnar @ 2018-02-18 13:15 UTC (permalink / raw) To: Thomas Gleixner Cc: Josh Poimboeuf, x86, linux-kernel, Steven Rostedt, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov * Ingo Molnar <mingo@kernel.org> wrote: > 2) code robustness > > For example: > > for (i = 0; i < 10; i++) > if (foo) > bar(i); > baz(i); > > Is probably buggy code, although technically it's valid syntax and will compile > just fine. > > If all multi-line statements have curly braces then this type of bug cannot occur: > > for (i = 0; i < 10; i++) { > if (foo) > bar(i); > baz(i); > } Note that newer versions of GCC will warn about this pattern: warning: this ‘for’ clause does not guard this statement, but the latter is misleadingly indented as if it were guarded by the ‘for’ But the warning is pretty restrictive and GCC won't warn about slightly more complex patterns like: for (i = 0; i < 10; i++) if (foo) bar(i); // debug_fn(); baz(i); Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] jump_label: Warn on failed jump_label patch 2018-02-16 16:31 [PATCH v2 0/2] jump_label: Robustify jump label patching Josh Poimboeuf 2018-02-16 16:31 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Josh Poimboeuf @ 2018-02-16 16:31 ` Josh Poimboeuf 1 sibling, 0 replies; 11+ messages in thread From: Josh Poimboeuf @ 2018-02-16 16:31 UTC (permalink / raw) To: x86 Cc: linux-kernel, Steven Rostedt, Ingo Molnar, Thomas Gleixner, Linus Torvalds, Peter Zijlstra, Jason Baron, Borislav Petkov When the jump label code encounters an address which isn't recognized by kernel_text_address(), it just silently fails. This can be dangerous because jump labels are used in a variety of places, and are generally expected to work. Convert the silent failure to a warning. This won't warn about attempted writes to tracepoints in __init code after initmem has been freed, as those are already guarded by the entry->code check. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- kernel/jump_label.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/kernel/jump_label.c b/kernel/jump_label.c index 96274c6d3511..08a02ae54997 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -367,12 +367,15 @@ static void __jump_label_update(struct static_key *key, { for (; (entry < stop) && (jump_entry_key(entry) == key); entry++) { /* - * entry->code set to 0 invalidates module init text sections - * kernel_text_address() verifies we are not in core kernel - * init code, see jump_label_invalidate_module_init(). + * An entry->code of 0 indicates an entry which has been + * disabled because it was in an init text area. */ - if (entry->code && kernel_text_address(entry->code)) - arch_jump_label_transform(entry, jump_label_type(entry)); + if (entry->code) { + if (kernel_text_address(entry->code)) + arch_jump_label_transform(entry, jump_label_type(entry)); + else + WARN_ONCE(1, "can't patch jump_label at %pS", (void *)entry->code); + } } } -- 2.14.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-18 13:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-16 16:31 [PATCH v2 0/2] jump_label: Robustify jump label patching Josh Poimboeuf 2018-02-16 16:31 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Josh Poimboeuf 2018-02-16 16:55 ` Jason Baron 2018-02-16 17:57 ` [PATCH] extable: Make init_kernel_text() global Josh Poimboeuf 2018-02-16 18:03 ` Steven Rostedt 2018-02-17 10:38 ` [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code Ingo Molnar 2018-02-17 13:40 ` Josh Poimboeuf 2018-02-17 20:13 ` Thomas Gleixner 2018-02-18 13:05 ` Ingo Molnar 2018-02-18 13:15 ` Ingo Molnar 2018-02-16 16:31 ` [PATCH v2 2/2] jump_label: Warn on failed jump_label patch Josh Poimboeuf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.