[PATCH v2 0/2] jump_label: Robustify jump label patching

[PATCH v2 0/2] jump_label: Robustify jump label patching

[Josh Poimboeuf]

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

[PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Josh Poimboeuf]

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

[PATCH v2 2/2] jump_label: Warn on failed jump_label patch

[Josh Poimboeuf]

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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Jason Baron]

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
> 

[PATCH] extable: Make init_kernel_text() global

[Josh Poimboeuf]

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

Re: [PATCH] extable: Make init_kernel_text() global

[Steven Rostedt]

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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Ingo Molnar]

* 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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Josh Poimboeuf]

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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Thomas Gleixner]

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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Ingo Molnar]

* 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

Re: [PATCH v2 1/2] jump_label: Explicitly disable jump labels in __init code

[Ingo Molnar]

* 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