[PATCH 0/2] Avoid crashes by early (boot) consoles using init memory

[PATCH 0/2] Avoid crashes by early (boot) consoles using init memory

[Petr Mladek]

Some early consoles have code and data in the init section. It makes some
sense but this might cause problems when they are not replaced by
the real console in time. The two patches fix the safequard and
help to avoid the problems.

I though about removing keep_bootcon option completely. But it is useful
at least for now. There is not an easy way to disable a particular bootconsole
when the related real console is installed. Instead all bootconsoles are
removed when the preferred (last on the commandline) console is registered.
But this is a bit cumbersome and non-intuitive.

In addition, the same problems might happen when the real console is
registered using a deferred probe or when it is not registered at all
for some reason.

The patchset is based on and inspired by the original patch from
Matt Redfearn, see the discussion starting by the mail
https://lkml.kernel.org/r/1499337481-19397-1-git-send-email-matt.redfearn@imgtec.com

Matt Redfearn (1):
  printk/console: Always disable boot consoles that use init memory
    before it is freed

Petr Mladek (1):
  printk/console: Enhance the check for consoles using init memory

 kernel/printk/printk.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

-- 
1.8.5.6

[PATCH 1/2] printk/console: Always disable boot consoles that use init memory before it is freed

[Petr Mladek]

From: Matt Redfearn <matt.redfearn@imgtec.com>

Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
printk_late_init() if keep_bootcon") added a check on keep_bootcon to
ensure that boot consoles were kept around until the real console is
registered.

This can lead to problems if the boot console data and code are in the
init section, since it can be freed before the boot console is
unregistered.

