linux-kernel - Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

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, 13 Jul 2010 09:34:33 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Avi Kivity <avi@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	"H. Peter Anvin" <hpa@...or.com>
Cc:	Peter Palfrader <peter@...frader.org>, Greg KH <gregkh@...e.de>,
	linux-kernel@...r.kernel.org, stable@...nel.org,
	stable-review@...nel.org, akpm@...ux-foundation.org,
	alan@...rguk.ukuu.org.uk, Glauber Costa <glommer@...hat.com>,
	Zachary Amsden <zamsden@...hat.com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Marcelo Tosatti <mtosatti@...hat.com>
Subject: Re: [patch 134/149] x86, paravirt: Add a global synchronization point 
	for pvclock

On Tue, Jul 13, 2010 at 8:57 AM, Avi Kivity <avi@...hat.com> wrote:
> On 07/13/2010 05:19 PM, Peter Palfrader wrote:
>>
>>> So it looks like last_value was placed in a read only section.  Please
>>> post your System.map somewhere.
>>>
>>
>> weasel@...repid:~$ publish System.map
>>
>> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
>> weasel@...repid:~$ grep -i last_value System.map
>> ffffffff81712e80 r last_value
>> ffffffff81b05240 b last_value.26163
>>
>
> "r" = "read only"
>
> How does it look in 'nm arch/x86/kernel/pvclock.o'?

I bet it is the same. And I have a suspicion: because the only write
access to that variable is in an asm that uses the "memory" clobber to
say it wrote to it (rather than say it writes to it directly), and
because the variable is marked 'static', gcc decides that nothing ever
writes to it in that compilation unit, and it can be made read-only.

Look at our definition for "xchg()" in
arch/x86/include/asm/cmpxchg_64.h. It boils down to

                asm volatile("xchgq %0,%1"                              \
                             : "=r" (__x)                               \
                             : "m" (*__xg(ptr)), "0" (__x)              \
                             : "memory");                               \

for the 8-byte case (which is obviously what atomic64_xchg() uses).
And the _reason_ we do that thing where we use a memory _input_ and
then a clobber is that older versions of gcc did not accept the thing
we _want_ to use, namely using "+m" to say that we actually change the
memory.  So the above is "wrong", but has historical reasons - and
it's apparently never been changed.

However, the "+m" was fixed, and we use it elsewhere, so I think the
"m" plus memory clobber is now purely historical. Does a patch
something like the appended fix it? I also suspect we should look at
some other uses in this area. The atomic64_64.h file uses "=m" and
"m", which looks like another legacy thing (again, "+m" historically
wasn't allowed, and then later became the 'correct' way to do things).

NOTE NOTE NOTE! This is UNTESTED and INCOMPLETE. We should do cmpxchg
too, and the 32-bit versions. I'm adding Ingo and Peter to the cc.

                     Linus

Download attachment "diff" of type "application/octet-stream" (1363 bytes)