linux-kernel - Re: [PATCH] mm: slub: Ensure that slab

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, 9 Mar 2016 18:52:45 +0530
From:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	<linux-parisc@...r.kernel>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Helge Deller <deller@....de>, <linux-kernel@...r.kernel.org>,
	<stable@...r.kernel.org>,
	"James E.J. Bottomley" <jejb@...isc-linux.org>,
	Pekka Enberg <penberg@...nel.org>, <linux-mm@...ck.org>,
	Noam Camus <noamc@...hip.com>,
	David Rientjes <rientjes@...gle.com>,
	Christoph Lameter <cl@...ux.com>,
	<linux-snps-arc@...ts.infradead.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] mm: slub: Ensure that slab_unlock() is atomic

On Wednesday 09 March 2016 03:43 PM, Peter Zijlstra wrote:
>> There is clearly a problem in slub code that it is pairing a test_and_set_bit()
>> with a __clear_bit(). Latter can obviously clobber former if they are not a single
>> instruction each unlike x86 or they use llock/scond kind of instructions where the
>> interim store from other core is detected and causes a retry of whole llock/scond
>> sequence.
> 
> Yes, test_and_set_bit() + __clear_bit() is broken.

But in SLUB: bit_spin_lock() + __bit_spin_unlock() is acceptable ? How so
(ignoring the performance thing for discussion sake, which is a side effect of
this implementation).

So despite the comment below in bit_spinlock.h I don't quite comprehend how this
is allowable. And if say, by deduction, this is fine for LLSC or lock prefixed
cases, then isn't this true in general for lot more cases in kernel, i.e. pairing
atomic lock with non-atomic unlock ? I'm missing something !

| /*
|  *  bit-based spin_unlock()
|  *  non-atomic version, which can be used eg. if the bit lock itself is
|  *  protecting the rest of the flags in the word.
|  */
| static inline void __bit_spin_unlock(int bitnum, unsigned long *addr)