Commit 81cc26f2bd11 ("printk: only unregister boot consoles when
necessary") fixed this a better way. It allowed to keep boot consoles
that did not use init data. Unfortunately it did not remove the check
of keep_bootcon.

This can lead to crashes and weird panics when the bootconsole is
accessed after free, especially if page poisoning is in use and the
code / data have been overwritten with a poison value.

To prevent this, always free the boot console if it is within the init
section. In addition, print a warning about that the console is removed
prematurely.

Finally there is a new comment how to avoid the warning. It replaced
an explanation that duplicated a more comprehensive function
description few lines above.

Fixes: 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in printk_late_init() if keep_bootcon")
Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
[pmladek@suse.com: print the warning, code and comments clean up]
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863f629c..f35d3ac3b8c7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2650,9 +2650,8 @@ void __init console_init(void)
  * makes it difficult to diagnose problems that occur during this time.
  *
  * To mitigate this problem somewhat, only unregister consoles whose memory
- * intersects with the init section. Note that code exists elsewhere to get
- * rid of the boot console as soon as the proper console shows up, so there
- * won't be side-effects from postponing the removal.
+ * intersects with the init section. Note that all other boot consoles will
+ * get unregistred when the real preferred console is registered.
  */
 static int __init printk_late_init(void)
 {
@@ -2660,16 +2659,15 @@ static int __init printk_late_init(void)
 	int ret;
 
 	for_each_console(con) {
-		if (!keep_bootcon && con->flags & CON_BOOT) {
+		if ((con->flags & CON_BOOT) &&
+		    init_section_intersects(con, sizeof(*con))) {
 			/*
-			 * Make sure to unregister boot consoles whose data
-			 * resides in the init section before the init section
-			 * is discarded. Boot consoles whose data will stick
-			 * around will automatically be unregistered when the
-			 * proper console replaces them.
+			 * Please, consider moving the reported consoles out
+			 * of the init section.
 			 */
-			if (init_section_intersects(con, sizeof(*con)))
-				unregister_console(con);
+			pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
+				con->name, con->index);
+			unregister_console(con);
 		}
 	}
 	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
-- 
1.8.5.6

Re: [PATCH 1/2] printk/console: Always disable boot consoles that use init memory before it is freed

[Sergey Senozhatsky]

On (07/14/17 14:51), Petr Mladek wrote:
> From: Matt Redfearn <matt.redfearn@imgtec.com>
> 
> Commit 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in
> printk_late_init() if keep_bootcon") added a check on keep_bootcon to
> ensure that boot consoles were kept around until the real console is
> registered.
> 
> This can lead to problems if the boot console data and code are in the
> init section, since it can be freed before the boot console is
> unregistered.
> 
> Commit 81cc26f2bd11 ("printk: only unregister boot consoles when
> necessary") fixed this a better way. It allowed to keep boot consoles
> that did not use init data. Unfortunately it did not remove the check
> of keep_bootcon.
> 
> This can lead to crashes and weird panics when the bootconsole is
> accessed after free, especially if page poisoning is in use and the
> code / data have been overwritten with a poison value.
> 
> To prevent this, always free the boot console if it is within the init
> section. In addition, print a warning about that the console is removed
> prematurely.
> 
> Finally there is a new comment how to avoid the warning. It replaced
> an explanation that duplicated a more comprehensive function
> description few lines above.
> 
> Fixes: 4c30c6f566c0 ("kernel/printk: do not turn off bootconsole in printk_late_init() if keep_bootcon")
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
> [pmladek@suse.com: print the warning, code and comments clean up]
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

[PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Petr Mladek]

printk_late_init() is responsible for disabling boot consoles that
use init memory. It checks the address of struct console for this.

But this is not enough. For example, there are several early
consoles that have write() method in the init section and
struct console in the normal section. They are not disabled
and could cause fancy and hard to debug system states.

It is even more complicated by the macros EARLYCON_DECLARE() and
OF_EARLYCON_DECLARE() where various struct members are set at
runtime by the provided setup() function.

I have tried to reproduce this problem and forced the classic uart
early console to stay using keep_bootcon parameter. In particular
I used earlycon=uart,io,0x3f8 keep_bootcon console=ttyS0,115200.
The system did not boot:

[    1.570496] PM: Image not found (code -22)
[    1.570496] PM: Image not found (code -22)
[    1.571886] PM: Hibernation image not present or could not be loaded.
[    1.571886] PM: Hibernation image not present or could not be loaded.
[    1.576407] Freeing unused kernel memory: 2528K
[    1.577244] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)

The double lines are caused by having both early uart console and
ttyS0 console enabled at the same time. The early console stopped
working when the init memory was freed. Fortunately, the invalid
call was caught by the NX-protexted page check and did not cause
any silent fancy problems.

This patch adds a check for many other addresses stored in
struct console. It omits setup() and match() that are used
only when the console is registered. Therefore they have
already been used at this point and there is no reason
to use them again.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f35d3ac3b8c7..1ebe1525ef64 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2659,8 +2659,16 @@ static int __init printk_late_init(void)
 	int ret;
 
 	for_each_console(con) {
-		if ((con->flags & CON_BOOT) &&
-		    init_section_intersects(con, sizeof(*con))) {
+		if (!(con->flags & CON_BOOT))
+			continue;
+
+		/* Check addresses that might be used for enabled consoles. */
+		if (init_section_intersects(con, sizeof(*con)) ||
+		    init_section_contains(con->write, 0) ||
+		    init_section_contains(con->read, 0) ||
+		    init_section_contains(con->device, 0) ||
+		    init_section_contains(con->unblank, 0) ||
+		    init_section_contains(con->data, 0)) {
 			/*
 			 * Please, consider moving the reported consoles out
 			 * of the init section.
-- 
1.8.5.6

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Sergey Senozhatsky]

On (07/14/17 14:51), Petr Mladek wrote:
[..]
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index f35d3ac3b8c7..1ebe1525ef64 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2659,8 +2659,16 @@ static int __init printk_late_init(void)
>  	int ret;
>  
>  	for_each_console(con) {
> -		if ((con->flags & CON_BOOT) &&
> -		    init_section_intersects(con, sizeof(*con))) {
> +		if (!(con->flags & CON_BOOT))
> +			continue;
> +
> +		/* Check addresses that might be used for enabled consoles. */
> +		if (init_section_intersects(con, sizeof(*con)) ||
> +		    init_section_contains(con->write, 0) ||
> +		    init_section_contains(con->read, 0) ||
> +		    init_section_contains(con->device, 0) ||
> +		    init_section_contains(con->unblank, 0) ||
> +		    init_section_contains(con->data, 0)) {

sort of a problem here is that the next time anyone adds a new ->foo()
callback to struct console, that person also needs to remember to update
printk_late_init().


a completely crazy idea,
can we have a dedicated "console init" section which we will not offload
if we see keep_bootcon?

or... even crazier... disable bootmem offloading (do not offload init
section) at all if we see keep_bootcon? keep_bootcon is a purely debugging
option which people enable when things are bad and unclear, no one should
be using it otherwise, so may be that idea can be a way to go.

thoughts?

	-ss

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Petr Mladek]

On Sat 2017-07-15 07:06:26, Sergey Senozhatsky wrote:
> On (07/14/17 14:51), Petr Mladek wrote:
> [..]
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index f35d3ac3b8c7..1ebe1525ef64 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2659,8 +2659,16 @@ static int __init printk_late_init(void)
> >  	int ret;
> >  
> >  	for_each_console(con) {
> > -		if ((con->flags & CON_BOOT) &&
> > -		    init_section_intersects(con, sizeof(*con))) {
> > +		if (!(con->flags & CON_BOOT))
> > +			continue;
> > +
> > +		/* Check addresses that might be used for enabled consoles. */
> > +		if (init_section_intersects(con, sizeof(*con)) ||
> > +		    init_section_contains(con->write, 0) ||
> > +		    init_section_contains(con->read, 0) ||
> > +		    init_section_contains(con->device, 0) ||
> > +		    init_section_contains(con->unblank, 0) ||
> > +		    init_section_contains(con->data, 0)) {
> 
> sort of a problem here is that the next time anyone adds a new ->foo()
> callback to struct console, that person also needs to remember to update
> printk_late_init().

I am not super happy with this as well. Any hint how to do it better
or more secure is welcome. But I do not see a beter solution at the moment.

Note that there are only 3 commits in the git history that change this
structure. Neither of them invalidates this check!

Just for record. The commits are:

  + c7cef0a84912cab3 ("console: Add extensible console matching")
      + Mar 9 2015
      + replaced .early_setup with .match

  + 18a8bd949d6adb31 ("serial: convert early_uart to earlycon
    for 8250")
	+ Jul 15 2007 (10 years ago)!
	+ added .early_setup

  + 6ae9200f2cab7b32 ("enlarge console.name")
	+ May 8 2007
	+ resized .name[8] -> .name[16]


> a completely crazy idea,
> can we have a dedicated "console init" section which we will not offload
> if we see keep_bootcon?

I though about this as well. But this will not avoid the above
problem. We still would need to make sure that the consoles
use the special section. Or do I miss anything?

Also note that only few early consoles actually use the init section
and are affected by this problem. Well, I did only a rough grepping.
It is not perfect but it might give a picture:

# struct console location

$> git grep "struct console.*{" | grep early | wc -l
23
$> git grep "struct console.*{" | grep early | grep init | wc -l
5

# write() function location of statically defined struct consoles

$> for func in `git grep -A 6 "struct console.*{" | grep write |
        cut -d= -f2 | cut -d',' -f1 | grep -v void` ;do 
    git grep "void.*$func(struct " ; done | grep early | wc -l
83

$> for func in `git grep -A 6 "struct console.*{" | grep write | 
        cut -d= -f2 | cut -d',' -f1 | grep -v void` ;
   do git grep "void.*$func(struct " ; done | grep early | grep init | wc -l
12

# write method location of struct consoles defined by OF_EARLYCON_DECLARE()

$> for func in `git grep 'con->write =' | cut -d= -f2 | cut -d';' -f1`
     ; do git grep "void.*$func(struct " ; done | grep early wc -l
26

$> for func in `git grep 'con->write =' | cut -d= -f2 | cut -d';' -f1`
     ; do git grep "void.*$func(struct " ; done | grep early | 
       grep init | wc -l
7


It means that less than 25% of early consoles are located in the init
code. I am not sure if it is worth introducing a new section.

Instead it would make sense to move all these consoles into the normal
section. But it is not strictly needed if the normal console is
registered using an init call (always in time). In this case, it is "enough"
to mention the real console as the last one on the command line.


> or... even crazier... disable bootmem offloading (do not offload init
> section) at all if we see keep_bootcon? keep_bootcon is a purely debugging
> option which people enable when things are bad and unclear, no one should
> be using it otherwise, so may be that idea can be a way to go.

I have talked about this with my colleagues. They told me that it
would be pity. The keep_bootcon option might be useful to debug
problems related to freeing the init memory.

In addition, it will not help to avoid the controversal check above.
We need to remove early icons also when the preferred console is
registered too late because of a deferred probe. Or when the preferred
console is not registred at all from some reasons. I mean that problematic
early consoles might need to be forcibly removed even without
keep_bootcon option.

I still vote for the proposed patch even though it is not perfect.
IMHO, "The perfect is the enemy of the good" fits here.

Best Regards,
Petr

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Sergey Senozhatsky]

Hello,

On (07/21/17 16:32), Petr Mladek wrote:
[..]
> > sort of a problem here is that the next time anyone adds a new ->foo()
> > callback to struct console, that person also needs to remember to update
> > printk_late_init().
> 
> I am not super happy with this as well. Any hint how to do it better
> or more secure is welcome. But I do not see a beter solution at the moment.
> 
> Note that there are only 3 commits in the git history that change this
> structure. Neither of them invalidates this check!

well, the console output is far from perfect, so I can imagine future
changes ;)

> > a completely crazy idea,
> > can we have a dedicated "console init" section which we will not offload
> > if we see keep_bootcon?
> 
> I though about this as well. But this will not avoid the above
> problem. We still would need to make sure that the consoles
> use the special section. Or do I miss anything?

you don't miss anything.

to fix the rootcause of the problem, and not its aftershock, we still
need to either:
	a) move consoles to normal section
or
	b) move consoles to a special section

I don't mind that warning, but I think we also need to tweak the
affected consoles. otherwise, upon maintainer's request to keep
bootcon, a user can just report back "uses init memory and must
be disabled even before the real one is ready" warning, yet the
kernel still would crash (a theoretical case, but for some reason
someone wanted to keep bootcon after all).

[..]
> It means that less than 25% of early consoles are located in the init
> code. I am not sure if it is worth introducing a new section.

ok, good.

> Instead it would make sense to move all these consoles into the normal
> section. But it is not strictly needed if the normal console is
> registered using an init call (always in time). In this case, it is "enough"
> to mention the real console as the last one on the command line.

let's move. to normal section, or to special section. depending on how
much space we can saved unloading the consoles.

> > or... even crazier... disable bootmem offloading (do not offload init
> > section) at all if we see keep_bootcon? keep_bootcon is a purely debugging
> > option which people enable when things are bad and unclear, no one should
> > be using it otherwise, so may be that idea can be a way to go.
> 
> I have talked about this with my colleagues. They told me that it
> would be pity. The keep_bootcon option might be useful to debug
> problems related to freeing the init memory.

agree.

	-ss

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Petr Mladek]

On Mon 2017-07-24 11:03:56, Sergey Senozhatsky wrote:
> Hello,
> 
> On (07/21/17 16:32), Petr Mladek wrote:
> [..]
> > > sort of a problem here is that the next time anyone adds a new ->foo()
> > > callback to struct console, that person also needs to remember to update
> > > printk_late_init().
> > 
> > I am not super happy with this as well. Any hint how to do it better
> > or more secure is welcome. But I do not see a beter solution at the moment.
> > 
> > Note that there are only 3 commits in the git history that change this
> > structure. Neither of them invalidates this check!
> 
> well, the console output is far from perfect, so I can imagine future
> changes ;)

Sure and we will need to deal with it. Anyway, I still thing that this
check is better than nothing. Even if we "fix" all consoles and move
them out of init section then this check will be useful to catch at least
some future mistakes.


> to fix the rootcause of the problem, and not its aftershock, we still
> need to either:
> 	a) move consoles to normal section
> or
> 	b) move consoles to a special section
> 
> I don't mind that warning, but I think we also need to tweak the
> affected consoles. otherwise, upon maintainer's request to keep
> bootcon, a user can just report back "uses init memory and must
> be disabled even before the real one is ready" warning, yet the
> kernel still would crash (a theoretical case, but for some reason
> someone wanted to keep bootcon after all).

I would leave this to the maintainers and developers of the respective
architectures. It would be nice to fix it now but I am not sure if
I would be able to do and test the changes properly and effectively.

Also it might be hard to sell. Note that it makes sense to keep early
con in the init section when the related real console is registered
during kernel initialization (no deferred probe) before late init
calls.


> > Instead it would make sense to move all these consoles into the normal
> > section. But it is not strictly needed if the normal console is
> > registered using an init call (always in time). In this case, it is "enough"
> > to mention the real console as the last one on the command line.
> 
> let's move. to normal section, or to special section. depending on how
> much space we can saved unloading the consoles.

I agree. We will do or suggest this when anyone see the warning
and ask for help.

Best Regards,
Petr

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Sergey Senozhatsky]

