linux-kernel - Re: [RFC PATCH 2/3] sched: membarrier: cover kthread_use

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]

Message-ID: <20201007150704.GH2628@hirez.programming.kicks-ass.net>
Date:   Wed, 7 Oct 2020 17:07:04 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
        Will Deacon <will@...nel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Andy Lutomirski <luto@...capital.net>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH 2/3] sched: membarrier: cover kthread_use_mm (v3)

On Thu, Sep 24, 2020 at 01:25:07PM -0400, Mathieu Desnoyers wrote:

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 2d95dc3f4644..bab6f4f2809f 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3736,6 +3736,8 @@ context_switch(struct rq *rq, struct task_struct *prev,
>  	 */
>  	arch_start_context_switch(prev);
>  
> +	membarrier_switch_mm(rq, prev->mm, next->mm);
> +
>  	/*
>  	 * kernel -> kernel   lazy + transfer active
>  	 *   user -> kernel   lazy + mmgrab() active
> @@ -3752,7 +3754,6 @@ context_switch(struct rq *rq, struct task_struct *prev,
>  		else
>  			prev->active_mm = NULL;
>  	} else {                                        // to user
> -		membarrier_switch_mm(rq, prev->active_mm, next->mm);
>  		/*
>  		 * sys_membarrier() requires an smp_mb() between setting
>  		 * rq->curr / membarrier_switch_mm() and returning to userspace.

I was thinking... do we need the above, when:

> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> index 8bc8b8a888b7..e5246580201b 100644
> --- a/kernel/sched/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -112,13 +112,9 @@ static int membarrier_global_expedited(void)
>  		    MEMBARRIER_STATE_GLOBAL_EXPEDITED))
>  			continue;
>  
> -		/*
> -		 * Skip the CPU if it runs a kernel thread. The scheduler
> -		 * leaves the prior task mm in place as an optimization when
> -		 * scheduling a kthread.
> -		 */
> +		/* Skip the CPU if it runs the idle thread. */
>  		p = rcu_dereference(cpu_rq(cpu)->curr);
> -		if (p->flags & PF_KTHREAD)

We retain this in the form:

		if ((p->flags & PF_KTHREAD) && !p-mm)
			continue;

> +		if (is_idle_task(p))
>  			continue;
>  
>  		__cpumask_set_cpu(cpu, tmpmask);

Specifically, we only care about kthreads when they're between
kthread_use_mm() / kthread_unuse_mm(), and in that case they will have
updated state already.

It's too late in the day to be sure about the memory ordering though;
but if we see !->mm, they'll do/have-done switch_mm() which implies
sufficient barriers().

Hmm?