linux-kernel - Re: sem_lock() vs qspinlocks

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:	Sat, 21 May 2016 15:49:20 +0200
From:	Manfred Spraul <manfred@...orfullife.com>
To:	Peter Zijlstra <peterz@...radead.org>,
	Davidlohr Bueso <dave@...olabs.net>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Waiman Long <Waiman.Long@....com>,
	Ingo Molnar <mingo@...nel.org>, ggherdovich@...e.com,
	Mel Gorman <mgorman@...hsingularity.net>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Will Deacon <will.deacon@....com>, 1vier1@....de
Subject: Re: sem_lock() vs qspinlocks

On 05/21/2016 09:37 AM, Peter Zijlstra wrote:
> On Fri, May 20, 2016 at 05:48:39PM -0700, Davidlohr Bueso wrote:
>> As opposed to spin_is_locked(), spin_unlock_wait() is perhaps more tempting
>> to use for locking correctness. For example, taking a look at nf_conntrack_all_lock(),
>> it too likes to get smart with spin_unlock_wait() -- also for finer graining purposes.
>> While not identical to sems, it goes like:
>>
>> nf_conntrack_all_lock():	nf_conntrack_lock():
>> spin_lock(B);			spin_lock(A);
>>
>> 				if (bar) { // false
>> bar = 1;			   ...
>> 				}
>> [loop ctrl-barrier]				
>>   spin_unlock_wait(A);
>> foo();				foo();
>>
>> If the spin_unlock_wait() doesn't yet see the store that makes A visibly locked,
>> we could end up with both threads in foo(), no?. (Although I'm unsure about that
>> ctrl barrier and archs could fall into it. The point was to see in-tree examples
>> of creative thinking with locking).
> I'm tempted to put that trailing smp_rmb() in spin_unlock_wait() too;
> because I suspect the netfilter code is broken without it.
>
> And it seems intuitive to assume that if we return from unlock_wait() we
> can indeed observe the critical section we waited on.
Then !spin_is_locked() and spin_unlock_wait() would be different with 
regards to memory barriers.
Would that really help?

My old plan was to document the rules, and define a generic 
smp_acquire__after_spin_is_unlocked.
https://lkml.org/lkml/2015/3/1/153

Noone supported it, so it ended up as 
ipc_smp_acquire__after_spin_is_unlocked().
Should we move it to linux/spinlock.h?

Who needs it?
- ipc/sem.c (but please start from the version from linux-next as 
reference, it is far less convoluted compared to the current code)
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/ipc/sem.c

- nf_conntrack

- task_rq_lock() perhaps needs smp_acquire__after_ctrl_dep
(I didn't figure out yet what happened to the proposed patch)
https://lkml.org/lkml/2015/2/17/129

--
     Manfred