linux-kernel - Re: [PATCH v2 tip/core/rcu 2/3] srcu: Force full grace-period ordering

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:   Mon, 23 Jan 2017 12:06:04 -0800
From:   Lance Roy <ldr709@...il.com>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     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, peterz@...radead.org,
        rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
        dvhart@...ux.intel.com, fweisbec@...il.com, oleg@...hat.com,
        bobby.prani@...il.com
Subject: Re: [PATCH v2 tip/core/rcu 2/3] srcu: Force full grace-period
 ordering

On Mon, 23 Jan 2017 11:12:07 -0800
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:

> On Mon, Jan 23, 2017 at 12:38:29AM -0800, Lance Roy wrote:
> > On Sun, 15 Jan 2017 14:42:34 -0800
> > "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:
> >   
> > > @@ -413,6 +415,8 @@ static void __synchronize_srcu(struct srcu_struct *sp,
> > > int trycount) 
> > >  	if (!done)
> > >  		wait_for_completion(&rcu.completion);
> > > +
> > > +	smp_mb(); /* Caller's later accesses after GP. */  
> > 
> > I think that this memory barrier is only necessary when done == false, as
> > otherwise srcu_advance_batches() should provide sufficient memory
> > ordering.  
> 
> Let me make sure that I understand your rationale here.
> 
> The idea is that although srcu_readers_active_idx_check() did a full
> memory barrier, this might have happened on some other CPU, which
> would not have provided ordering to the current CPU in the race case
> where current CPU didn't actually sleep.  (This can happen where the
> current task is preempted, and then is resumed just as the grace period
> completes.)
> 
> Or are you concerned about some other sequence of events?
Yes, the problem only occurs when the only full memory barrier is executed on a
different CPU.

> (I have moved the smp_mb() inside the "if (!done)" in the meantime.)
Thanks.

> > > @@ -587,6 +591,7 @@ static void srcu_invoke_callbacks(struct srcu_struct
> > > *sp) int i;
> > >  	struct rcu_head *head;
> > >  
> > > +	smp_mb(); /* Callback accesses after GP. */  
> > 
> > Shouldn't srcu_advance_batches() have already run all necessary memory
> > barriers?  
> 
> It does look that way:
> 
> o	process_srcu() is the only thing that invokes
> srcu_invoke_callbacks().
> 
> o	process_srcu() invokes srcu_advance_batches() immediately before
> 	srcu_invoke_callbacks(), so any memory barriers invoked from
> 	srcu_advance_batches() affect process_srcu() (unlike the earlier
> 	example where srcu_advance_batches() might be executed in the
> 	context of some other task).
> 
> o	srcu_advance_batches() unconditionally invokes try_check_zero(),
> 	which in turn unconditionally invokes srcu_readers_active_idx_check(),
> 	which in turn invokes smp_mb().
> 
> 	This smp_mb() precedes a successful check that all pre-existing
> 	readers are done, otherwise srcu_advance_batches() won't have
> 	returned (or won't have advanced the callbacks, which in turn
> 	will prevent them from being invoked).
> 
> I have removed this memory barrier and added a comment.
> 
> And thank you for your review and comments!!!

Thanks,
Lance