linux-kernel - Re: [PATCH 2/2] rcu-tasks: Eliminate deadlocks involving do

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] [day] [month] [year] [list]

Message-ID: <ZcSwPifss+ho3hRt@lothringen>
Date: Thu, 8 Feb 2024 11:43:10 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: "Paul E. McKenney" <paulmck@...nel.org>
Cc: Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
	rcu@...r.kernel.org, linux-doc@...r.kernel.org,
	Chen Zhongjin <chenzhongjin@...wei.com>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Daniel Bristot de Oliveira <bristot@...hat.com>,
	Valentin Schneider <vschneid@...hat.com>,
	Neeraj Upadhyay <neeraj.iitr10@...il.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Josh Triplett <josh@...htriplett.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Zqiang <qiang.zhang1211@...il.com>,
	Kent Overstreet <kent.overstreet@...ux.dev>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Heiko Carstens <hca@...ux.ibm.com>, Arnd Bergmann <arnd@...db.de>,
	Oleg Nesterov <oleg@...hat.com>,
	Christian Brauner <brauner@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Mike Christie <michael.christie@...cle.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Mateusz Guzik <mjguzik@...il.com>,
	Nicholas Piggin <npiggin@...il.com>,
	Peng Zhang <zhangpeng.00@...edance.com>
Subject: Re: [PATCH 2/2] rcu-tasks: Eliminate deadlocks involving do_exit()
 and RCU tasks

On Thu, Feb 08, 2024 at 01:56:10AM -0800, Paul E. McKenney wrote:
> On Thu, Feb 08, 2024 at 03:10:32AM +0100, Frederic Weisbecker wrote:
> This ordering is not needed.  The lock orders addition to this
> list against removal from tasklist.  If we hold this lock, either
> the task is already on this list or our holding this lock prevents
> it from removing itself from the tasklist.
> 
> We have already scanned the task list, and we have already done
> whatever update we are worried about.
> 
> So, if the task was on the tasklist when we scanned, well and
> good.  If the task was created after we scanned the tasklist,
> then it cannot possibly access whatever we removed.
> 
> But please double-check!!!

Heh, right, another new pattern for me to discover :-/

C r-LOCK

{
}

P0(spinlock_t *LOCK, int *X, int *Y)
{
	int r1;
	int r2;
	
	r1 = READ_ONCE(*X);

	spin_lock(LOCK);
	r2 = READ_ONCE(*Y);
	spin_unlock(LOCK);
}

P1(spinlock_t *LOCK, int *X, int *Y)
{
	spin_lock(LOCK);
	WRITE_ONCE(*Y, 1);
	spin_unlock(LOCK);
	WRITE_ONCE(*X, 1);
}

exists (0:r1=1 /\ 0:r2=0) (* never *)


> 
> > > synchronize_rcu_tasks()                       do_exit()
> > > ----------------------                        ---------
> > > //for_each_process_thread()
> > > READ tasklist                                 WRITE rtpcp->rtp_exit_list
> > > LOCK rtpcp->lock                              UNLOCK rtpcp->lock
> > > smp_mb__after_unlock_lock()                   WRITE tasklist //unhash_process()
> > > READ rtpcp->rtp_exit_list
> > > 
> > > Does this work? Hmm, I'll play with litmus once I have a fresh brain...
> 
> First, thank you very much for the review!!!
> 
> > ie: does smp_mb__after_unlock_lock() order only what precedes the UNLOCK with
> > the UNLOCK itself? (but then the UNLOCK itself can be reordered with anything
> > that follows)? Or does it also order what follows the UNLOCK with the UNLOCK
> > itself? If both, then it looks ok, otherwise...
> 
> If you have this:
> 
> 	earlier_accesses();
> 	spin_lock(...);
> 	ill_considered_memory_accesses();
> 	smp_mb__after_unlock_lock();
> 	later_accesses();
> 
> Then earlier_accesses() will be ordered against later_accesses(), but
> ill_considered_memory_accesses() won't necessarily be ordered.  Also,
> any accesses before any prior release of that same lock will be ordered
> against later_accesses().
> 
> (In real life, ill_considered_memory_accesses() will be fully ordered
> against either spin_lock() on the one hand or smp_mb__after_unlock_lock()
> on the other, with x86 doing the first and PowerPC doing the second.
> So please try to avoid any ill_considered_memory_accesses().)

Thanks a lot for that explanation!


> 
> > Also on the other end, does LOCK/smp_mb__after_unlock_lock() order against what
> > precedes the LOCK? That also is necessary for the above to work.
> 
> It looks like an smp_mb__after_spinlock() would also be needed, for
> example, on ARMv8.
> 
> > Of course by the time I'm writing this email, litmus would have told me
> > already...
> 
> ;-) ;-) ;-)
> 
> But I believe that simple locking covers this case.  Famous last words...

Indeed, looks right!

Thanks!
> 							Thanx, Paul