* [PATCH v2] exit: Move preemption fixup up, move blocking operations down
@ 2020-03-05 22:06 Jann Horn
2020-03-05 22:13 ` Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jann Horn @ 2020-03-05 22:06 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner, Ingo Molnar
Cc: Andrew Morton, Frederic Weisbecker, Steven Rostedt, linux-kernel
With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
non-preemptible context look untidy; after the main oops, the kernel prints
a "sleeping function called from invalid context" report because
exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
fixup.
It looks like the same thing applies to profile_task_exit() and
kcov_task_exit().
Fix it by moving the preemption fixup up and the calls to
profile_task_exit() and kcov_task_exit() down.
Fixes: 1dc0fffc48af ("sched/core: Robustify preemption leak checks")
Signed-off-by: Jann Horn <jannh@google.com>
---
As so often, I have no idea which tree this should go through. tip? mm?
v2: now without adding redundant whitespace...
kernel/exit.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 2833ffb0c211..eb42d49fd99d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -713,8 +713,12 @@ void __noreturn do_exit(long code)
struct task_struct *tsk = current;
int group_dead;
- profile_task_exit(tsk);
- kcov_task_exit(tsk);
+ /*
+ * We can get here from a kernel oops, sometimes with preemption off.
+ * Start by checking for critical errors.
+ * Then fix up important state like USER_DS and preemption.
+ * Then do everything else.
+ */
WARN_ON(blk_needs_flush_plug(tsk));
@@ -732,6 +736,16 @@ void __noreturn do_exit(long code)
*/
set_fs(USER_DS);
+ if (unlikely(in_atomic())) {
+ pr_info("note: %s[%d] exited with preempt_count %d\n",
+ current->comm, task_pid_nr(current),
+ preempt_count());
+ preempt_count_set(PREEMPT_ENABLED);
+ }
+
+ profile_task_exit(tsk);
+ kcov_task_exit(tsk);
+
ptrace_event(PTRACE_EVENT_EXIT, code);
validate_creds_for_do_exit(tsk);
@@ -749,13 +763,6 @@ void __noreturn do_exit(long code)
exit_signals(tsk); /* sets PF_EXITING */
- if (unlikely(in_atomic())) {
- pr_info("note: %s[%d] exited with preempt_count %d\n",
- current->comm, task_pid_nr(current),
- preempt_count());
- preempt_count_set(PREEMPT_ENABLED);
- }
-
/* sync mm's RSS info before statistics gathering */
if (tsk->mm)
sync_mm_rss(tsk->mm);
base-commit: 9f65ed5fe41ce08ed1cb1f6a950f9ec694c142ad
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] exit: Move preemption fixup up, move blocking operations down
2020-03-05 22:06 [PATCH v2] exit: Move preemption fixup up, move blocking operations down Jann Horn
@ 2020-03-05 22:13 ` Steven Rostedt
2020-03-05 22:30 ` Jann Horn
2020-03-24 9:30 ` Jann Horn
2020-05-01 18:22 ` [tip: sched/core] " tip-bot2 for Jann Horn
2 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2020-03-05 22:13 UTC (permalink / raw)
To: Jann Horn
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Andrew Morton,
Frederic Weisbecker, linux-kernel
On Thu, 5 Mar 2020 23:06:57 +0100
Jann Horn <jannh@google.com> wrote:
> With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
> non-preemptible context look untidy; after the main oops, the kernel prints
> a "sleeping function called from invalid context" report because
> exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
> can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
> fixup.
>
> It looks like the same thing applies to profile_task_exit() and
> kcov_task_exit().
>
> Fix it by moving the preemption fixup up and the calls to
> profile_task_exit() and kcov_task_exit() down.
>
> Fixes: 1dc0fffc48af ("sched/core: Robustify preemption leak checks")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> @@ -732,6 +736,16 @@ void __noreturn do_exit(long code)
> */
> set_fs(USER_DS);
>
> + if (unlikely(in_atomic())) {
> + pr_info("note: %s[%d] exited with preempt_count %d\n",
> + current->comm, task_pid_nr(current),
> + preempt_count());
This should be more than a pr_info. It should also probably state the
"Dazed and confused, best to reboot" message.
Because if something crashed in a non preempt section, it may likely be
holding a lock that it will never release, causing a soon to be deadlock!
-- Steve
> + preempt_count_set(PREEMPT_ENABLED);
> + }
> +
> + profile_task_exit(tsk);
> + kcov_task_exit(tsk);
> +
> ptrace_event(PTRACE_EVENT_EXIT, code);
>
> validate_creds_for_do_exit(tsk);
> @@ -749,13 +763,6 @@ void __noreturn do_exit(long code)
>
> exit_signals(tsk); /* sets PF_EXITING */
>
> - if (unlikely(in_atomic())) {
> - pr_info("note: %s[%d] exited with preempt_count %d\n",
> - current->comm, task_pid_nr(current),
> - preempt_count());
> - preempt_count_set(PREEMPT_ENABLED);
> - }
> -
> /* sync mm's RSS info before statistics gathering */
> if (tsk->mm)
> sync_mm_rss(tsk->mm);
>
> base-commit: 9f65ed5fe41ce08ed1cb1f6a950f9ec694c142ad
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] exit: Move preemption fixup up, move blocking operations down
2020-03-05 22:13 ` Steven Rostedt
@ 2020-03-05 22:30 ` Jann Horn
2020-03-05 22:44 ` Steven Rostedt
0 siblings, 1 reply; 7+ messages in thread
From: Jann Horn @ 2020-03-05 22:30 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Andrew Morton,
Frederic Weisbecker, kernel list
On Thu, Mar 5, 2020 at 11:13 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 5 Mar 2020 23:06:57 +0100
> Jann Horn <jannh@google.com> wrote:
>
> > With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
> > non-preemptible context look untidy; after the main oops, the kernel prints
> > a "sleeping function called from invalid context" report because
> > exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
> > can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
> > fixup.
> >
> > It looks like the same thing applies to profile_task_exit() and
> > kcov_task_exit().
> >
> > Fix it by moving the preemption fixup up and the calls to
> > profile_task_exit() and kcov_task_exit() down.
[...]
> > + if (unlikely(in_atomic())) {
> > + pr_info("note: %s[%d] exited with preempt_count %d\n",
> > + current->comm, task_pid_nr(current),
> > + preempt_count());
>
> This should be more than a pr_info. It should also probably state the
> "Dazed and confused, best to reboot" message.
>
> Because if something crashed in a non preempt section, it may likely be
> holding a lock that it will never release, causing a soon to be deadlock!
I didn't write that code, I'm just moving it around. :P But I guess if
you want, I can change it in the same patch... something like this on
top? Does that look reasonable?
if (unlikely(in_atomic())) {
- pr_info("note: %s[%d] exited with preempt_count %d\n",
+ pr_emerg("note: %s[%d] exited with preempt_count %d,
system might deadlock, please reboot\n",
current->comm, task_pid_nr(current),
preempt_count());
preempt_count_set(PREEMPT_ENABLED);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] exit: Move preemption fixup up, move blocking operations down
2020-03-05 22:30 ` Jann Horn
@ 2020-03-05 22:44 ` Steven Rostedt
0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2020-03-05 22:44 UTC (permalink / raw)
To: Jann Horn
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Andrew Morton,
Frederic Weisbecker, kernel list
On Thu, 5 Mar 2020 23:30:13 +0100
Jann Horn <jannh@google.com> wrote:
> On Thu, Mar 5, 2020 at 11:13 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 5 Mar 2020 23:06:57 +0100
> > Jann Horn <jannh@google.com> wrote:
> >
> > > With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
> > > non-preemptible context look untidy; after the main oops, the kernel prints
> > > a "sleeping function called from invalid context" report because
> > > exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
> > > can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
> > > fixup.
> > >
> > > It looks like the same thing applies to profile_task_exit() and
> > > kcov_task_exit().
> > >
> > > Fix it by moving the preemption fixup up and the calls to
> > > profile_task_exit() and kcov_task_exit() down.
> [...]
> > > + if (unlikely(in_atomic())) {
> > > + pr_info("note: %s[%d] exited with preempt_count %d\n",
> > > + current->comm, task_pid_nr(current),
> > > + preempt_count());
> >
> > This should be more than a pr_info. It should also probably state the
> > "Dazed and confused, best to reboot" message.
> >
> > Because if something crashed in a non preempt section, it may likely be
> > holding a lock that it will never release, causing a soon to be deadlock!
>
> I didn't write that code, I'm just moving it around. :P But I guess if
Ah, I didn't scroll down enough to see it was just moved.
> you want, I can change it in the same patch... something like this on
> top? Does that look reasonable?
No, an update to the text should be done as a separate patch, as it is a
different type of change.
Thanks,
-- Steve
>
> if (unlikely(in_atomic())) {
> - pr_info("note: %s[%d] exited with preempt_count %d\n",
> + pr_emerg("note: %s[%d] exited with preempt_count %d,
> system might deadlock, please reboot\n",
> current->comm, task_pid_nr(current),
> preempt_count());
> preempt_count_set(PREEMPT_ENABLED);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] exit: Move preemption fixup up, move blocking operations down
2020-03-05 22:06 [PATCH v2] exit: Move preemption fixup up, move blocking operations down Jann Horn
2020-03-05 22:13 ` Steven Rostedt
@ 2020-03-24 9:30 ` Jann Horn
2020-04-16 8:52 ` Peter Zijlstra
2020-05-01 18:22 ` [tip: sched/core] " tip-bot2 for Jann Horn
2 siblings, 1 reply; 7+ messages in thread
From: Jann Horn @ 2020-03-24 9:30 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner, Ingo Molnar
Cc: Andrew Morton, Frederic Weisbecker, Steven Rostedt, kernel list
On Thu, Mar 5, 2020 at 11:07 PM Jann Horn <jannh@google.com> wrote:
> With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
> non-preemptible context look untidy; after the main oops, the kernel prints
> a "sleeping function called from invalid context" report because
> exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
> can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
> fixup.
>
> It looks like the same thing applies to profile_task_exit() and
> kcov_task_exit().
>
> Fix it by moving the preemption fixup up and the calls to
> profile_task_exit() and kcov_task_exit() down.
>
> Fixes: 1dc0fffc48af ("sched/core: Robustify preemption leak checks")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> As so often, I have no idea which tree this should go through. tip? mm?
Do the tip folks want to take this, since it's vaguely locking-related
and the fixed commit also came that way? Or should it go through
akpm's tree?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] exit: Move preemption fixup up, move blocking operations down
2020-03-24 9:30 ` Jann Horn
@ 2020-04-16 8:52 ` Peter Zijlstra
0 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2020-04-16 8:52 UTC (permalink / raw)
To: Jann Horn
Cc: Thomas Gleixner, Ingo Molnar, Andrew Morton, Frederic Weisbecker,
Steven Rostedt, kernel list
On Tue, Mar 24, 2020 at 10:30:02AM +0100, Jann Horn wrote:
> On Thu, Mar 5, 2020 at 11:07 PM Jann Horn <jannh@google.com> wrote:
> > With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
> > non-preemptible context look untidy; after the main oops, the kernel prints
> > a "sleeping function called from invalid context" report because
> > exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
> > can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
> > fixup.
> >
> > It looks like the same thing applies to profile_task_exit() and
> > kcov_task_exit().
> >
> > Fix it by moving the preemption fixup up and the calls to
> > profile_task_exit() and kcov_task_exit() down.
> >
> > Fixes: 1dc0fffc48af ("sched/core: Robustify preemption leak checks")
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> > As so often, I have no idea which tree this should go through. tip? mm?
>
> Do the tip folks want to take this, since it's vaguely locking-related
> and the fixed commit also came that way? Or should it go through
> akpm's tree?
I've picked it up, as it seems to be languishing. Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip: sched/core] exit: Move preemption fixup up, move blocking operations down
2020-03-05 22:06 [PATCH v2] exit: Move preemption fixup up, move blocking operations down Jann Horn
2020-03-05 22:13 ` Steven Rostedt
2020-03-24 9:30 ` Jann Horn
@ 2020-05-01 18:22 ` tip-bot2 for Jann Horn
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Jann Horn @ 2020-05-01 18:22 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Jann Horn, Peter Zijlstra (Intel), x86, LKML
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 586b58cac8b4683eb58a1446fbc399de18974e40
Gitweb: https://git.kernel.org/tip/586b58cac8b4683eb58a1446fbc399de18974e40
Author: Jann Horn <jannh@google.com>
AuthorDate: Thu, 05 Mar 2020 23:06:57 +01:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 30 Apr 2020 20:14:38 +02:00
exit: Move preemption fixup up, move blocking operations down
With CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_CGROUPS=y, kernel oopses in
non-preemptible context look untidy; after the main oops, the kernel prints
a "sleeping function called from invalid context" report because
exit_signals() -> cgroup_threadgroup_change_begin() -> percpu_down_read()
can sleep, and that happens before the preempt_count_set(PREEMPT_ENABLED)
fixup.
It looks like the same thing applies to profile_task_exit() and
kcov_task_exit().
Fix it by moving the preemption fixup up and the calls to
profile_task_exit() and kcov_task_exit() down.
Fixes: 1dc0fffc48af ("sched/core: Robustify preemption leak checks")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200305220657.46800-1-jannh@google.com
---
kernel/exit.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index ce2a75b..d56fe51 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -708,8 +708,12 @@ void __noreturn do_exit(long code)
struct task_struct *tsk = current;
int group_dead;
- profile_task_exit(tsk);
- kcov_task_exit(tsk);
+ /*
+ * We can get here from a kernel oops, sometimes with preemption off.
+ * Start by checking for critical errors.
+ * Then fix up important state like USER_DS and preemption.
+ * Then do everything else.
+ */
WARN_ON(blk_needs_flush_plug(tsk));
@@ -727,6 +731,16 @@ void __noreturn do_exit(long code)
*/
set_fs(USER_DS);
+ if (unlikely(in_atomic())) {
+ pr_info("note: %s[%d] exited with preempt_count %d\n",
+ current->comm, task_pid_nr(current),
+ preempt_count());
+ preempt_count_set(PREEMPT_ENABLED);
+ }
+
+ profile_task_exit(tsk);
+ kcov_task_exit(tsk);
+
ptrace_event(PTRACE_EVENT_EXIT, code);
validate_creds_for_do_exit(tsk);
@@ -744,13 +758,6 @@ void __noreturn do_exit(long code)
exit_signals(tsk); /* sets PF_EXITING */
- if (unlikely(in_atomic())) {
- pr_info("note: %s[%d] exited with preempt_count %d\n",
- current->comm, task_pid_nr(current),
- preempt_count());
- preempt_count_set(PREEMPT_ENABLED);
- }
-
/* sync mm's RSS info before statistics gathering */
if (tsk->mm)
sync_mm_rss(tsk->mm);
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-01 18:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-05 22:06 [PATCH v2] exit: Move preemption fixup up, move blocking operations down Jann Horn
2020-03-05 22:13 ` Steven Rostedt
2020-03-05 22:30 ` Jann Horn
2020-03-05 22:44 ` Steven Rostedt
2020-03-24 9:30 ` Jann Horn
2020-04-16 8:52 ` Peter Zijlstra
2020-05-01 18:22 ` [tip: sched/core] " tip-bot2 for Jann Horn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).