linux-kernel - Re: [PATCH 6/8] i386: bitops: Don't mark memory as clobbered unnecessarily

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:	Tue, 24 Jul 2007 18:39:15 +1000
From:	Nick Piggin <nickpiggin@...oo.com.au>
To:	Satyam Sharma <ssatyam@....iitk.ac.in>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	David Howells <dhowells@...hat.com>, Andi Kleen <ak@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jeremy Fitzhardinge <jeremy@...p.org>
Subject: Re: [PATCH 6/8] i386: bitops: Don't mark memory as clobbered unnecessarily

Satyam Sharma wrote:
> On Tue, 24 Jul 2007, Nick Piggin wrote:
> 
> 
>>>>[...]
>>>>
>>>>__test_and_change_bit is one that you could remove the memory clobber
>>>>from.
>>>
>>>Yes, for the atomic versions we don't care if we're asking gcc to
>>>generate trashy code (even though I'd have wanted to only disallow
>>>problematic optimizations -- ones involving the passed bit-string
>>>address -- there, and allow other memory references to be optimized
>>>as and how the compiler feels like it) because the atomic variants
>>>are slow anyway and we probably want to be extra-safe there.
>>>
>>>But for the non-atomic variants, it does make sense to remove the
>>>memory clobber (and the unneeded __asm__ __volatile__ that another
>>>patch did -- for the non-atomic variants, again).
>>
>>No. It has nothing to do with atomicity and all to do with ordering.
> 
> 
> The memory clobber, or the volatile asm? There's more than one variable
> here ... but still, I don't think either affects _ordering_ in any
> _direct_ way.

The clobber which you remove with this patch.


>>For example test_bit, clear_bit, set_bit, etc are all atomic but none
>>place any restrictions on ordering.
> 
> 
> In that case we need to update comments in include/asm-i386/bitops.h

Hmm... yeah it looks like they could be reordered. I think?


>>__test_and_change_bit has no restriction on ordering, so as long as
>>the correct operands are clobbered, a "memory" clobber to enforce a
>>compiler barrier is not needed.
> 
> 
> But why even for the other operations? Consider (current code of)
> test_and_set_bit():
> 
> static inline int test_and_set_bit(int nr, volatile unsigned long * addr)
> {
> 	int oldbit;
> 	__asm__ __volatile__( LOCK_PREFIX
> 		"btsl %2,%1\n\tsbbl %0,%0"
> 		:"=r" (oldbit),"+m" (ADDR)
> 		:"Ir" (nr) : "memory");
> 
> 	return oldbit;
> }
> 
> The only memory reference in there is to the passed address, it will
> be modified, yes, but that's been made obvious to gcc in the asm
> already. So why are we marking all of memory as clobbered, is the
> question. (I just read Jeremy's latest reply, but I don't see how
> or why the memory clobber helps that case either -- does a memory
> clobber affect how gcc would order / reorder code?)

Of course, because then the compiler can't assume anything about
the contents of memory after the operation.

   #define barrier() __asm__ __volatile__("": : :"memory")

A memory clobber is equivalent to a compiler barrier.

-- 
SUSE Labs, Novell Inc.
-
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/