Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hello,
sorry, I'm on a sick leave and can't check emails that often.
On (07/11/17 14:43), Petr Mladek wrote:
[..]
> > >>The keep_bootcon flag prevents the unregistration of a boot console,
> > >>even if it's data and code reside in the init section and are about to
> > >>be freed. 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.
>
> > >if someone asked to `keep_bootcon' but we actually don't keep it, then
> > >what's the purpose of the flag and
> > > pr_info("debug: skip boot console de-registration.\n")?
>
> Exactly. The important information is in the commit 7bf693951a8e5f7e
> ("console: allow to retain boot console via boot option keep_bootcon"):
>
> On some architectures, the boot process involves de-registering the boot
> console (early boot), initialize drivers and then re-register the console.
>
> This mechanism introduces a window in which no printk can happen on the
> console and messages are buffered and then printed once the new console is
> available.
>
> If a kernel crashes during this window, all it's left on the boot console
> is "console [foo] enabled, boot console disabled" making debug of the
> crash
> rather 'interesting'.
sure. no objections here.
I probably mis-worded my thoughts. I agree with Matt's conclusions and
he made valid points I just disagree with the fix.
> > >keeping `early_printk' sometimes can be helpful and people even want to
> > >use `early_printk' as a panic() console fallback (because of those nasty
> > >deadlocks that spoil printk->call_console_drivers()).
> > >
> >
> > Sure, but as a user, how are you supposed to know that?
>
> Good point! I wonder if the authors of the keep_bootcon option
> actually knew about it. I do not see this risk mentioned anywhere
> and the early consoles might work long enough by chance.
>
> One problem is that real consoles might be registered much later
> when it is done using an async probe calls. It might open a big window
> when there is no visible output and debugging is "impossible".
yes. besides, Peter wants to have early consoles as panic fallback.
> I am not comfortable with removing the only way to debug some type
> of bugs. But the current state is broken as well.
yes and yes.
> IMHO, the reasonable solution is to move early console code and data
> out of the init sections. We should do this for the early consoles
> where the corresponding real console is registered using a deferred
> probe. Others should be already replaced by the real console when
> printk_late_init() is called. At least this is how I understand it.
so I was thinking about two options:
a) do not keep consoles in init section
b) have a special init section for consoles code and avoid offloading
the corresponding pages when we see that keep flag
"b" seems like a crazy idea at glance, but it kinda makes some sense at
the same time. dunno.
-ss
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hello,
sorry, I'm on a sick leave and can't check emails that often.
On (07/11/17 14:43), Petr Mladek wrote:
[..]
> > >>The keep_bootcon flag prevents the unregistration of a boot console,
> > >>even if it's data and code reside in the init section and are about to
> > >>be freed. 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.
>
> > >if someone asked to `keep_bootcon' but we actually don't keep it, then
> > >what's the purpose of the flag and
> > > pr_info("debug: skip boot console de-registration.\n")?
>
> Exactly. The important information is in the commit 7bf693951a8e5f7e
> ("console: allow to retain boot console via boot option keep_bootcon"):
>
> On some architectures, the boot process involves de-registering the boot
> console (early boot), initialize drivers and then re-register the console.
>
> This mechanism introduces a window in which no printk can happen on the
> console and messages are buffered and then printed once the new console is
> available.
>
> If a kernel crashes during this window, all it's left on the boot console
> is "console [foo] enabled, boot console disabled" making debug of the
> crash
> rather 'interesting'.
sure. no objections here.
I probably mis-worded my thoughts. I agree with Matt's conclusions and
he made valid points I just disagree with the fix.
> > >keeping `early_printk' sometimes can be helpful and people even want to
> > >use `early_printk' as a panic() console fallback (because of those nasty
> > >deadlocks that spoil printk->call_console_drivers()).
> > >
> >
> > Sure, but as a user, how are you supposed to know that?
>
> Good point! I wonder if the authors of the keep_bootcon option
> actually knew about it. I do not see this risk mentioned anywhere
> and the early consoles might work long enough by chance.
>
> One problem is that real consoles might be registered much later
> when it is done using an async probe calls. It might open a big window
> when there is no visible output and debugging is "impossible".
yes. besides, Peter wants to have early consoles as panic fallback.
> I am not comfortable with removing the only way to debug some type
> of bugs. But the current state is broken as well.
yes and yes.
> IMHO, the reasonable solution is to move early console code and data
> out of the init sections. We should do this for the early consoles
> where the corresponding real console is registered using a deferred
> probe. Others should be already replaced by the real console when
> printk_late_init() is called. At least this is how I understand it.
so I was thinking about two options:
a) do not keep consoles in init section
b) have a special init section for consoles code and avoid offloading
the corresponding pages when we see that keep flag
"b" seems like a crazy idea at glance, but it kinda makes some sense at
the same time. dunno.
-ss
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On 14/07/17 13:40, Petr Mladek wrote: On Wed 2017-07-12 13:11:17, Petr Mladek wrote: On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: On 11/07/17 13:43, Petr Mladek wrote: IMHO, the reasonable solution is to move early console code and data out of the init sections. We should do this for the early consoles where the corresponding real console is registered using a deferred probe. Others should be already replaced by the real console when printk_late_init() is called. At least this is how I understand it. This seems like the most reasonable way forward to me as well, though sadly will lead to some post-init kernel bloat. I still think, however, that this patch is a reasonable change to make. The thing is that this patch "silently" makes the keep_bootcon option almost unusable. I was wrong here. I thought that most early consoles used the init section. It was mentioned somewhere and I looked a wrong way. But this is not true. In fact, it seems that there are only few of them. Most early consoles have struct console and the write() callback in the normal section that is preserved. Matt's patch and the keep_bootcon option makes sense to me after all. Let me to resend Matt's patch with some small improvements and one more patch that improves the check of early consoles that use init section. I'll keep Matt as the author of the first patch. Thanks for looking into this and your fix Petr! Matt Best Regards, Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On 14/07/17 13:40, Petr Mladek wrote: On Wed 2017-07-12 13:11:17, Petr Mladek wrote: On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: On 11/07/17 13:43, Petr Mladek wrote: IMHO, the reasonable solution is to move early console code and data out of the init sections. We should do this for the early consoles where the corresponding real console is registered using a deferred probe. Others should be already replaced by the real console when printk_late_init() is called. At least this is how I understand it. This seems like the most reasonable way forward to me as well, though sadly will lead to some post-init kernel bloat. I still think, however, that this patch is a reasonable change to make. The thing is that this patch "silently" makes the keep_bootcon option almost unusable. I was wrong here. I thought that most early consoles used the init section. It was mentioned somewhere and I looked a wrong way. But this is not true. In fact, it seems that there are only few of them. Most early consoles have struct console and the write() callback in the normal section that is preserved. Matt's patch and the keep_bootcon option makes sense to me after all. Let me to resend Matt's patch with some small improvements and one more patch that improves the check of early consoles that use init section. I'll keep Matt as the author of the first patch. Thanks for looking into this and your fix Petr! Matt Best Regards, Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On Wed 2017-07-12 13:11:17, Petr Mladek wrote: > On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: > > On 11/07/17 13:43, Petr Mladek wrote: > > >IMHO, the reasonable solution is to move early console code and data > > >out of the init sections. We should do this for the early consoles > > >where the corresponding real console is registered using a deferred > > >probe. Others should be already replaced by the real console when > > >printk_late_init() is called. At least this is how I understand it. > > > > This seems like the most reasonable way forward to me as well, > > though sadly will lead to some post-init kernel bloat. > > > > I still think, however, that this patch is a reasonable change to > > make. > > The thing is that this patch "silently" makes the keep_bootcon > option almost unusable. I was wrong here. I thought that most early consoles used the init section. It was mentioned somewhere and I looked a wrong way. But this is not true. In fact, it seems that there are only few of them. Most early consoles have struct console and the write() callback in the normal section that is preserved. Matt's patch and the keep_bootcon option makes sense to me after all. Let me to resend Matt's patch with some small improvements and one more patch that improves the check of early consoles that use init section. I'll keep Matt as the author of the first patch. Best Regards, Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On Wed 2017-07-12 13:11:17, Petr Mladek wrote: > On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: > > On 11/07/17 13:43, Petr Mladek wrote: > > >IMHO, the reasonable solution is to move early console code and data > > >out of the init sections. We should do this for the early consoles > > >where the corresponding real console is registered using a deferred > > >probe. Others should be already replaced by the real console when > > >printk_late_init() is called. At least this is how I understand it. > > > > This seems like the most reasonable way forward to me as well, > > though sadly will lead to some post-init kernel bloat. > > > > I still think, however, that this patch is a reasonable change to > > make. > > The thing is that this patch "silently" makes the keep_bootcon > option almost unusable. I was wrong here. I thought that most early consoles used the init section. It was mentioned somewhere and I looked a wrong way. But this is not true. In fact, it seems that there are only few of them. Most early consoles have struct console and the write() callback in the normal section that is preserved. Matt's patch and the keep_bootcon option makes sense to me after all. Let me to resend Matt's patch with some small improvements and one more patch that improves the check of early consoles that use init section. I'll keep Matt as the author of the first patch. Best Regards, Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: > On 11/07/17 13:43, Petr Mladek wrote: > >IMHO, the reasonable solution is to move early console code and data > >out of the init sections. We should do this for the early consoles > >where the corresponding real console is registered using a deferred > >probe. Others should be already replaced by the real console when > >printk_late_init() is called. At least this is how I understand it. > > This seems like the most reasonable way forward to me as well, > though sadly will lead to some post-init kernel bloat. > > I still think, however, that this patch is a reasonable change to > make. The thing is that this patch "silently" makes the keep_bootcon option almost unusable. The remaining use in register_console() does not make much sense. It keeps the boot console after the preferred real one was registered. > My other patch, placing serial earlycon in the init section is then > the wrong way round and we need to remove __init from e.g. early8250 > and so on. Yup. > >Matt, is there any chance that you look into this possibility? > > Sure, I can look at fixing up console drivers which we use on the > MIPS architecture. I have discussed this with my colleagues. It seems that implementation of keep_bootcon option is pretty broken. It allows to debug broken system using unreliable consoles (already freed). Therefore enabling the option adds more problems on its own. It would be nice to fix it. But it looks rather complicated. There are several possibilities that are more or less hacky. It is hard to guess the best solution if we do not know who is using it, how it helps or sucks for them. Before we spend a lot of time on this. Let me first try to send an RFC to remove the feature and put more people into CC. It will either be accepted or we will get arguments that would justify the needed changes to fix it. Best Regards, Petr PS: If you already stared migrating some consoles from the init section, I would be interested into feedback about what problems you did meet so far.
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On Tue 2017-07-11 15:41:50, Matt Redfearn wrote: > On 11/07/17 13:43, Petr Mladek wrote: > >IMHO, the reasonable solution is to move early console code and data > >out of the init sections. We should do this for the early consoles > >where the corresponding real console is registered using a deferred > >probe. Others should be already replaced by the real console when > >printk_late_init() is called. At least this is how I understand it. > > This seems like the most reasonable way forward to me as well, > though sadly will lead to some post-init kernel bloat. > > I still think, however, that this patch is a reasonable change to > make. The thing is that this patch "silently" makes the keep_bootcon option almost unusable. The remaining use in register_console() does not make much sense. It keeps the boot console after the preferred real one was registered. > My other patch, placing serial earlycon in the init section is then > the wrong way round and we need to remove __init from e.g. early8250 > and so on. Yup. > >Matt, is there any chance that you look into this possibility? > > Sure, I can look at fixing up console drivers which we use on the > MIPS architecture. I have discussed this with my colleagues. It seems that implementation of keep_bootcon option is pretty broken. It allows to debug broken system using unreliable consoles (already freed). Therefore enabling the option adds more problems on its own. It would be nice to fix it. But it looks rather complicated. There are several possibilities that are more or less hacky. It is hard to guess the best solution if we do not know who is using it, how it helps or sucks for them. Before we spend a lot of time on this. Let me first try to send an RFC to remove the feature and put more people into CC. It will either be accepted or we will get arguments that would justify the needed changes to fix it. Best Regards, Petr PS: If you already stared migrating some consoles from the init section, I would be interested into feedback about what problems you did meet so far.
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hi Petr,
On 11/07/17 13:43, Petr Mladek wrote:
Hi all,
let's first make sure that we understand the code the same way.
On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
On 07/07/17 05:45, Sergey Senozhatsky wrote:
On (07/06/17 11:38), Matt Redfearn wrote:
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
deregistered.
Yes.
This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
This does not make sense to me in this context. This commit has an
effect only when keep_bootcon is false. While the commit 4c30c6f566c0
("kernel/printk: do not turn off boot console in printk_late_init()
if keep_bootcon") _causes problems only_ when keep_bootcon is true.
What I want to say is that the two commits have effect when
keep_bootcon has different value. Therefore they could not fix
each other.
Yeah, what I meant was the situation of the boot console being freed
while still active (if keep_bootcon is false) was fixed by 81cc26f2bd11.
After 4c30c6f566c0, if keep_bootcon is true, then you run in to the
issue I had. The boot console is still active. It's code & data are
freed and poisoned with the SLAB poison. Some of that section is then
executed when the boot console is accessed. By happy coincidence(!), on
MIPS, the poison value 0x happens to be a valid opcode with no
side effect (it's a memory prefetch), so the CPU merrily runs off into
the weeds executing Mb's worth of them before finally executing
something that causes an exception of some kind. You get a kernel panic
with a cause and address that make no sense and have to backtrack trying
to figure out at what point the actual error occurred, and get nicely
sidetracked form the issue you were actually trying to debug that
necessitated keep_bootcon ;-).
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
Exactly. The important information is in the commit 7bf693951a8e5f7e
("console: allow to retain boot console via boot option keep_bootcon"):
On some architectures, the boot process involves de-registering the boot
console (early boot), initialize drivers and then re-register the console.
This mechanism introduces a window in which no printk can happen on the
console and messages are buffered and then printed once the new console is
available.
If a kernel crashes during this window, all it's left on the boot console
is "console [foo] enabled, boot console disabled" making debug of the crash
rather 'interesting'.
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
Sure, but as a user, how are you supposed to know that?
Good point! I wonder if the authors of the keep_bootcon option
actually knew about it. I do not see this risk mentioned anywhere
and the early consoles might work long enough by chance.
One problem is that real consoles might be registered much later
when it is done using an async probe calls. It might open a big window
when there is no visible output and debugging is "impossible".
I am not comfortable with removing the only way to debug some type
of bugs. But the current state is broken as well.
IMHO, the reasonable solution is to move early console code and data
out of the init sections. We should do this for the early consoles
where the corresponding real console is registered using a deferred
probe. Others should be already replaced by the real console when
printk_late_init() is called. At least this is how I understand it.
This seems like the most reasonable way forward to me as well, though
sadly will lead to some post-init kernel bloat.
I still think, however, that this patch is a reasonable change to make.
Though perhaps some warning (if keep_bootcon is active?) would be
appropriate so that a user knows that the boot console is being
unregistered (because it may no longer be functional after being freed)
and therefore silence can be expected - and we know that a driver needs
fixing. We can then proceed as you suggest and migrate away
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hi Petr,
On 11/07/17 13:43, Petr Mladek wrote:
Hi all,
let's first make sure that we understand the code the same way.
On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
On 07/07/17 05:45, Sergey Senozhatsky wrote:
On (07/06/17 11:38), Matt Redfearn wrote:
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
deregistered.
Yes.
This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
This does not make sense to me in this context. This commit has an
effect only when keep_bootcon is false. While the commit 4c30c6f566c0
("kernel/printk: do not turn off boot console in printk_late_init()
if keep_bootcon") _causes problems only_ when keep_bootcon is true.
What I want to say is that the two commits have effect when
keep_bootcon has different value. Therefore they could not fix
each other.
Yeah, what I meant was the situation of the boot console being freed
while still active (if keep_bootcon is false) was fixed by 81cc26f2bd11.
After 4c30c6f566c0, if keep_bootcon is true, then you run in to the
issue I had. The boot console is still active. It's code & data are
freed and poisoned with the SLAB poison. Some of that section is then
executed when the boot console is accessed. By happy coincidence(!), on
MIPS, the poison value 0x happens to be a valid opcode with no
side effect (it's a memory prefetch), so the CPU merrily runs off into
the weeds executing Mb's worth of them before finally executing
something that causes an exception of some kind. You get a kernel panic
with a cause and address that make no sense and have to backtrack trying
to figure out at what point the actual error occurred, and get nicely
sidetracked form the issue you were actually trying to debug that
necessitated keep_bootcon ;-).
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
Exactly. The important information is in the commit 7bf693951a8e5f7e
("console: allow to retain boot console via boot option keep_bootcon"):
On some architectures, the boot process involves de-registering the boot
console (early boot), initialize drivers and then re-register the console.
This mechanism introduces a window in which no printk can happen on the
console and messages are buffered and then printed once the new console is
available.
If a kernel crashes during this window, all it's left on the boot console
is "console [foo] enabled, boot console disabled" making debug of the crash
rather 'interesting'.
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
Sure, but as a user, how are you supposed to know that?
Good point! I wonder if the authors of the keep_bootcon option
actually knew about it. I do not see this risk mentioned anywhere
and the early consoles might work long enough by chance.
One problem is that real consoles might be registered much later
when it is done using an async probe calls. It might open a big window
when there is no visible output and debugging is "impossible".
I am not comfortable with removing the only way to debug some type
of bugs. But the current state is broken as well.
IMHO, the reasonable solution is to move early console code and data
out of the init sections. We should do this for the early consoles
where the corresponding real console is registered using a deferred
probe. Others should be already replaced by the real console when
printk_late_init() is called. At least this is how I understand it.
This seems like the most reasonable way forward to me as well, though
sadly will lead to some post-init kernel bloat.
I still think, however, that this patch is a reasonable change to make.
Though perhaps some warning (if keep_bootcon is active?) would be
appropriate so that a user knows that the boot console is being
unregistered (because it may no longer be functional after being freed)
and therefore silence can be expected - and we know that a driver needs
fixing. We can then proceed as you suggest and migrate away
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hi all,
let's first make sure that we understand the code the same way.
On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
> On 07/07/17 05:45, Sergey Senozhatsky wrote:
> >On (07/06/17 11:38), Matt Redfearn wrote:
> >>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
> >>deregistered.
Yes.
> >>This was fixed by commit 81cc26f2bd11 ("printk: only
> >>unregister boot consoles when necessary").
This does not make sense to me in this context. This commit has an
effect only when keep_bootcon is false. While the commit 4c30c6f566c0
("kernel/printk: do not turn off boot console in printk_late_init()
if keep_bootcon") _causes problems only_ when keep_bootcon is true.
What I want to say is that the two commits have effect when
keep_bootcon has different value. Therefore they could not fix
each other.
> >>The keep_bootcon flag prevents the unregistration of a boot console,
> >>even if it's data and code reside in the init section and are about to
> >>be freed. 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.
> >if someone asked to `keep_bootcon' but we actually don't keep it, then
> >what's the purpose of the flag and
> > pr_info("debug: skip boot console de-registration.\n")?
Exactly. The important information is in the commit 7bf693951a8e5f7e
("console: allow to retain boot console via boot option keep_bootcon"):
On some architectures, the boot process involves de-registering the boot
console (early boot), initialize drivers and then re-register the console.
This mechanism introduces a window in which no printk can happen on the
console and messages are buffered and then printed once the new console is
available.
If a kernel crashes during this window, all it's left on the boot console
is "console [foo] enabled, boot console disabled" making debug of the crash
rather 'interesting'.
> >keeping `early_printk' sometimes can be helpful and people even want to
> >use `early_printk' as a panic() console fallback (because of those nasty
> >deadlocks that spoil printk->call_console_drivers()).
> >
>
> Sure, but as a user, how are you supposed to know that?
Good point! I wonder if the authors of the keep_bootcon option
actually knew about it. I do not see this risk mentioned anywhere
and the early consoles might work long enough by chance.
One problem is that real consoles might be registered much later
when it is done using an async probe calls. It might open a big window
when there is no visible output and debugging is "impossible".
I am not comfortable with removing the only way to debug some type
of bugs. But the current state is broken as well.
IMHO, the reasonable solution is to move early console code and data
out of the init sections. We should do this for the early consoles
where the corresponding real console is registered using a deferred
probe. Others should be already replaced by the real console when
printk_late_init() is called. At least this is how I understand it.
Matt, is there any chance that you look into this possibility?
Best Regards,
Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
Hi all,
let's first make sure that we understand the code the same way.
On Fri 2017-07-07 08:58:01, Matt Redfearn wrote:
> On 07/07/17 05:45, Sergey Senozhatsky wrote:
> >On (07/06/17 11:38), Matt Redfearn wrote:
> >>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
> >>deregistered.
Yes.
> >>This was fixed by commit 81cc26f2bd11 ("printk: only
> >>unregister boot consoles when necessary").
This does not make sense to me in this context. This commit has an
effect only when keep_bootcon is false. While the commit 4c30c6f566c0
("kernel/printk: do not turn off boot console in printk_late_init()
if keep_bootcon") _causes problems only_ when keep_bootcon is true.
What I want to say is that the two commits have effect when
keep_bootcon has different value. Therefore they could not fix
each other.
> >>The keep_bootcon flag prevents the unregistration of a boot console,
> >>even if it's data and code reside in the init section and are about to
> >>be freed. 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.
> >if someone asked to `keep_bootcon' but we actually don't keep it, then
> >what's the purpose of the flag and
> > pr_info("debug: skip boot console de-registration.\n")?
Exactly. The important information is in the commit 7bf693951a8e5f7e
("console: allow to retain boot console via boot option keep_bootcon"):
On some architectures, the boot process involves de-registering the boot
console (early boot), initialize drivers and then re-register the console.
This mechanism introduces a window in which no printk can happen on the
console and messages are buffered and then printed once the new console is
available.
If a kernel crashes during this window, all it's left on the boot console
is "console [foo] enabled, boot console disabled" making debug of the crash
rather 'interesting'.
> >keeping `early_printk' sometimes can be helpful and people even want to
> >use `early_printk' as a panic() console fallback (because of those nasty
> >deadlocks that spoil printk->call_console_drivers()).
> >
>
> Sure, but as a user, how are you supposed to know that?
Good point! I wonder if the authors of the keep_bootcon option
actually knew about it. I do not see this risk mentioned anywhere
and the early consoles might work long enough by chance.
One problem is that real consoles might be registered much later
when it is done using an async probe calls. It might open a big window
when there is no visible output and debugging is "impossible".
I am not comfortable with removing the only way to debug some type
of bugs. But the current state is broken as well.
IMHO, the reasonable solution is to move early console code and data
out of the init sections. We should do this for the early consoles
where the corresponding real console is registered using a deferred
probe. Others should be already replaced by the real console when
printk_late_init() is called. At least this is how I understand it.
Matt, is there any chance that you look into this possibility?
Best Regards,
Petr
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On 07/07/17 05:45, Sergey Senozhatsky wrote:
On (07/06/17 11:38), Matt Redfearn wrote:
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
deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
Signed-off-by: Matt Redfearn
well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
Sure, but as a user, how are you supposed to know that? And if your
kernel is configured with SLAB poisoning, when the earlycon resident in
initdata is freed all of it's text and data is replaced with a poison
value, meaning it definitely won't work anymore anyway, and will instead
likely just panic the kernel, often with a completely meaningless
message (I know, because that's how I tracked down the need for this patch).
With this patch, the earlycon will only be unregistered if it is in the
init section and will no longer be functional anyway. Perhaps it would
be better to change the structure to make that more explicit, something
like:
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.
*/
unregister_console(con);
}
Thanks,
Matt
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..b5411f44eed4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ 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) {
/*
* Make sure to unregister boot consoles whose data
* resides in the init section before the init section
what about bootconsoles that are still not getting de-registered due
to keep_bootcon in register_console()?
-ss
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On 07/07/17 05:45, Sergey Senozhatsky wrote:
On (07/06/17 11:38), Matt Redfearn wrote:
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
deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
Signed-off-by: Matt Redfearn
well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
Sure, but as a user, how are you supposed to know that? And if your
kernel is configured with SLAB poisoning, when the earlycon resident in
initdata is freed all of it's text and data is replaced with a poison
value, meaning it definitely won't work anymore anyway, and will instead
likely just panic the kernel, often with a completely meaningless
message (I know, because that's how I tracked down the need for this patch).
With this patch, the earlycon will only be unregistered if it is in the
init section and will no longer be functional anyway. Perhaps it would
be better to change the structure to make that more explicit, something
like:
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.
*/
unregister_console(con);
}
Thanks,
Matt
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..b5411f44eed4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ 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) {
/*
* Make sure to unregister boot consoles whose data
* resides in the init section before the init section
what about bootconsoles that are still not getting de-registered due
to keep_bootcon in register_console()?
-ss
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On (07/06/17 11:38), Matt Redfearn wrote:
> 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
> deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
> unregister boot consoles when necessary").
> The keep_bootcon flag prevents the unregistration of a boot console,
> even if it's data and code reside in the init section and are about to
> be freed. 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.
>
> Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
> Signed-off-by: Matt Redfearn
well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index a1db38abac5b..b5411f44eed4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2660,7 +2660,7 @@ 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) {
> /*
>* Make sure to unregister boot consoles whose data
>* resides in the init section before the init section
what about bootconsoles that are still not getting de-registered due
to keep_bootcon in register_console()?
-ss
Re: [PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
On (07/06/17 11:38), Matt Redfearn wrote:
> 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
> deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
> unregister boot consoles when necessary").
> The keep_bootcon flag prevents the unregistration of a boot console,
> even if it's data and code reside in the init section and are about to
> be freed. 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.
>
> Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
> Signed-off-by: Matt Redfearn
well... hm. don't specify `keep_bootcon' if your bootcon access initdata?
keeping `early_printk' sometimes can be helpful and people even want to
use `early_printk' as a panic() console fallback (because of those nasty
deadlocks that spoil printk->call_console_drivers()).
if someone asked to `keep_bootcon' but we actually don't keep it, then
what's the purpose of the flag and
pr_info("debug: skip boot console de-registration.\n")?
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index a1db38abac5b..b5411f44eed4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2660,7 +2660,7 @@ 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) {
> /*
>* Make sure to unregister boot consoles whose data
>* resides in the init section before the init section
what about bootconsoles that are still not getting de-registered due
to keep_bootcon in register_console()?
-ss
[PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
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
deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
Signed-off-by: Matt Redfearn
---
kernel/printk/printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..b5411f44eed4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ 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) {
/*
* Make sure to unregister boot consoles whose data
* resides in the init section before the init section
--
2.7.4
[PATCH 1/2] printk: Unconditionally unregister boot consoles if in init section
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
deregistered. This was fixed by commit 81cc26f2bd11 ("printk: only
unregister boot consoles when necessary").
The keep_bootcon flag prevents the unregistration of a boot console,
even if it's data and code reside in the init section and are about to
be freed. 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.
Fixes 81cc26f2bd11 ("printk: only unregister boot consoles when necessary").
Signed-off-by: Matt Redfearn
---
kernel/printk/printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..b5411f44eed4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ 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) {
/*
* Make sure to unregister boot consoles whose data
* resides in the init section before the init section
--
2.7.4
