linux-kernel - Re: [RFC PATCH] sched/core: Fix premature p->migration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Feb 2021 17:23:44 +0000
From:   Qais Yousef <qais.yousef@....com>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...nel.org,
        bigeasy@...utronix.de, swood@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vincent.donnefort@....com,
        tj@...nel.org
Subject: Re: [RFC PATCH] sched/core: Fix premature p->migration_pending
 completion

On 01/27/21 19:30, Valentin Schneider wrote:
> Fiddling some more with a TLA+ model of set_cpus_allowed_ptr() & friends
> unearthed one more outstanding issue. This doesn't even involve
> migrate_disable(), but rather affinity changes and execution of the stopper
> racing with each other.
> 
> My own interpretation of the (lengthy) TLA+ splat (note the potential for
> errors at each level) is:
> 
>   Initial conditions:
>     victim.cpus_mask = {CPU0, CPU1}
> 
>   CPU0                             CPU1                             CPU<don't care>
> 
>   switch_to(victim)
> 								    set_cpus_allowed(victim, {CPU1})
> 								      kick CPU0 migration_cpu_stop({.dest_cpu = CPU1})
>   switch_to(stopper/0)
> 								    // e.g. CFS load balance
> 								    move_queued_task(CPU0, victim, CPU1);
> 				   switch_to(victim)
> 								    set_cpus_allowed(victim, {CPU0});
> 								      task_rq_unlock();
>   migration_cpu_stop(dest_cpu=CPU1)

This migration stop is due to set_cpus_allowed(victim, {CPU1}), right?

>     task_rq(p) != rq && pending
>       kick CPU1 migration_cpu_stop({.dest_cpu = CPU1})
> 
> 				   switch_to(stopper/1)
> 				   migration_cpu_stop(dest_cpu=CPU1)

And this migration stop is due to set_cpus_allowed(victim, {CPU0}), right?

If I didn't miss something, then dest_cpu should be CPU0 too, not CPU1 and the
task should be moved back to CPU0 as expected?

Thanks

--
Qais Yousef

> 				     task_rq(p) == rq && pending
> 				       __migrate_task(dest_cpu) // no-op
> 				     complete_all() <-- !!! affinity is {CPU0} !!!
> 
> I believe there are two issues there:
> - retriggering of migration_cpu_stop() from within migration_cpu_stop()
>   itself doesn't change arg.dest_cpu
> - we'll issue a complete_all() in the task_rq(p) == rq path of
>   migration_cpu_stop() even if the dest_cpu has been superseded by a
>   further affinity change.
> 
> Something similar could happen with NUMA's migrate_task_to(), and arguably
> any other user of migration_cpu_stop() with a .dest_cpu >= 0.
> Consider:
> 
>   CPU0					CPUX
> 
>   switch_to(victim)
> 					migrate_task_to(victim, CPU1)
> 					  kick CPU0 migration_cpu_stop({.dest_cpu = CPU1})
> 
> 					set_cpus_allowed(victim, {CPU42})
> 					  task_rq_unlock();
>   switch_to(stopper/0)
>   migration_cpu_stop(dest_cpu=CPU1)
>     task_rq(p) == rq && pending
>       __migrate_task(dest_cpu)
>     complete_all() <-- !!! affinity is {CPU42} !!!
> 
> Prevent such premature completions by ensuring the dest_cpu in
> migration_cpu_stop() is in the task's allowed cpumask.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@....com>
> ---
>  kernel/sched/core.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 06b449942adf..b57326b0a742 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1923,20 +1923,28 @@ static int migration_cpu_stop(void *data)
>  			complete = true;
>  		}
>  
> -		/* migrate_enable() --  we must not race against SCA */
> -		if (dest_cpu < 0) {
> -			/*
> -			 * When this was migrate_enable() but we no longer
> -			 * have a @pending, a concurrent SCA 'fixed' things
> -			 * and we should be valid again. Nothing to do.
> -			 */
> -			if (!pending) {
> -				WARN_ON_ONCE(!cpumask_test_cpu(task_cpu(p), &p->cpus_mask));
> -				goto out;
> -			}
> +	       /*
> +		* When this was migrate_enable() but we no longer
> +		* have a @pending, a concurrent SCA 'fixed' things
> +		* and we should be valid again.
> +		*
> +		* This can also be a stopper invocation that was 'fixed' by an
> +		* earlier one.
> +		*
> +		* Nothing to do.
> +		*/
> +		if ((dest_cpu < 0 || dest_cpu == cpu_of(rq)) && !pending) {
> +			WARN_ON_ONCE(!cpumask_test_cpu(task_cpu(p), &p->cpus_mask));
> +			goto out;
> +		}
>  
> +		/*
> +		 * Catch any affinity change between the stop_cpu() call and us
> +		 * getting here.
> +		 * For migrate_enable(), we just want to pick an allowed one.
> +		 */
> +		if (dest_cpu < 0 || !cpumask_test_cpu(dest_cpu, &p->cpus_mask))
>  			dest_cpu = cpumask_any_distribute(&p->cpus_mask);
> -		}
>  
>  		if (task_on_rq_queued(p))
>  			rq = __migrate_task(rq, &rf, p, dest_cpu);
> -- 
> 2.27.0
>