[PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used

[PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>


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.10e598f2@gandalf.local.home

Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with __sched_setscheduler()")
Cc: Andrew Morton <akpm@osdl.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 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

Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used

[Steven Rostedt]

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 <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 
> 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.10e598f2@gandalf.local.home
> 
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with __sched_setscheduler()")
> Cc: Andrew Morton <akpm@osdl.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  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) {

Re: [PATCH] sched: Do not bug in __sched_setscheduler() when pi is not used

[Peter Zijlstra]

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

[Florian Fainelli]

On 3/9/17 7:18 AM, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 
> 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.10e598f2@gandalf.local.home
> 
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with __sched_setscheduler()")
> Cc: Andrew Morton <akpm@osdl.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

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

[Greg KH]

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)" <rostedt@goodmis.org>
> > 
> > 
> > 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.10e598f2@gandalf.local.home
> > 
> > Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > Fixes: dbc7f069b93a ("sched: Use replace normalize_task() with __sched_setscheduler()")
> > Cc: Andrew Morton <akpm@osdl.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> 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

[Steven Rostedt]

On Mon, 19 Nov 2018 16:13:11 +0100
Greg KH <gregkh@linuxfoundation.org> 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

[Greg KH]

On Mon, Nov 19, 2018 at 10:46:54AM -0500, Steven Rostedt wrote:
> On Mon, 19 Nov 2018 16:13:11 +0100
> Greg KH <gregkh@linuxfoundation.org> 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

[Steven Rostedt]

On Mon, 19 Nov 2018 17:24:32 +0100
Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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

[Florian Fainelli]

On 11/19/18 8:35 AM, Steven Rostedt wrote:
> On Mon, 19 Nov 2018 17:24:32 +0100
> Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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

[Steven Rostedt]

On Mon, 19 Nov 2018 11:09:50 -0800
Florian Fainelli <f.fainelli@gmail.com> 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) <rostedt@goodmis.org>

-- 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

[Greg KH]

On Mon, Nov 19, 2018 at 02:27:56PM -0500, Steven Rostedt wrote:
> On Mon, 19 Nov 2018 11:09:50 -0800
> Florian Fainelli <f.fainelli@gmail.com> 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) <rostedt@goodmis.org>

Now applied, thanks.

greg k-h