On (07/27/17 11:28), Petr Mladek wrote:
> > well, the console output is far from perfect, so I can imagine future
> > changes ;)
> 
> Sure and we will need to deal with it. Anyway, I still thing that this
> check is better than nothing. Even if we "fix" all consoles and move
> them out of init section then this check will be useful to catch at least
> some future mistakes.

yep, let's keep it.

[..]
> > let's move. to normal section, or to special section. depending on how
> > much space we can saved unloading the consoles.
> 
> I agree. We will do or suggest this when anyone see the warning
> and ask for help.

works for me.

	-ss

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Sergey Senozhatsky]

On (07/14/17 14:51), Petr Mladek wrote:
> printk_late_init() is responsible for disabling boot consoles that
> use init memory. It checks the address of struct console for this.
> 
> But this is not enough. For example, there are several early
> consoles that have write() method in the init section and
> struct console in the normal section. They are not disabled
> and could cause fancy and hard to debug system states.
> 
> It is even more complicated by the macros EARLYCON_DECLARE() and
> OF_EARLYCON_DECLARE() where various struct members are set at
> runtime by the provided setup() function.
> 
> I have tried to reproduce this problem and forced the classic uart
> early console to stay using keep_bootcon parameter. In particular
> I used earlycon=uart,io,0x3f8 keep_bootcon console=ttyS0,115200.
> The system did not boot:
> 
> [    1.570496] PM: Image not found (code -22)
> [    1.570496] PM: Image not found (code -22)
> [    1.571886] PM: Hibernation image not present or could not be loaded.
> [    1.571886] PM: Hibernation image not present or could not be loaded.
> [    1.576407] Freeing unused kernel memory: 2528K
> [    1.577244] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
> 
> The double lines are caused by having both early uart console and
> ttyS0 console enabled at the same time. The early console stopped
> working when the init memory was freed. Fortunately, the invalid
> call was caught by the NX-protexted page check and did not cause
> any silent fancy problems.
> 
> This patch adds a check for many other addresses stored in
> struct console. It omits setup() and match() that are used
> only when the console is registered. Therefore they have
> already been used at this point and there is no reason
> to use them again.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Petr Mladek]

