[PATCH 1/2] asm/sections: Add helpers to check for section data

[PATCH 1/2] asm/sections: Add helpers to check for section data

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add a helper to check if an object (given an address and a size) is part
of a section (given beginning and end addresses). For convenience, also
provide a helper that performs this check for __init data using the
__init_begin and __init_end limits.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/asm-generic/sections.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index b58fd667f87b..529909090cd6 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -63,4 +63,15 @@ static inline int arch_is_kernel_data(unsigned long addr)
 }
 #endif
 
+static inline bool section_contains(void *virt, size_t size, void *begin,
+				    void *end)
+{
+	return virt >= begin && virt + size <= end;
+}
+
+static inline bool init_section_contains(void *virt, size_t size)
+{
+	return section_contains(virt, size, __init_begin, __init_end);
+}
+
 #endif /* _ASM_GENERIC_SECTIONS_H_ */
-- 
2.5.0

[PATCH 2/2] printk: Only unregister boot consoles when necessary

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Boot consoles are typically replaced by proper consoles during the boot
process. This can be problematic if the boot console data is part of the
init section that is reclaimed late during boot. If the proper console
does not register before this point in time, the boot console will need
to be removed (so that the freed memory is not accessed), leaving the
system without output for some time.

There are various reasons why the proper console may not register early
enough, such as deferred probe or the driver being a loadable module. If
that happens, there is some amount of time where no console messages are
visible to the user, which in turn can mean that they won't see crashes
or other potentially useful information.

To avoid this situation, only remove the boot console when it resides in
the init section. Code exists to replace the boot console by the proper
console when it is registered, keeping a seamless transition between the
boot and proper consoles.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 kernel/printk/printk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b16f35487b67..6164b6f48930 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -48,6 +48,7 @@
 #include <linux/uio.h>
 
 #include <asm/uaccess.h>
+#include <asm-generic/sections.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/printk.h>
@@ -2661,7 +2662,8 @@ static int __init printk_late_init(void)
 
 	for_each_console(con) {
 		if (!keep_bootcon && con->flags & CON_BOOT) {
-			unregister_console(con);
+			if (init_section_contains(con, sizeof(*con)))
+				unregister_console(con);
 		}
 	}
 	hotcpu_notifier(console_cpu_notify, 0);
-- 
2.5.0

Re: [PATCH 1/2] asm/sections: Add helpers to check for section data

[Andrew Morton]

On Thu, 29 Oct 2015 09:26:06 +0100 Thierry Reding <thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
> 
> Add a helper to check if an object (given an address and a size) is part
> of a section (given beginning and end addresses). For convenience, also
> provide a helper that performs this check for __init data using the
> __init_begin and __init_end limits.
> 
> ...
>
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -63,4 +63,15 @@ static inline int arch_is_kernel_data(unsigned long addr)
>  }
>  #endif
>  
> +static inline bool section_contains(void *virt, size_t size, void *begin,
> +				    void *end)
> +{
> +	return virt >= begin && virt + size <= end;
> +}

This is ambiguous: does it return true if the object is wholly within
the region, or if it is only partially within the region.  A code
comment will help.  Something like

/*
 * Return true if the region described by [virt, virt+size) is wholly contained
 * within the region described by [begin, end).
 */


Which makes me wonder whether this function should in fact do
"partially within".  Shrug, doesn't matter much I guess.

Re: [PATCH 2/2] printk: Only unregister boot consoles when necessary

[Andrew Morton]

On Thu, 29 Oct 2015 09:26:07 +0100 Thierry Reding <thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
> 
> Boot consoles are typically replaced by proper consoles during the boot
> process. This can be problematic if the boot console data is part of the
> init section that is reclaimed late during boot. If the proper console
> does not register before this point in time, the boot console will need
> to be removed (so that the freed memory is not accessed), leaving the
> system without output for some time.
> 
> There are various reasons why the proper console may not register early
> enough, such as deferred probe or the driver being a loadable module. If
> that happens, there is some amount of time where no console messages are
> visible to the user, which in turn can mean that they won't see crashes
> or other potentially useful information.
> 
> To avoid this situation, only remove the boot console when it resides in
> the init section. Code exists to replace the boot console by the proper
> console when it is registered, keeping a seamless transition between the
> boot and proper consoles.

OK.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -48,6 +48,7 @@
>  #include <linux/uio.h>
>  
>  #include <asm/uaccess.h>
> +#include <asm-generic/sections.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/printk.h>
> @@ -2661,7 +2662,8 @@ static int __init printk_late_init(void)
>  
>  	for_each_console(con) {
>  		if (!keep_bootcon && con->flags & CON_BOOT) {
> -			unregister_console(con);
> +			if (init_section_contains(con, sizeof(*con)))
> +				unregister_console(con);
>  		}

It's a bit hacky.  It might be nicer to add a new CON_foo flag
specifying the treatment but a) there are waaay too many consoles in
the kernel and b) it's fragile to require every register_console()
caller to remember to do this, and to maintain the CON_foo as code
evolves.  So I guess we go with hacky.


There is no way in which a reader can work out why this code is here,
without forcing them to go off and wrestle with git-blame.  Can we
please add a comment to save them from this fate?