[PATCH] sched/core: Add missing completion for affine_move_task() waiters

[PATCH] sched/core: Add missing completion for affine_move_task() waiters

[Valentin Schneider]

Qian reported that some fuzzer issuing sched_setaffinity() ends up stuck on
a wait_for_completion(). The problematic pattern seems to be:

  affine_move_task()
      // task_running() case
      stop_one_cpu();
      wait_for_completion(&pending->done);

Combined with, on the stopper side:

  migration_cpu_stop()
    // Task moved between unlocks and scheduling the stopper
    task_rq(p) != rq &&
    // task_running() case
    dest_cpu >= 0

    => no complete_all()

This can happen with both PREEMPT and !PREEMPT, although !PREEMPT should
be more likely to see this given the targeted task has a much bigger window
to block and be woken up elsewhere before the stopper runs.

Make migration_cpu_stop() always look at pending affinity requests; signal
their completion if the stopper hits a rq mismatch but the task is
still within its allowed mask. When Migrate-Disable isn't involved, this
matches the previous set_cpus_allowed_ptr() vs migration_cpu_stop()
behaviour.

Link: https://lore.kernel.org/lkml/8b62fd1ad1b18def27f18e2ee2df3ff5b36d0762.camel@redhat.com
Fixes: 6d337eab041d ("sched: Fix migrate_disable() vs set_cpus_allowed_ptr()")
Reported-by: Qian Cai <cai@redhat.com>
Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
---
 kernel/sched/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 02076e6d3792..fad0a8e62aca 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1923,7 +1923,7 @@ static int migration_cpu_stop(void *data)
 		else
 			p->wake_cpu = dest_cpu;
 
-	} else if (dest_cpu < 0) {
+	} else if (dest_cpu < 0 || pending) {
 		/*
 		 * This happens when we get migrated between migrate_enable()'s
 		 * preempt_enable() and scheduling the stopper task. At that
@@ -1933,6 +1933,17 @@ static int migration_cpu_stop(void *data)
 		 * more likely.
 		 */
 
+		/*
+		 * The task moved before the stopper got to run. We're holding
+		 * ->pi_lock, so the allowed mask is stable - if it got
+		 * somewhere allowed, we're done.
+		 */
+		if (pending && cpumask_test_cpu(task_cpu(p), p->cpus_ptr)) {
+			p->migration_pending = NULL;
+			complete = true;
+			goto out;
+		}
+
 		/*
 		 * When this was migrate_enable() but we no longer have an
 		 * @pending, a concurrent SCA 'fixed' things and we should be
-- 
2.27.0

Re: [PATCH] sched/core: Add missing completion for affine_move_task() waiters

[Tao Zhou]

Hi,

On Fri, Nov 13, 2020 at 11:24:14AM +0000, Valentin Schneider wrote:

> Qian reported that some fuzzer issuing sched_setaffinity() ends up stuck on
> a wait_for_completion(). The problematic pattern seems to be:
>   affine_move_task()
>       // task_running() case
>       stop_one_cpu();
>       wait_for_completion(&pending->done);
> 
> Combined with, on the stopper side:
> 
>   migration_cpu_stop()
>     // Task moved between unlocks and scheduling the stopper
>     task_rq(p) != rq &&
>     // task_running() case
>     dest_cpu >= 0
> 
>     => no complete_all()
> 
> This can happen with both PREEMPT and !PREEMPT, although !PREEMPT should
> be more likely to see this given the targeted task has a much bigger window
> to block and be woken up elsewhere before the stopper runs.
> 
> Make migration_cpu_stop() always look at pending affinity requests; signal
> their completion if the stopper hits a rq mismatch but the task is
> still within its allowed mask. When Migrate-Disable isn't involved, this
> matches the previous set_cpus_allowed_ptr() vs migration_cpu_stop()
> behaviour.
> 
> Link: https://lore.kernel.org/lkml/8b62fd1ad1b18def27f18e2ee2df3ff5b36d0762.camel@redhat.com
> Fixes: 6d337eab041d ("sched: Fix migrate_disable() vs set_cpus_allowed_ptr()")
> Reported-by: Qian Cai <cai@redhat.com>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
>  kernel/sched/core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 02076e6d3792..fad0a8e62aca 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1923,7 +1923,7 @@ static int migration_cpu_stop(void *data)
>  		else
>  			p->wake_cpu = dest_cpu;
>  
> -	} else if (dest_cpu < 0) {
> +	} else if (dest_cpu < 0 || pending) {
>  		/*
>  		 * This happens when we get migrated between migrate_enable()'s
>  		 * preempt_enable() and scheduling the stopper task. At that
> @@ -1933,6 +1933,17 @@ static int migration_cpu_stop(void *data)
>  		 * more likely.
>  		 */
>  
> +		/*
> +		 * The task moved before the stopper got to run. We're holding
> +		 * ->pi_lock, so the allowed mask is stable - if it got
> +		 * somewhere allowed, we're done.
> +		 */
> +		if (pending && cpumask_test_cpu(task_cpu(p), p->cpus_ptr)) {
> +			p->migration_pending = NULL;
> +			complete = true;
> +			goto out;
> +		}
> +
>  		/*
>  		 * When this was migrate_enable() but we no longer have an
>  		 * @pending, a concurrent SCA 'fixed' things and we should be
> -- 
> 2.27.0

Oh, I did not receive this patch from 'ouwen210@hotmail.com'
account. Checked that you sent the patch to that mail address
from web. If 'ouwen210' is not a good mail account name(I have
used this name since 2002), I will change to use this one(Now
is smooth enough and can go to lkml).

Thanks,
Tao

Re: [PATCH] sched/core: Add missing completion for affine_move_task() waiters

[Valentin Schneider]

On 15/11/20 11:59, Tao Zhou wrote:
> Oh, I did not receive this patch from 'ouwen210@hotmail.com'
> account. Checked that you sent the patch to that mail address
> from web. If 'ouwen210' is not a good mail account name(I have
> used this name since 2002), I will change to use this one(Now
> is smooth enough and can go to lkml).
>

Nah, any address is fine. I grabbed whatever emails were cc'd to the patch
this is fixing - just make sure you're using the right address (i.e. the
one you want LKML stuff to go to) when interacting on the list and you
should be good.

> Thanks,
> Tao