linux-kernel - Re: [PATCH tip/core/rcu 02/18] rcu: Move rcu_report_exp

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, 7 Oct 2015 08:42:05 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>, dipankar@...ibm.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	josh@...htriplett.org, Thomas Gleixner <tglx@...utronix.de>,
	rostedt <rostedt@...dmis.org>, dhowells@...hat.com,
	edumazet@...gle.com, dvhart@...ux.intel.com, fweisbec@...il.com,
	oleg@...hat.com, bobby prani <bobby.prani@...il.com>
Subject: Re: [PATCH tip/core/rcu 02/18] rcu: Move rcu_report_exp_rnp() to
 allow consolidation

----- On Oct 7, 2015, at 3:51 AM, Peter Zijlstra peterz@...radead.org wrote:

> On Tue, Oct 06, 2015 at 01:58:50PM -0700, Paul E. McKenney wrote:
>> On Tue, Oct 06, 2015 at 10:29:37PM +0200, Peter Zijlstra wrote:
>> > On Tue, Oct 06, 2015 at 09:29:21AM -0700, Paul E. McKenney wrote:
>> > > +static void __maybe_unused rcu_report_exp_rnp(struct rcu_state *rsp,
>> > > +					      struct rcu_node *rnp, bool wake)
>> > > +{
>> > > +	unsigned long flags;
>> > > +	unsigned long mask;
>> > > +
>> > > +	raw_spin_lock_irqsave(&rnp->lock, flags);
>> > 
>> > Normally we require a comment with barriers, explaining the order and
>> > the pairing etc.. :-)
>> > 
>> > > +	smp_mb__after_unlock_lock();
>> 
>> Hmmmm...  That is not good.
>> 
>> Worse yet, I am missing comments on most of the pre-existing barriers
>> of this form.
> 
> Yes I noticed.. :/
> 
>> The purpose is to enforce the heavy-weight grace-period memory-ordering
>> guarantees documented in the synchronize_sched() header comment and
>> elsewhere.
> 
>> They pair with anything you might use to check for violation
>> of these guarantees, or, simiarly, any ordering that you might use when
>> relying on these guarantees.
> 
> I'm sure you know what that means, but I've no clue ;-) That is, I
> wouldn't know where to start looking in the RCU implementation to verify
> the barrier is either needed or sufficient. Unless you mean _everywhere_
> :-)

One example is the new membarrier system call. It relies on synchronize_sched()
to enforce this:

from kernel/membarrier.c:

 * All memory accesses performed in program order from each targeted thread
 * is guaranteed to be ordered with respect to sys_membarrier(). If we use
 * the semantic "barrier()" to represent a compiler barrier forcing memory
 * accesses to be performed in program order across the barrier, and
 * smp_mb() to represent explicit memory barriers forcing full memory
 * ordering across the barrier, we have the following ordering table for
 * each pair of barrier(), sys_membarrier() and smp_mb():
 *
 * The pair ordering is detailed as (O: ordered, X: not ordered):
 *
 *                        barrier()   smp_mb() sys_membarrier()
 *        barrier()          X           X            O
 *        smp_mb()           X           O            O
 *        sys_membarrier()   O           O            O

And include/uapi/linux/membarrier.h:

 * @MEMBARRIER_CMD_SHARED:  Execute a memory barrier on all running threads.
 *                          Upon return from system call, the caller thread
 *                          is ensured that all running threads have passed
 *                          through a state where all memory accesses to
 *                          user-space addresses match program order between
 *                          entry to and return from the system call
 *                          (non-running threads are de facto in such a
 *                          state). This covers threads from all processes
 *                          running on the system. This command returns 0.

I hope this sheds light on a userspace-facing interface to
synchronize_sched() and clarifies its expected semantic a bit.

Thanks,

Mathieu

> 
>> I could add something like  "/* Enforce GP memory ordering. */"
>> 
>> Or perhaps "/* See synchronize_sched() header. */"
>> 
>> I do not propose reproducing the synchronize_sched() header on each
>> of these.  That would be verbose, even for me!  ;-)
>> 
>> Other thoughts?
> 
> Well, this is an UNLOCK+LOCK on non-matching lock variables upgrade to
> full barrier thing, right?
> 
> To me its not clear which UNLOCK we even match here. I've just read the
> sync_sched() header, but that doesn't help me either, so referring to
> that isn't really helpful either.
> 
> In any case, I don't want to make too big a fuzz here, but I just
> stumbled over a lot of unannotated barriers and figured I ought to say
> something about it.

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/