On Wed 2017-07-26 22:08:04, Sergey Senozhatsky wrote:
> On (07/14/17 14:51), Petr Mladek wrote:
> > printk_late_init() is responsible for disabling boot consoles that
> > use init memory. It checks the address of struct console for this.
> > 
> > But this is not enough. For example, there are several early
> > consoles that have write() method in the init section and
> > struct console in the normal section. They are not disabled
> > and could cause fancy and hard to debug system states.
> > 
> > It is even more complicated by the macros EARLYCON_DECLARE() and
> > OF_EARLYCON_DECLARE() where various struct members are set at
> > runtime by the provided setup() function.
> > 
> > I have tried to reproduce this problem and forced the classic uart
> > early console to stay using keep_bootcon parameter. In particular
> > I used earlycon=uart,io,0x3f8 keep_bootcon console=ttyS0,115200.
> > The system did not boot:
> > 
> > [    1.570496] PM: Image not found (code -22)
> > [    1.570496] PM: Image not found (code -22)
> > [    1.571886] PM: Hibernation image not present or could not be loaded.
> > [    1.571886] PM: Hibernation image not present or could not be loaded.
> > [    1.576407] Freeing unused kernel memory: 2528K
> > [    1.577244] kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
> > 
> > The double lines are caused by having both early uart console and
> > ttyS0 console enabled at the same time. The early console stopped
> > working when the init memory was freed. Fortunately, the invalid
> > call was caught by the NX-protexted page check and did not cause
> > any silent fancy problems.
> > 
> > This patch adds a check for many other addresses stored in
> > struct console. It omits setup() and match() that are used
> > only when the console is registered. Therefore they have
> > already been used at this point and there is no reason
> > to use them again.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> 
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Thanks for the review. I am going to push the two patches into
for-4.14 branch so that we could get some testing via linux-next.

