Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Mon, Nov 19, 2018 at 02:27:56PM -0500, Steven Rostedt wrote: > On Mon, 19 Nov 2018 11:09:50 -0800 > Florian Fainelli wrote: > > > > > I did the change against v4.2.8 below. > > > > Thanks Steven! Here is the local 4.9 backport: > > Yours even updates the comment. > > Reviewed-by: Steven Rostedt (VMware) Now applied, thanks. greg k-h
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Mon, 19 Nov 2018 11:09:50 -0800 Florian Fainelli wrote: > > I did the change against v4.2.8 below. > > Thanks Steven! Here is the local 4.9 backport: Yours even updates the comment. Reviewed-by: Steven Rostedt (VMware) -- Steve > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 917be221438b..6b3fff6a6437 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -4087,8 +4087,8 @@ static int __sched_setscheduler(struct task_struct *p, > int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE; > struct rq *rq; > > - /* may grab non-irq protected spin_locks */ > - BUG_ON(in_interrupt()); > + /* The pi code expects interrupts enabled */ > + BUG_ON(pi && in_interrupt()); > recheck: > /* double check policy once rq lock held */
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On 11/19/18 8:35 AM, Steven Rostedt wrote: > On Mon, 19 Nov 2018 17:24:32 +0100 > Greg KH wrote: > >> On Mon, Nov 19, 2018 at 10:46:54AM -0500, Steven Rostedt wrote: >>> On Mon, 19 Nov 2018 16:13:11 +0100 >>> Greg KH wrote: >>> > Can this patch also be applied to the stable trees? The offending commit > was first introduced in 4.2. What is the git commit id of this patch in Linus's tree? >>> >>> 896bbb2522587e3b8eb2a0d204d43ccc1042a00d >>> >>> The subject was changed when it was applied. >> >> Ah, that helps. >> >> But why is this really needed in the older kernels? You want to crash >> your machine if someone got things wrong? Given that I doubt this is >> being hit anymore, why is it needed in 4.9.y and 4.4.y? > > The problem is that it can crash when people didn't get it wrong (by > using sysrq). > > The bug happened when we combined two functions into one, and where the > BUG_ON() from one was now added to the other function. The one (where > pi is true) requires the function to be called with interrupts > disabled. The other did not have this requirement (and pi happens to be > false). Thus, you can trigger the BUG_ON(in_interrupt()) with a simple > sysrq key stroke, and crash your kernel when it did nothing wrong. > >> >> Also, it doesn't apply there so someone needs to do the backport... >> > > I did the change against v4.2.8 below. Thanks Steven! Here is the local 4.9 backport: diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 917be221438b..6b3fff6a6437 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4087,8 +4087,8 @@ static int __sched_setscheduler(struct task_struct *p, int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE; struct rq *rq; - /* may grab non-irq protected spin_locks */ - BUG_ON(in_interrupt()); + /* The pi code expects interrupts enabled */ + BUG_ON(pi && in_interrupt()); recheck: /* double check policy once rq lock held */ if (policy < 0) { > > -- Steve > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 6776631676e0..b2af7989eb5b 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -3682,7 +3682,7 @@ static int __sched_setscheduler(struct task_struct *p, > int reset_on_fork; > > /* may grab non-irq protected spin_locks */ > - BUG_ON(in_interrupt()); > + BUG_ON(pi && in_interrupt()); > recheck: > /* double check policy once rq lock held */ > if (policy < 0) { > -- Florian
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Mon, 19 Nov 2018 17:24:32 +0100 Greg KH wrote: > On Mon, Nov 19, 2018 at 10:46:54AM -0500, Steven Rostedt wrote: > > On Mon, 19 Nov 2018 16:13:11 +0100 > > Greg KH wrote: > > > > > > Can this patch also be applied to the stable trees? The offending commit > > > > was first introduced in 4.2. > > > > > > What is the git commit id of this patch in Linus's tree? > > > > 896bbb2522587e3b8eb2a0d204d43ccc1042a00d > > > > The subject was changed when it was applied. > > Ah, that helps. > > But why is this really needed in the older kernels? You want to crash > your machine if someone got things wrong? Given that I doubt this is > being hit anymore, why is it needed in 4.9.y and 4.4.y? The problem is that it can crash when people didn't get it wrong (by using sysrq). The bug happened when we combined two functions into one, and where the BUG_ON() from one was now added to the other function. The one (where pi is true) requires the function to be called with interrupts disabled. The other did not have this requirement (and pi happens to be false). Thus, you can trigger the BUG_ON(in_interrupt()) with a simple sysrq key stroke, and crash your kernel when it did nothing wrong. > > Also, it doesn't apply there so someone needs to do the backport... > I did the change against v4.2.8 below. -- Steve diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 6776631676e0..b2af7989eb5b 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -3682,7 +3682,7 @@ static int __sched_setscheduler(struct task_struct *p, int reset_on_fork; /* may grab non-irq protected spin_locks */ - BUG_ON(in_interrupt()); + BUG_ON(pi && in_interrupt()); recheck: /* double check policy once rq lock held */ if (policy < 0) {
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Mon, Nov 19, 2018 at 10:46:54AM -0500, Steven Rostedt wrote: > On Mon, 19 Nov 2018 16:13:11 +0100 > Greg KH wrote: > > > > Can this patch also be applied to the stable trees? The offending commit > > > was first introduced in 4.2. > > > > What is the git commit id of this patch in Linus's tree? > > 896bbb2522587e3b8eb2a0d204d43ccc1042a00d > > The subject was changed when it was applied. Ah, that helps. But why is this really needed in the older kernels? You want to crash your machine if someone got things wrong? Given that I doubt this is being hit anymore, why is it needed in 4.9.y and 4.4.y? Also, it doesn't apply there so someone needs to do the backport... thanks, greg k-h
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Mon, 19 Nov 2018 16:13:11 +0100 Greg KH wrote: > > Can this patch also be applied to the stable trees? The offending commit > > was first introduced in 4.2. > > What is the git commit id of this patch in Linus's tree? 896bbb2522587e3b8eb2a0d204d43ccc1042a00d The subject was changed when it was applied. Thanks, -- Steve
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Wed, Nov 14, 2018 at 12:55:20PM -0800, Florian Fainelli wrote: > On 3/9/17 7:18 AM, Steven Rostedt wrote: > > From: "Steven Rostedt (VMware)" > > > > > > When priority inheritance was added back in 2.6.18 to sched_setscheduler, it > > added a path to taking an rt-mutex wait_lock, which is not IRQ safe. As PI > > is not a common occurrence, lockdep will likely never trigger if > > sched_setscheduler was called from interrupt context. A BUG_ON() was added > > to trigger if __sched_setscheduler() was ever called from interrupt context > > because there was a possibility to take the wait_lock. > > > > Today the wait_lock is irq safe, but the path to taking it in > > sched_setscheduler() is the same as the path to taking it from normal > > context. The wait_lock is taken with raw_spin_lock_irq() and released with > > raw_spin_unlock_irq() which will indiscriminately enable interrupts, > > which would be bad in interrupt context. > > > > The problem is that normalize_rt_tasks, which is called by triggering the > > sysrq nice-all-RT-tasks was changed to call __sched_setscheduler(), and this > > is done from interrupt context! > > > > Now __sched_setscheduler() takes a "pi" parameter that is used to know if > > the priority inheritance should be called or not. As the BUG_ON() only cares > > about calling the PI code, it should only bug if called from interrupt > > context with the "pi" parameter set to true. > > > > Link: http://lkml.kernel.org/r/20170308124654.10e59...@gandalf.local.home > > > > Reported-by: Laurent Dufour > > Tested-by: Laurent Dufour > > Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with > > __sched_setscheduler()") > > Cc: Andrew Morton > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: Peter Zijlstra > > Cc: sta...@vger.kernel.org > > Signed-off-by: Steven Rostedt (VMware) > > Can this patch also be applied to the stable trees? The offending commit > was first introduced in 4.2. What is the git commit id of this patch in Linus's tree? thanks, greg k-h
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On 3/9/17 7:18 AM, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > > When priority inheritance was added back in 2.6.18 to sched_setscheduler, it > added a path to taking an rt-mutex wait_lock, which is not IRQ safe. As PI > is not a common occurrence, lockdep will likely never trigger if > sched_setscheduler was called from interrupt context. A BUG_ON() was added > to trigger if __sched_setscheduler() was ever called from interrupt context > because there was a possibility to take the wait_lock. > > Today the wait_lock is irq safe, but the path to taking it in > sched_setscheduler() is the same as the path to taking it from normal > context. The wait_lock is taken with raw_spin_lock_irq() and released with > raw_spin_unlock_irq() which will indiscriminately enable interrupts, > which would be bad in interrupt context. > > The problem is that normalize_rt_tasks, which is called by triggering the > sysrq nice-all-RT-tasks was changed to call __sched_setscheduler(), and this > is done from interrupt context! > > Now __sched_setscheduler() takes a "pi" parameter that is used to know if > the priority inheritance should be called or not. As the BUG_ON() only cares > about calling the PI code, it should only bug if called from interrupt > context with the "pi" parameter set to true. > > Link: http://lkml.kernel.org/r/20170308124654.10e59...@gandalf.local.home > > Reported-by: Laurent Dufour > Tested-by: Laurent Dufour > Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with > __sched_setscheduler()") > Cc: Andrew Morton > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: sta...@vger.kernel.org > Signed-off-by: Steven Rostedt (VMware) Can this patch also be applied to the stable trees? The offending commit was first introduced in 4.2. Thank you! > --- > kernel/sched/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 3b31fc0..7292fa9 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -4129,8 +4129,8 @@ static int __sched_setscheduler(struct task_struct *p, > int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE; > struct rq *rq; > > - /* May grab non-irq protected spin_locks: */ > - BUG_ON(in_interrupt()); > + /* The pi code expects interrupts enabled */ > + BUG_ON(pi && in_interrupt()); > recheck: > /* Double check policy once rq lock held: */ > if (policy < 0) { > -- Florian
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
On Tue, May 16, 2017 at 06:55:20PM -0400, Steven Rostedt wrote: > > Peter, > > I've just been pinged by someone that triggered this bug again. Can you > take this patch and it probably should be marked for stable too. > Oh, rite. Sorry for letting it slip.
Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
Peter, I've just been pinged by someone that triggered this bug again. Can you take this patch and it probably should be marked for stable too. -- Steve On Thu, 9 Mar 2017 10:18:42 -0500 Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > > When priority inheritance was added back in 2.6.18 to sched_setscheduler, it > added a path to taking an rt-mutex wait_lock, which is not IRQ safe. As PI > is not a common occurrence, lockdep will likely never trigger if > sched_setscheduler was called from interrupt context. A BUG_ON() was added > to trigger if __sched_setscheduler() was ever called from interrupt context > because there was a possibility to take the wait_lock. > > Today the wait_lock is irq safe, but the path to taking it in > sched_setscheduler() is the same as the path to taking it from normal > context. The wait_lock is taken with raw_spin_lock_irq() and released with > raw_spin_unlock_irq() which will indiscriminately enable interrupts, > which would be bad in interrupt context. > > The problem is that normalize_rt_tasks, which is called by triggering the > sysrq nice-all-RT-tasks was changed to call __sched_setscheduler(), and this > is done from interrupt context! > > Now __sched_setscheduler() takes a "pi" parameter that is used to know if > the priority inheritance should be called or not. As the BUG_ON() only cares > about calling the PI code, it should only bug if called from interrupt > context with the "pi" parameter set to true. > > Link: http://lkml.kernel.org/r/20170308124654.10e59...@gandalf.local.home > > Reported-by: Laurent Dufour > Tested-by: Laurent Dufour > Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with > __sched_setscheduler()") > Cc: Andrew Morton > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: sta...@vger.kernel.org > Signed-off-by: Steven Rostedt (VMware) > --- > kernel/sched/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 3b31fc0..7292fa9 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -4129,8 +4129,8 @@ static int __sched_setscheduler(struct task_struct *p, > int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE; > struct rq *rq; > > - /* May grab non-irq protected spin_locks: */ > - BUG_ON(in_interrupt()); > + /* The pi code expects interrupts enabled */ > + BUG_ON(pi && in_interrupt()); > recheck: > /* Double check policy once rq lock held: */ > if (policy < 0) {
[PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used
From: "Steven Rostedt (VMware)" When priority inheritance was added back in 2.6.18 to sched_setscheduler, it added a path to taking an rt-mutex wait_lock, which is not IRQ safe. As PI is not a common occurrence, lockdep will likely never trigger if sched_setscheduler was called from interrupt context. A BUG_ON() was added to trigger if __sched_setscheduler() was ever called from interrupt context because there was a possibility to take the wait_lock. Today the wait_lock is irq safe, but the path to taking it in sched_setscheduler() is the same as the path to taking it from normal context. The wait_lock is taken with raw_spin_lock_irq() and released with raw_spin_unlock_irq() which will indiscriminately enable interrupts, which would be bad in interrupt context. The problem is that normalize_rt_tasks, which is called by triggering the sysrq nice-all-RT-tasks was changed to call __sched_setscheduler(), and this is done from interrupt context! Now __sched_setscheduler() takes a "pi" parameter that is used to know if the priority inheritance should be called or not. As the BUG_ON() only cares about calling the PI code, it should only bug if called from interrupt context with the "pi" parameter set to true. Link: http://lkml.kernel.org/r/20170308124654.10e59...@gandalf.local.home Reported-by: Laurent Dufour Tested-by: Laurent Dufour Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with __sched_setscheduler()") Cc: Andrew Morton Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Peter Zijlstra Cc: sta...@vger.kernel.org Signed-off-by: Steven Rostedt (VMware) --- kernel/sched/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 3b31fc0..7292fa9 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4129,8 +4129,8 @@ static int __sched_setscheduler(struct task_struct *p, int queue_flags = DEQUEUE_SAVE | DEQUEUE_MOVE; struct rq *rq; - /* May grab non-irq protected spin_locks: */ - BUG_ON(in_interrupt()); + /* The pi code expects interrupts enabled */ + BUG_ON(pi && in_interrupt()); recheck: /* Double check policy once rq lock held: */ if (policy < 0) { -- 2.9.3