[PATCH 0/2] genirq: Move prio assignment into the newly created thread

[PATCH 0/2] genirq: Move prio assignment into the newly created thread

[Sebastian Andrzej Siewior]

Mike reported [0] a lockdep splat triggered by the nouveau driver together
with enabled threaded interrupts.
I can't easily tell if moving / changing locking within nouveau does not
break anything and does not create the same lockchain in another path.
Mike did confirm that the lockdep splat can be avoided by by moving
`request_irq()' out of the picture [1]. He also tested the two patches
which are scattered in the thread.

Mike is betting on commit
   710da3c8ea7df ("sched/core: Prevent race condition between cpuset and __sched_setscheduler()")

as the source change that introduce that problem assuming that locking
within the nouveau remained unchanged. He did confirm that this splat
exists also in a v5.4 stable kernel [2] which is where the change was
introduced.

[0] https://lkml.kernel.org/r/a23a826af7c108ea5651e73b8fbae5e653f16e86.camel@gmx.de
[1] https://lkml.kernel.org/r/431e81699f2310eabfe5af0a3de400ab99d9323b.camel@gmx.de
[2] https://lkml.kernel.org/r/f7e43b369c7d168c2ff04bfd91dfdf3afd0da12c.camel@gmx.de

Sebastian

[PATCH 1/2] kthread: Move prio/affinite change into the newly created thread

[Sebastian Andrzej Siewior]

With enabled threaded interrupts the nouveau driver reported the
following:
| Chain exists of:
|   &mm->mmap_lock#2 --> &device->mutex --> &cpuset_rwsem
|
|  Possible unsafe locking scenario:
|
|        CPU0                    CPU1
|        ----                    ----
|   lock(&cpuset_rwsem);
|                                lock(&device->mutex);
|                                lock(&cpuset_rwsem);
|   lock(&mm->mmap_lock#2);

The device->mutex is nvkm_device::mutex.

Unblocking the lockchain at `cpuset_rwsem' is probably the easiest thing
to do.
Move the priority reset to the start of the newly created thread.

Fixes: 710da3c8ea7df ("sched/core: Prevent race condition between cpuset and __sched_setscheduler()")
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://lkml.kernel.org/r/a23a826af7c108ea5651e73b8fbae5e653f16e86.camel@gmx.de
---
 kernel/kthread.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 933a625621b8d..4a31127c6efbf 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -243,6 +243,7 @@ EXPORT_SYMBOL_GPL(kthread_parkme);
 
 static int kthread(void *_create)
 {
+	static const struct sched_param param = { .sched_priority = 0 };
 	/* Copy data: it's on kthread's stack */
 	struct kthread_create_info *create = _create;
 	int (*threadfn)(void *data) = create->threadfn;
@@ -273,6 +274,13 @@ static int kthread(void *_create)
 	init_completion(&self->parked);
 	current->vfork_done = &self->exited;
 
+	/*
+	 * The new thread inherited kthreadd's priority and CPU mask. Reset
+	 * back to default in case they have been changed.
+	 */
+	sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
+	set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_FLAG_KTHREAD));
+
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;
@@ -370,7 +378,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	}
 	task = create->result;
 	if (!IS_ERR(task)) {
-		static const struct sched_param param = { .sched_priority = 0 };
 		char name[TASK_COMM_LEN];
 
 		/*
@@ -379,13 +386,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 		 */
 		vsnprintf(name, sizeof(name), namefmt, args);
 		set_task_comm(task, name);
-		/*
-		 * root may have changed our (kthreadd's) priority or CPU mask.
-		 * The kernel thread should not inherit these properties.
-		 */
-		sched_setscheduler_nocheck(task, SCHED_NORMAL, &param);
-		set_cpus_allowed_ptr(task,
-				     housekeeping_cpumask(HK_FLAG_KTHREAD));
 	}
 	kfree(create);
 	return task;
