Mark noticed that syzkaller is able to reliably trigger the following
dl_rq->running_bw > dl_rq->this_bw
WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124 switched_from_dl+0x454/0x608
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x0/0x458
show_stack+0x20/0x30
dump_stack+0x180/0x250
panic+0x2dc/0x4ec
__warn_printk+0x0/0x150
report_bug+0x228/0x2d8
bug_handler+0xa0/0x1a0
brk_handler+0x2f0/0x568
do_debug_exception+0x1bc/0x5d0
el1_dbg+0x18/0x78
switched_from_dl+0x454/0x608
__sched_setscheduler+0x8cc/0x2018
sys_sched_setattr+0x340/0x758
el0_svc_naked+0x30/0x34
syzkaller reproducer runs a bunch of threads that constantly switch
between DEADLINE and NORMAL classes while interacting through futexes.
The splat above is caused by the fact that if a DEADLINE task is setattr
back to NORMAL while in non_contending state (blocked on a futex -
inactive timer armed), its contribution to running_bw is not removed
before sub_rq_bw() gets called (!task_on_rq_queued() branch) and the
latter sees running_bw > this_bw.
Fix it by removing a task contribution from running_bw if the task is
not queued and in non_contending state while switched to a different
class.
Reported-by: Mark Rutland <[email protected]>
Signed-off-by: Juri Lelli <[email protected]>
---
kernel/sched/deadline.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fbfc3f1d368a..10c7b51c0d1f 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2290,8 +2290,17 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
if (task_on_rq_queued(p) && p->dl.dl_runtime)
task_non_contending(p);
- if (!task_on_rq_queued(p))
+ if (!task_on_rq_queued(p)) {
+ /*
+ * Inactive timer is armed. However, p is leaving DEADLINE and
+ * might migrate away from this rq while continuing to run on
+ * some other class. We need to remove its contribution from
+ * this rq running_bw now, or sub_rq_bw (below) will complain.
+ */
+ if (p->dl.dl_non_contending)
+ sub_running_bw(&p->dl, &rq->dl);
sub_rq_bw(&p->dl, &rq->dl);
+ }
/*
* We cannot use inactive_task_timer() to invoke sub_running_bw()
--
2.14.4
On Wed, 11 Jul 2018 09:29:48 +0200
Juri Lelli <[email protected]> wrote:
> Mark noticed that syzkaller is able to reliably trigger the following
>
> dl_rq->running_bw > dl_rq->this_bw
> WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124
> switched_from_dl+0x454/0x608 Kernel panic - not syncing:
> panic_on_warn set ...
>
> CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
> Hardware name: linux,dummy-virt (DT)
> Call trace:
> dump_backtrace+0x0/0x458
> show_stack+0x20/0x30
> dump_stack+0x180/0x250
> panic+0x2dc/0x4ec
> __warn_printk+0x0/0x150
> report_bug+0x228/0x2d8
> bug_handler+0xa0/0x1a0
> brk_handler+0x2f0/0x568
> do_debug_exception+0x1bc/0x5d0
> el1_dbg+0x18/0x78
> switched_from_dl+0x454/0x608
> __sched_setscheduler+0x8cc/0x2018
> sys_sched_setattr+0x340/0x758
> el0_svc_naked+0x30/0x34
>
> syzkaller reproducer runs a bunch of threads that constantly switch
> between DEADLINE and NORMAL classes while interacting through futexes.
>
> The splat above is caused by the fact that if a DEADLINE task is
> setattr back to NORMAL while in non_contending state (blocked on a
> futex - inactive timer armed), its contribution to running_bw is not
> removed before sub_rq_bw() gets called (!task_on_rq_queued() branch)
> and the latter sees running_bw > this_bw.
>
> Fix it by removing a task contribution from running_bw if the task is
> not queued and in non_contending state while switched to a different
> class.
>
> Reported-by: Mark Rutland <[email protected]>
> Signed-off-by: Juri Lelli <[email protected]>
> ---
> kernel/sched/deadline.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
Reviewed-by: Luca Abeni <[email protected]>
And, thanks for taking care of this issue!
Thanks,
Luca
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fbfc3f1d368a..10c7b51c0d1f 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2290,8 +2290,17 @@ static void switched_from_dl(struct rq *rq,
> struct task_struct *p) if (task_on_rq_queued(p) && p->dl.dl_runtime)
> task_non_contending(p);
>
> - if (!task_on_rq_queued(p))
> + if (!task_on_rq_queued(p)) {
> + /*
> + * Inactive timer is armed. However, p is leaving
> DEADLINE and
> + * might migrate away from this rq while continuing
> to run on
> + * some other class. We need to remove its
> contribution from
> + * this rq running_bw now, or sub_rq_bw (below) will
> complain.
> + */
> + if (p->dl.dl_non_contending)
> + sub_running_bw(&p->dl, &rq->dl);
> sub_rq_bw(&p->dl, &rq->dl);
> + }
>
> /*
> * We cannot use inactive_task_timer() to invoke
> sub_running_bw()
On 07/11/2018 09:29 AM, Juri Lelli wrote:
> Mark noticed that syzkaller is able to reliably trigger the following
>
> dl_rq->running_bw > dl_rq->this_bw
> WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124 switched_from_dl+0x454/0x608
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
> Hardware name: linux,dummy-virt (DT)
> Call trace:
> dump_backtrace+0x0/0x458
> show_stack+0x20/0x30
> dump_stack+0x180/0x250
> panic+0x2dc/0x4ec
> __warn_printk+0x0/0x150
> report_bug+0x228/0x2d8
> bug_handler+0xa0/0x1a0
> brk_handler+0x2f0/0x568
> do_debug_exception+0x1bc/0x5d0
> el1_dbg+0x18/0x78
> switched_from_dl+0x454/0x608
> __sched_setscheduler+0x8cc/0x2018
> sys_sched_setattr+0x340/0x758
> el0_svc_naked+0x30/0x34
>
> syzkaller reproducer runs a bunch of threads that constantly switch
> between DEADLINE and NORMAL classes while interacting through futexes.
>
> The splat above is caused by the fact that if a DEADLINE task is setattr
> back to NORMAL while in non_contending state (blocked on a futex -
> inactive timer armed), its contribution to running_bw is not removed
> before sub_rq_bw() gets called (!task_on_rq_queued() branch) and the
> latter sees running_bw > this_bw.
>
> Fix it by removing a task contribution from running_bw if the task is
> not queued and in non_contending state while switched to a different
> class.
>
> Reported-by: Mark Rutland <[email protected]>
> Signed-off-by: Juri Lelli <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
Thanks!
-- Daniel
Commit-ID: e117cb52bdb4d376b711bee34af6434c9e314b3b
Gitweb: https://git.kernel.org/tip/e117cb52bdb4d376b711bee34af6434c9e314b3b
Author: Juri Lelli <[email protected]>
AuthorDate: Wed, 11 Jul 2018 09:29:48 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 15 Jul 2018 23:47:33 +0200
sched/deadline: Fix switched_from_dl() warning
Mark noticed that syzkaller is able to reliably trigger the following warning:
dl_rq->running_bw > dl_rq->this_bw
WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124 switched_from_dl+0x454/0x608
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x0/0x458
show_stack+0x20/0x30
dump_stack+0x180/0x250
panic+0x2dc/0x4ec
__warn_printk+0x0/0x150
report_bug+0x228/0x2d8
bug_handler+0xa0/0x1a0
brk_handler+0x2f0/0x568
do_debug_exception+0x1bc/0x5d0
el1_dbg+0x18/0x78
switched_from_dl+0x454/0x608
__sched_setscheduler+0x8cc/0x2018
sys_sched_setattr+0x340/0x758
el0_svc_naked+0x30/0x34
syzkaller reproducer runs a bunch of threads that constantly switch
between DEADLINE and NORMAL classes while interacting through futexes.
The splat above is caused by the fact that if a DEADLINE task is setattr
back to NORMAL while in non_contending state (blocked on a futex -
inactive timer armed), its contribution to running_bw is not removed
before sub_rq_bw() gets called (!task_on_rq_queued() branch) and the
latter sees running_bw > this_bw.
Fix it by removing a task contribution from running_bw if the task is
not queued and in non_contending state while switched to a different
class.
Reported-by: Mark Rutland <[email protected]>
Signed-off-by: Juri Lelli <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
Reviewed-by: Luca Abeni <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/sched/deadline.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fbfc3f1d368a..10c7b51c0d1f 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2290,8 +2290,17 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
if (task_on_rq_queued(p) && p->dl.dl_runtime)
task_non_contending(p);
- if (!task_on_rq_queued(p))
+ if (!task_on_rq_queued(p)) {
+ /*
+ * Inactive timer is armed. However, p is leaving DEADLINE and
+ * might migrate away from this rq while continuing to run on
+ * some other class. We need to remove its contribution from
+ * this rq running_bw now, or sub_rq_bw (below) will complain.
+ */
+ if (p->dl.dl_non_contending)
+ sub_running_bw(&p->dl, &rq->dl);
sub_rq_bw(&p->dl, &rq->dl);
+ }
/*
* We cannot use inactive_task_timer() to invoke sub_running_bw()
On Wed, 11 Jul 2018 09:29:48 +0200
Juri Lelli <[email protected]> wrote:
> Mark noticed that syzkaller is able to reliably trigger the following
>
> dl_rq->running_bw > dl_rq->this_bw
> WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124 switched_from_dl+0x454/0x608
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
> Hardware name: linux,dummy-virt (DT)
> Call trace:
> dump_backtrace+0x0/0x458
> show_stack+0x20/0x30
> dump_stack+0x180/0x250
> panic+0x2dc/0x4ec
> __warn_printk+0x0/0x150
> report_bug+0x228/0x2d8
> bug_handler+0xa0/0x1a0
> brk_handler+0x2f0/0x568
> do_debug_exception+0x1bc/0x5d0
> el1_dbg+0x18/0x78
> switched_from_dl+0x454/0x608
> __sched_setscheduler+0x8cc/0x2018
> sys_sched_setattr+0x340/0x758
> el0_svc_naked+0x30/0x34
>
> syzkaller reproducer runs a bunch of threads that constantly switch
> between DEADLINE and NORMAL classes while interacting through futexes.
>
> The splat above is caused by the fact that if a DEADLINE task is setattr
> back to NORMAL while in non_contending state (blocked on a futex -
> inactive timer armed), its contribution to running_bw is not removed
> before sub_rq_bw() gets called (!task_on_rq_queued() branch) and the
> latter sees running_bw > this_bw.
>
> Fix it by removing a task contribution from running_bw if the task is
> not queued and in non_contending state while switched to a different
> class.
>
> Reported-by: Mark Rutland <[email protected]>
> Signed-off-by: Juri Lelli <[email protected]>
> ---
> kernel/sched/deadline.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fbfc3f1d368a..10c7b51c0d1f 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2290,8 +2290,17 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
> if (task_on_rq_queued(p) && p->dl.dl_runtime)
> task_non_contending(p);
>
> - if (!task_on_rq_queued(p))
> + if (!task_on_rq_queued(p)) {
> + /*
> + * Inactive timer is armed. However, p is leaving DEADLINE and
> + * might migrate away from this rq while continuing to run on
> + * some other class. We need to remove its contribution from
> + * this rq running_bw now, or sub_rq_bw (below) will complain.
> + */
> + if (p->dl.dl_non_contending)
> + sub_running_bw(&p->dl, &rq->dl);
> sub_rq_bw(&p->dl, &rq->dl);
> + }
>
> /*
> * We cannot use inactive_task_timer() to invoke sub_running_bw()
Looking at this code:
if (!task_on_rq_queued(p)) {
/*
* Inactive timer is armed. However, p is leaving DEADLINE and
* might migrate away from this rq while continuing to run on
* some other class. We need to remove its contribution from
* this rq running_bw now, or sub_rq_bw (below) will complain.
*/
if (p->dl.dl_non_contending)
sub_running_bw(&p->dl, &rq->dl);
sub_rq_bw(&p->dl, &rq->dl);
}
/*
* We cannot use inactive_task_timer() to invoke sub_running_bw()
* at the 0-lag time, because the task could have been migrated
* while SCHED_OTHER in the meanwhile.
*/
if (p->dl.dl_non_contending)
p->dl.dl_non_contending = 0;
Question. Is the "dl_non_contending" only able to be set
if !task_on_rq_queued(p) is true? In that case, we could just clear it
in the first if block. If it's not true, I would think the subtraction
is needed regardless.
-- Steve
On 01/08/18 23:19, Steven Rostedt wrote:
> On Wed, 11 Jul 2018 09:29:48 +0200
> Juri Lelli <[email protected]> wrote:
>
> > Mark noticed that syzkaller is able to reliably trigger the following
> >
> > dl_rq->running_bw > dl_rq->this_bw
> > WARNING: CPU: 1 PID: 153 at kernel/sched/deadline.c:124 switched_from_dl+0x454/0x608
> > Kernel panic - not syncing: panic_on_warn set ...
> >
> > CPU: 1 PID: 153 Comm: syz-executor253 Not tainted 4.18.0-rc3+ #29
> > Hardware name: linux,dummy-virt (DT)
> > Call trace:
> > dump_backtrace+0x0/0x458
> > show_stack+0x20/0x30
> > dump_stack+0x180/0x250
> > panic+0x2dc/0x4ec
> > __warn_printk+0x0/0x150
> > report_bug+0x228/0x2d8
> > bug_handler+0xa0/0x1a0
> > brk_handler+0x2f0/0x568
> > do_debug_exception+0x1bc/0x5d0
> > el1_dbg+0x18/0x78
> > switched_from_dl+0x454/0x608
> > __sched_setscheduler+0x8cc/0x2018
> > sys_sched_setattr+0x340/0x758
> > el0_svc_naked+0x30/0x34
> >
> > syzkaller reproducer runs a bunch of threads that constantly switch
> > between DEADLINE and NORMAL classes while interacting through futexes.
> >
> > The splat above is caused by the fact that if a DEADLINE task is setattr
> > back to NORMAL while in non_contending state (blocked on a futex -
> > inactive timer armed), its contribution to running_bw is not removed
> > before sub_rq_bw() gets called (!task_on_rq_queued() branch) and the
> > latter sees running_bw > this_bw.
> >
> > Fix it by removing a task contribution from running_bw if the task is
> > not queued and in non_contending state while switched to a different
> > class.
> >
> > Reported-by: Mark Rutland <[email protected]>
> > Signed-off-by: Juri Lelli <[email protected]>
> > ---
> > kernel/sched/deadline.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index fbfc3f1d368a..10c7b51c0d1f 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -2290,8 +2290,17 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
> > if (task_on_rq_queued(p) && p->dl.dl_runtime)
> > task_non_contending(p);
> >
> > - if (!task_on_rq_queued(p))
> > + if (!task_on_rq_queued(p)) {
> > + /*
> > + * Inactive timer is armed. However, p is leaving DEADLINE and
> > + * might migrate away from this rq while continuing to run on
> > + * some other class. We need to remove its contribution from
> > + * this rq running_bw now, or sub_rq_bw (below) will complain.
> > + */
> > + if (p->dl.dl_non_contending)
> > + sub_running_bw(&p->dl, &rq->dl);
> > sub_rq_bw(&p->dl, &rq->dl);
> > + }
> >
> > /*
> > * We cannot use inactive_task_timer() to invoke sub_running_bw()
>
> Looking at this code:
>
> if (!task_on_rq_queued(p)) {
> /*
> * Inactive timer is armed. However, p is leaving DEADLINE and
> * might migrate away from this rq while continuing to run on
> * some other class. We need to remove its contribution from
> * this rq running_bw now, or sub_rq_bw (below) will complain.
> */
> if (p->dl.dl_non_contending)
> sub_running_bw(&p->dl, &rq->dl);
> sub_rq_bw(&p->dl, &rq->dl);
> }
>
> /*
> * We cannot use inactive_task_timer() to invoke sub_running_bw()
> * at the 0-lag time, because the task could have been migrated
> * while SCHED_OTHER in the meanwhile.
> */
> if (p->dl.dl_non_contending)
> p->dl.dl_non_contending = 0;
>
> Question. Is the "dl_non_contending" only able to be set
> if !task_on_rq_queued(p) is true? In that case, we could just clear it
> in the first if block.
Code right before the if block does
if (task_on_rq_queued(p) && p->dl.dl_runtime)
task_non_contending(p);
So we can end up with dl_non_contending being set even if task_on_rq_
queued(p) is true.
> If it's not true, I would think the subtraction
> is needed regardless.
And if we do sub_running_bw unconditionally we might end up subtracting
twice if inactive timer fired (resetting dl_non_contending) before we
end up here, no?
Thanks,
- Juri