Best Regards,
Petr

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Sergey Senozhatsky]

On (07/27/17 11:29), Petr Mladek wrote:
[..]
> > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > 
> > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> Thanks for the review. I am going to push the two patches into
> for-4.14 branch so that we could get some testing via linux-next.

good. agree.

how do you think,

would pr_warn() be enough for people to notice the wrongdoing or
shall we put WARN_ON() there for a while at least? (assuming that
people pay more attention to backtraces).

	-ss

Re: [PATCH 2/2] printk/console: Enhance the check for consoles using init memory

[Petr Mladek]

On Thu 2017-07-27 18:51:01, Sergey Senozhatsky wrote:
> On (07/27/17 11:29), Petr Mladek wrote:
> [..]
> > > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > > 
> > > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > 
> > Thanks for the review. I am going to push the two patches into
> > for-4.14 branch so that we could get some testing via linux-next.
> 
> good. agree.
> 
> how do you think,
> 
> would pr_warn() be enough for people to notice the wrongdoing or
> shall we put WARN_ON() there for a while at least? (assuming that
> people pay more attention to backtraces).

I would keep pr_warn(). The warning is interesting only when people
want to debug and the real console does not appear in time.
Then this will be the last but one message on the early console.

Best Regards,
Petr

Re: [PATCH 0/2] Avoid crashes by early (boot) consoles using init memory