-- 
2.29.2

[PATCH 2/2] genirq: Move prio assignment into the newly created thread

[Sebastian Andrzej Siewior]

From: Thomas Gleixner <tglx@linutronix.de>

With enabled threaded interrupts the nouveau driver reported the
following:
| Chain exists of:
|   &mm->mmap_lock#2 --> &device->mutex --> &cpuset_rwsem
|
|  Possible unsafe locking scenario:
|
|        CPU0                    CPU1
|        ----                    ----
|   lock(&cpuset_rwsem);
|                                lock(&device->mutex);
|                                lock(&cpuset_rwsem);
|   lock(&mm->mmap_lock#2);

The device->mutex is nvkm_device::mutex.

Unblocking the lockchain at `cpuset_rwsem' is probably the easiest thing
to do.
Move the priority assignment to the start of the newly created thread.

Fixes: 710da3c8ea7df ("sched/core: Prevent race condition between cpuset and __sched_setscheduler()")
Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[bigeasy: Patch description]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://lkml.kernel.org/r/a23a826af7c108ea5651e73b8fbae5e653f16e86.camel@gmx.de
---
 kernel/irq/manage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c460e0496006e..079688d122a70 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1155,6 +1155,8 @@ static int irq_thread(void *data)
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
 			struct irqaction *action);
 
+	sched_set_fifo(current);
+
 	if (force_irqthreads && test_bit(IRQTF_FORCED_THREAD,
 					&action->thread_flags))
 		handler_fn = irq_forced_thread_fn;
@@ -1320,8 +1322,6 @@ setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
 	if (IS_ERR(t))
 		return PTR_ERR(t);
 
-	sched_set_fifo(t);
-
 	/*
 	 * We keep the reference to the task struct even if
 	 * the thread dies to avoid that the interrupt code
-- 
2.29.2

Re: [PATCH 1/2] kthread: Move prio/affinite change into the newly created thread

[Peter Zijlstra]

On Tue, Nov 10, 2020 at 12:38:47PM +0100, Sebastian Andrzej Siewior wrote:
> With enabled threaded interrupts the nouveau driver reported the
> following:
> | Chain exists of:
> |   &mm->mmap_lock#2 --> &device->mutex --> &cpuset_rwsem
> |
> |  Possible unsafe locking scenario:
> |
> |        CPU0                    CPU1
> |        ----                    ----
> |   lock(&cpuset_rwsem);
> |                                lock(&device->mutex);
> |                                lock(&cpuset_rwsem);
> |   lock(&mm->mmap_lock#2);
> 
> The device->mutex is nvkm_device::mutex.
> 
> Unblocking the lockchain at `cpuset_rwsem' is probably the easiest thing
> to do.
> Move the priority reset to the start of the newly created thread.
> 
> Fixes: 710da3c8ea7df ("sched/core: Prevent race condition between cpuset and __sched_setscheduler()")
> Reported-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Link: https://lkml.kernel.org/r/a23a826af7c108ea5651e73b8fbae5e653f16e86.camel@gmx.de

Moo... yes this is certainly the easiest solution, because nouveau is a
horrible rats nest. But when I spoke to Greg KH about this, he suggested
nouveau ought to be fixed.

Ben, I got terminally lost when trying to untangle nouvea init, is there
any chance this can be fixed to not hold that nvkm_device::mutex thing
while doing request_irq() ?

> ---
>  kernel/kthread.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 933a625621b8d..4a31127c6efbf 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -243,6 +243,7 @@ EXPORT_SYMBOL_GPL(kthread_parkme);
>  
>  static int kthread(void *_create)
>  {
> +	static const struct sched_param param = { .sched_priority = 0 };
>  	/* Copy data: it's on kthread's stack */
>  	struct kthread_create_info *create = _create;
>  	int (*threadfn)(void *data) = create->threadfn;
> @@ -273,6 +274,13 @@ static int kthread(void *_create)
>  	init_completion(&self->parked);
>  	current->vfork_done = &self->exited;
>  
> +	/*
> +	 * The new thread inherited kthreadd's priority and CPU mask. Reset
> +	 * back to default in case they have been changed.
> +	 */
> +	sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
> +	set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_FLAG_KTHREAD));
> +
>  	/* OK, tell user we're spawned, wait for stop or wakeup */
>  	__set_current_state(TASK_UNINTERRUPTIBLE);
>  	create->result = current;
> @@ -370,7 +378,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  	}
>  	task = create->result;
>  	if (!IS_ERR(task)) {
> -		static const struct sched_param param = { .sched_priority = 0 };
>  		char name[TASK_COMM_LEN];
>  
>  		/*
> @@ -379,13 +386,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  		 */
>  		vsnprintf(name, sizeof(name), namefmt, args);
>  		set_task_comm(task, name);
> -		/*
> -		 * root may have changed our (kthreadd's) priority or CPU mask.
> -		 * The kernel thread should not inherit these properties.
> -		 */
> -		sched_setscheduler_nocheck(task, SCHED_NORMAL, &param);
> -		set_cpus_allowed_ptr(task,
> -				     housekeeping_cpumask(HK_FLAG_KTHREAD));
>  	}
>  	kfree(create);
>  	return task;
> -- 
> 2.29.2
> 

