Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693752 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] printk/console: Enhance the check for consoles using  init memory Petr Mladek <pmladek@suse.com> - 2017-07-21 16:40 +0200
    Re: [PATCH 2/2] printk/console: Enhance the check for consoles using  init memory Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-07-24 04:10 +0200
      Re: [PATCH 2/2] printk/console: Enhance the check for consoles using  init memory Petr Mladek <pmladek@suse.com> - 2017-07-27 11:30 +0200
        Re: [PATCH 2/2] printk/console: Enhance the check for consoles using  init memory Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-07-27 12:00 +0200

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

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

[toc] | [next] | [standalone]

#1694401

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

[toc] | [prev] | [next] | [standalone]

#1697841

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

[toc] | [prev] | [next] | [standalone]

#1697855

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web