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 21:04:54 +0300
From:	Avi Kivity <avi@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	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 07/13/2010 07:34 PM, Linus Torvalds wrote:
>
> 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).
>
>    

Well, current upstream uses "m":

>         case 8:                                                         \
>                 asm volatile(lock "cmpxchgq %1,%2"                      \
>                              : "=a"(__ret)                              \
>                              : "r"(__new), "m"(*__xg(ptr)), "0"(__old)  \
>                              : "memory");                               \
>                 break;                                                  \

and works; I also failed to reproduce with 2.6.32.16.  So I expect some 
toolchain involvement.

Peter, what gcc are you using?


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
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/