[Fabio M. Di Nitto]

Hi Petr,

On 7/14/2017 2:51 PM, Petr Mladek wrote:
> Some early consoles have code and data in the init section. It makes some
> sense but this might cause problems when they are not replaced by
> the real console in time. The two patches fix the safequard and
> help to avoid the problems.
> 
> I though about removing keep_bootcon option completely. But it is useful
> at least for now.

Let´s just keep in mind that keep_bootcon was introduced only to debug
issues (read crashes or hangs) that could happen between disabling
bootconsole and enabling the real console. It shouldn´t be used for
anything else really.

If the new code can replace keep_bootcon, by all mean, go for it :-)

Cheers
Fabio

> There is not an easy way to disable a particular bootconsole
> when the related real console is installed. Instead all bootconsoles are
> removed when the preferred (last on the commandline) console is registered.
> But this is a bit cumbersome and non-intuitive.
> 
> In addition, the same problems might happen when the real console is
> registered using a deferred probe or when it is not registered at all
> for some reason.
> 
> The patchset is based on and inspired by the original patch from
> Matt Redfearn, see the discussion starting by the mail
> https://lkml.kernel.org/r/1499337481-19397-1-git-send-email-matt.redfearn@imgtec.com
> 
> Matt Redfearn (1):
>   printk/console: Always disable boot consoles that use init memory
>     before it is freed
> 
> Petr Mladek (1):
>   printk/console: Enhance the check for consoles using init memory
> 
>  kernel/printk/printk.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 

