linux-kernel - Re: spin_lock implicit/explicit memory barrier

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, 15 Aug 2016 13:28:39 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Manfred Spraul <manfred@...orfullife.com>
Cc:	Davidlohr Bueso <dave@...olabs.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Susanne Spraul <1vier1@....de>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: spin_lock implicit/explicit memory barrier

On Mon, Aug 15, 2016 at 10:06:39PM +0200, Manfred Spraul wrote:
> Hi Paul,
> 
> On 08/10/2016 11:00 PM, Paul E. McKenney wrote:
> >On Wed, Aug 10, 2016 at 12:17:57PM -0700, Davidlohr Bueso wrote:
> >>[...]
> >>   CPU0			      CPU1
> >>   complex_mode = true	      spin_lock(l)
> >>   smp_mb()				  <--- do we want a smp_mb() here?
> >>   spin_unlock_wait(l)	      if (!smp_load_acquire(complex_mode))
> >>   foo()			 foo()
> >>
> >>We should not be doing an smp_mb() right after a spin_lock(), makes no sense. The
> >>spinlock machinery should guarantee us the barriers in the unorthodox locking cases,
> >>such as this.
> >In this case, from what I can see, we do need a store-load fence.
> >That said, yes, it really should be smp_mb__after_unlock_lock() rather
> >than smp_mb().  So if this code pattern is both desired and legitimate,
> >the smp_mb__after_unlock_lock() definitions probably need to move out
> >of kernel/rcu/tree.h to barrier.h or some such.
> Can you explain the function name, why smp_mb__after_unlock_lock()?

When placed after a locking function, it provides full ordering for
all the accesses within this critical section against all the accesses
in the previous critical section for this lock.  In addition, it
provides full ordering for all accesses within this critical section
against all previous critical sections for all locks acquired by this
task/CPU.

In short, it acts on the prior lock in combination with some earlier
unlock, hence the name.

> I would have called it smp_mb__after_spin_lock().

It works on mutexes as well as spinlocks, for whatever that is worth.

> For ipc/sem.c, the use case is:
> [sorry, I only now notice that the mailer ate the formatting]:
> 
>  cpu 1: complex_mode_enter():
>     smp_store_mb(sma->complex_mode, true);
> 
>    for (i = 0; i < sma->sem_nsems; i++) {
>         sem = sma->sem_base + i;
>         spin_unlock_wait(&sem->lock);
>     }
> 
> cpu 2: sem_lock():
>         spin_lock(&sem->lock);
>         smp_mb();
>         if (!smp_load_acquire(&sma->complex_mode)) {
> 
> 
> What is forbidden is that both cpu1 and cpu2 proceed.

It looks to me that CPU 2's smp_mb() could be an
smp_mb__after_unlock_lock() in this case, although that does mean
defining its relationship to spin_unlock_wait() in general.  Use of
smp_mb__after_unlock_lock() would get rid of a memory barrier on many
architectures, while still guaranteeing full ordering.  Probably not
measurable at the system level, though.

							Thanx, Paul