linux-kernel - Re: [tip: sched/urgent] sched/membarrier: Fix p->mm->membarrier

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: <20191001085033.GP4519@hirez.programming.kicks-ass.net>
Date:   Tue, 1 Oct 2019 10:50:33 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Chris Metcalf <cmetcalf@...hip.com>,
        Christoph Lameter <cl@...ux.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Kirill Tkhai <tkhai@...dex.ru>, Mike Galbraith <efault@....de>,
        Oleg Nesterov <oleg@...hat.com>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [tip: sched/urgent] sched/membarrier: Fix
 p->mm->membarrier_state racy load

On Tue, Oct 01, 2019 at 10:44:05AM +0200, Ingo Molnar wrote:
> 
> * tip-bot2 for Mathieu Desnoyers <tip-bot2@...utronix.de> wrote:
> 
> > The following commit has been merged into the sched/urgent branch of tip:
> > 
> > Commit-ID:     227a4aadc75ba22fcb6c4e1c078817b8cbaae4ce
> > Gitweb:        https://git.kernel.org/tip/227a4aadc75ba22fcb6c4e1c078817b8cbaae4ce
> > Author:        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > AuthorDate:    Thu, 19 Sep 2019 13:37:02 -04:00
> > Committer:     Ingo Molnar <mingo@...nel.org>
> > CommitterDate: Wed, 25 Sep 2019 17:42:30 +02:00
> > 
> > sched/membarrier: Fix p->mm->membarrier_state racy load
> 
> > +	rcu_read_unlock();
> >  	if (!fallback) {
> >  		preempt_disable();
> >  		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
> > @@ -136,6 +178,7 @@ static int membarrier_private_expedited(int flags)
> >  	}
> >  
> >  	cpus_read_lock();
> > +	rcu_read_lock();
> >  	for_each_online_cpu(cpu) {
> >  		struct task_struct *p;
> >  
> > @@ -157,8 +200,8 @@ static int membarrier_private_expedited(int flags)
> >  			else
> >  				smp_call_function_single(cpu, ipi_mb, NULL, 1);
> >  		}
> > -		rcu_read_unlock();
> >  	}
> > +	rcu_read_unlock();
> 
> I noticed this too late, but the locking in this part is now bogus:
> 
> 	rcu_read_lock();
> 	for_each_online_cpu(cpu) {
> 		struct task_struct *p;
> 
> 		/*
> 		 * Skipping the current CPU is OK even through we can be
> 		 * migrated at any point. The current CPU, at the point
> 		 * where we read raw_smp_processor_id(), is ensured to
> 		 * be in program order with respect to the caller
> 		 * thread. Therefore, we can skip this CPU from the
> 		 * iteration.
> 		 */
> 		if (cpu == raw_smp_processor_id())
> 			continue;
> 		rcu_read_lock();

Yeah, that one needs to go.

> 		p = rcu_dereference(cpu_rq(cpu)->curr);
> 		if (p && p->mm == mm)
> 			__cpumask_set_cpu(cpu, tmpmask);
> 	}
> 	rcu_read_unlock();
> 
> Note the double rcu_read_lock() ....
> 
> This bug is now upstream, so requires an urgent fix, as it should be 
> trivial to trigger with pretty much any membarrier user.

---
Subject: membarrier: Fix faulty merge

Commit 227a4aadc75b ("sched/membarrier: Fix p->mm->membarrier_state racy
load") got fat fingered by me when merging it with other patches. It
meant to move the rcu section out of the for loop but ended up doing it
partially, leaving a superfluous rcu_read_lock() inside, causing havok.

Fixes: 227a4aadc75b ("sched/membarrier: Fix p->mm->membarrier_state racy load")
Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
 kernel/sched/membarrier.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
index a39bed2c784f..168479a7d61b 100644
--- a/kernel/sched/membarrier.c
+++ b/kernel/sched/membarrier.c
@@ -174,7 +174,6 @@ static int membarrier_private_expedited(int flags)
 		 */
 		if (cpu == raw_smp_processor_id())
 			continue;
-		rcu_read_lock();
 		p = rcu_dereference(cpu_rq(cpu)->curr);
 		if (p && p->mm == mm)
 			__cpumask_set_cpu(cpu, tmpmask);