Re: [PATCH 0/2] Avoid crashes by early (boot) consoles using init memory

[Petr Mladek]

On Fri 2017-07-14 14:57:45, Fabio M. Di Nitto wrote:
> Hi Petr,
> 
> On 7/14/2017 2:51 PM, Petr Mladek wrote:
> > Some early consoles have code and data in the init section. It makes some
> > sense but this might cause problems when they are not replaced by
> > the real console in time. The two patches fix the safequard and
> > help to avoid the problems.
> > 
> > I though about removing keep_bootcon option completely. But it is useful
> > at least for now.
> 
> Let´s just keep in mind that keep_bootcon was introduced only to debug
> issues (read crashes or hangs) that could happen between disabling
> bootconsole and enabling the real console. It shouldn´t be used for
> anything else really.

This was my initial replay as well. But then I realized that
it was a bad idea to use a freed code and data to debug any other
issue. It would just create crazy issues on its own.

I tried to google 'keep_bootcon'. It found several links to strange
crashes related to this option. Maybe I was not patient enough but
I did not find any page where this option was suggested and helped.

I still think that the option makes some sense but only when
it does not cause more breakages on its own.

> If the new code can replace keep_bootcon, by all mean, go for it :-)

keep_bootcon stays usable for most early consoles. We print a warning
when an unusable console is disabled too early. Also there is a
comment how to fix it.

Best Regards,
Petr

Re: [PATCH 0/2] Avoid crashes by early (boot) consoles using init memory

[Fabio M. Di Nitto]

On 7/14/2017 4:37 PM, Petr Mladek wrote:
> On Fri 2017-07-14 14:57:45, Fabio M. Di Nitto wrote:
>> Hi Petr,
>>
>> On 7/14/2017 2:51 PM, Petr Mladek wrote:
>>> Some early consoles have code and data in the init section. It makes some
>>> sense but this might cause problems when they are not replaced by
>>> the real console in time. The two patches fix the safequard and
>>> help to avoid the problems.
>>>
>>> I though about removing keep_bootcon option completely. But it is useful
>>> at least for now.
>>
>> Let´s just keep in mind that keep_bootcon was introduced only to debug
>> issues (read crashes or hangs) that could happen between disabling
>> bootconsole and enabling the real console. It shouldn´t be used for
>> anything else really.
> 
> This was my initial replay as well. But then I realized that
> it was a bad idea to use a freed code and data to debug any other
> issue. It would just create crazy issues on its own.

Perhaps some changes that were made later in the code introduced this
behavior. I honestly didn´t check. It´s been just so long ago :-)

> 
> I tried to google 'keep_bootcon'. It found several links to strange
> crashes related to this option. Maybe I was not patient enough but
> I did not find any page where this option was suggested and helped.

It´s in Documentation/admin-guide/kernel-parameters.txt. It´s also rare
that consoles are not working or something is crashing in that small
window, so I am not entirely surprised that there are few to none
references of users using it.

> 
> I still think that the option makes some sense but only when
> it does not cause more breakages on its own.

agreed.

Cheers
Fabio

> 
>> If the new code can replace keep_bootcon, by all mean, go for it :-)
> 
> keep_bootcon stays usable for most early consoles. We print a warning
> when an unusable console is disabled too early. Also there is a
> comment how to fix it.
> 
> Best Regards,
> Petr
>