Re: [PATCH 1/2] kthread: Move prio/affinite change into the newly created thread

[Sebastian Andrzej Siewior]

On 2020-11-17 13:45:03 [+0100], Peter Zijlstra wrote:
> On Tue, Nov 10, 2020 at 12:38:47PM +0100, Sebastian Andrzej Siewior wrote:
> > With enabled threaded interrupts the nouveau driver reported the
> > following:
> > | Chain exists of:
> > |   &mm->mmap_lock#2 --> &device->mutex --> &cpuset_rwsem
> > |
> > |  Possible unsafe locking scenario:
> > |
> > |        CPU0                    CPU1
> > |        ----                    ----
> > |   lock(&cpuset_rwsem);
> > |                                lock(&device->mutex);
> > |                                lock(&cpuset_rwsem);
> > |   lock(&mm->mmap_lock#2);
> > 
> > The device->mutex is nvkm_device::mutex.
> > 
> > Unblocking the lockchain at `cpuset_rwsem' is probably the easiest thing
> > to do.
> > Move the priority reset to the start of the newly created thread.
> > 
> > Fixes: 710da3c8ea7df ("sched/core: Prevent race condition between cpuset and __sched_setscheduler()")
> > Reported-by: Mike Galbraith <efault@gmx.de>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Link: https://lkml.kernel.org/r/a23a826af7c108ea5651e73b8fbae5e653f16e86.camel@gmx.de
> 
> Moo... yes this is certainly the easiest solution, because nouveau is a
> horrible rats nest. But when I spoke to Greg KH about this, he suggested
> nouveau ought to be fixed.
> 
> Ben, I got terminally lost when trying to untangle nouvea init, is there
> any chance this can be fixed to not hold that nvkm_device::mutex thing
> while doing request_irq() ?

Ben, did you had a chance to peek at this?

> > ---
> >  kernel/kthread.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 933a625621b8d..4a31127c6efbf 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -243,6 +243,7 @@ EXPORT_SYMBOL_GPL(kthread_parkme);
> >  
> >  static int kthread(void *_create)
> >  {
> > +	static const struct sched_param param = { .sched_priority = 0 };
> >  	/* Copy data: it's on kthread's stack */
> >  	struct kthread_create_info *create = _create;
> >  	int (*threadfn)(void *data) = create->threadfn;
> > @@ -273,6 +274,13 @@ static int kthread(void *_create)
> >  	init_completion(&self->parked);
> >  	current->vfork_done = &self->exited;
> >  
> > +	/*
> > +	 * The new thread inherited kthreadd's priority and CPU mask. Reset
> > +	 * back to default in case they have been changed.
> > +	 */
> > +	sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
> > +	set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_FLAG_KTHREAD));
> > +
> >  	/* OK, tell user we're spawned, wait for stop or wakeup */
> >  	__set_current_state(TASK_UNINTERRUPTIBLE);
> >  	create->result = current;
> > @@ -370,7 +378,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
> >  	}
> >  	task = create->result;
> >  	if (!IS_ERR(task)) {
> > -		static const struct sched_param param = { .sched_priority = 0 };
> >  		char name[TASK_COMM_LEN];
> >  
> >  		/*
> > @@ -379,13 +386,6 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
> >  		 */
> >  		vsnprintf(name, sizeof(name), namefmt, args);
> >  		set_task_comm(task, name);
> > -		/*
> > -		 * root may have changed our (kthreadd's) priority or CPU mask.
> > -		 * The kernel thread should not inherit these properties.
> > -		 */
> > -		sched_setscheduler_nocheck(task, SCHED_NORMAL, &param);
> > -		set_cpus_allowed_ptr(task,
> > -				     housekeeping_cpumask(HK_FLAG_KTHREAD));
> >  	}
> >  	kfree(create);
> >  	return task;

Sebastian

Re: [PATCH 1/2] kthread: Move prio/affinite change into the newly created thread

[Thomas Gleixner]

On Tue, Nov 17 2020 at 13:45, Peter Zijlstra wrote:
> On Tue, Nov 10, 2020 at 12:38:47PM +0100, Sebastian Andrzej Siewior wrote:
>
> Moo... yes this is certainly the easiest solution, because nouveau is a
> horrible rats nest. But when I spoke to Greg KH about this, he suggested
> nouveau ought to be fixed.
>
> Ben, I got terminally lost when trying to untangle nouvea init, is there
> any chance this can be fixed to not hold that nvkm_device::mutex thing
> while doing request_irq() ?

OTOH, creating a dependency chain vs. cpuset_rwsem and whatever lock is
held by the caller via request_irq() or kthread_create() is not
necessarily restricted to the nivea driver. struct device::mutex (not
the nkvm_device::mutex) is always held when a driver is probed.

The cpuset_rwsem -> mmap_lock dependency is a given, so we're one step
away from a circular dependency vs. mmap_lock.

That was my reasoning to move the stuff out into the thread context.

Thanks,

        tglx

Re: [PATCH 1/2] kthread: Move prio/affinite change into the newly created thread

[Sebastian Andrzej Siewior]

On 2020-11-21 11:55:48 [+0100], Thomas Gleixner wrote:
> On Tue, Nov 17 2020 at 13:45, Peter Zijlstra wrote:
> > On Tue, Nov 10, 2020 at 12:38:47PM +0100, Sebastian Andrzej Siewior wrote:
> >
> > Moo... yes this is certainly the easiest solution, because nouveau is a
> > horrible rats nest. But when I spoke to Greg KH about this, he suggested
> > nouveau ought to be fixed.
> >
> > Ben, I got terminally lost when trying to untangle nouvea init, is there
> > any chance this can be fixed to not hold that nvkm_device::mutex thing
> > while doing request_irq() ?
> 
> OTOH, creating a dependency chain vs. cpuset_rwsem and whatever lock is
> held by the caller via request_irq() or kthread_create() is not
> necessarily restricted to the nivea driver. struct device::mutex (not
> the nkvm_device::mutex) is always held when a driver is probed.
> 
> The cpuset_rwsem -> mmap_lock dependency is a given, so we're one step
> away from a circular dependency vs. mmap_lock.
> 
> That was my reasoning to move the stuff out into the thread context.

Just a friendly ping that this is still in my queue…

Ben could please reply here stating your view of the situation?

> Thanks,
> 
>         tglx

Sebastian