linux-kernel - Re: [PATCH tip/core/rcu 4/5] sys_membarrier: Add expedited option

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: <20170727142955.bb2bdgbjcm745xwo@tardis>
Date:   Thu, 27 Jul 2017 22:29:55 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, rostedt@...dmis.org,
        dhowells@...hat.com, edumazet@...gle.com, fweisbec@...il.com,
        oleg@...hat.com, will.deacon@....com
Subject: Re: [PATCH tip/core/rcu 4/5] sys_membarrier: Add expedited option

On Thu, Jul 27, 2017 at 07:16:33AM -0700, Paul E. McKenney wrote:
> On Thu, Jul 27, 2017 at 09:55:51PM +0800, Boqun Feng wrote:
> > Hi Paul,
> > 
> > I have a side question out of curiosity:
> > 
> > How does synchronize_sched() work properly for sys_membarrier()?
> > 
> > sys_membarrier() requires every other CPU does a smp_mb() before it
> > returns, and I know synchronize_sched() will wait until all CPUs running
> > a kernel thread do a context-switch, which has a smp_mb(). However, I
> > believe sched flavor RCU treat CPU running a user thread as a quiesent
> > state, so synchronize_sched() could return without that CPU does a
> > context switch. 
> > 
> > So why could we use synchronize_sched() for sys_membarrier()?
> > 
> > In particular, could the following happens?
> > 
> > 	CPU 0:				CPU 1:
> > 	=========================	==========================
> > 	<in user space>			<in user space>
> > 					{read Y}(reordered) <------------------------------+
> > 	store Y;			                                                   |
> > 					read X; --------------------------------------+    |
> > 	sys_membarrier():		<timer interrupt>                             |    |
> > 	  synchronize_sched();		  update_process_times(user): //user == true  |    |
> > 	  				    rcu_check_callbacks(usr):                 |    |
> > 					      if (user || ..) {                       |    |
> > 					        rcu_sched_qs()                        |    |
> > 						...                                   |    |
> > 						<report quesient state in softirq>    |    |
> 
> The reporting of the quiescent state will acquire the leaf rcu_node
> structure's lock, with an smp_mb__after_unlock_lock(), which will
> one way or another be a full memory barrier.  So the reorderings
> cannot happen.
> 
> Unless I am missing something subtle.  ;-)
> 

Well, smp_mb__after_unlock_lock() in ARM64 is a no-op, and ARM64's lock
doesn't provide a smp_mb().

So my point is more like: synchronize_sched() happens to be a
sys_membarrier() because of some implementation detail, and if some day
we come up with a much cheaper way to implement sched flavor
RCU(hopefully!), synchronize_sched() may be not good for the job. So at
least, we'd better document this somewhere?

Regards,
Boqun

> 						Thanx, Paul
> 
> > 					<return to user space>                        |    |
> > 					read Y; --------------------------------------+----+
> > 	store X;			                                              |
> > 					{read X}(reordered) <-------------------------+
> > 
> > I assume the timer interrupt handler, which interrupts a user space and
> > reports a quiesent state for sched flavor RCU, may not have a smp_mb()
> > in some code path.
> > 
> > I may miss something subtle, but it just not very obvious how
> > synchronize_sched() will guarantee a remote CPU running in userspace to
> > do a smp_mb() before it returns, this is at least not in RCU
> > requirements, right?
> > 
> > Regards,
> > Boqun
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)