* [PATCH] tell gcc optimizer to never introduce new data races
@ 2014-06-10 13:23 Jiri Kosina
2014-06-10 14:53 ` Peter Zijlstra
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Jiri Kosina @ 2014-06-10 13:23 UTC (permalink / raw
To: Linus Torvalds, Paul E. McKenney, Peter Zijlstra, Andrew Morton
Cc: Martin Jambor, Petr Mladek, linux-kernel, gcc
We have been chasing a memory corruption bug, which turned out to be
caused by very old gcc (4.3.4), which happily turned conditional load into
a non-conditional one, and that broke correctness (the condition was met
only if lock was held) and corrupted memory.
This particular problem with that particular code did not happen when
never gccs were used. I've brought this up with our gcc folks, as I wanted
to make sure that this can't really happen again, and it turns out it
actually can.
Quoting Martin Jambor <mjambor@suse.cz>:
====
More current GCCs are more careful when it comes to replacing a
conditional load with a non-conditional one, most notably they check
that a store happens in each iteration of _a_ loop but they assume
loops are executed. They also perform a simple check whether the
store cannot trap which currently passes only for non-const
variables. A simple testcase demonstrating it on an x86_64 is for
example the following:
$ cat cond_store.c
int g_1 = 1;
int g_2[1024] __attribute__((section ("safe_section"), aligned (4096)));
int c = 4;
int __attribute__ ((noinline))
foo (void)
{
int l;
for (l = 0; (l != 4); l++) {
if (g_1)
return l;
for (g_2[0] = 0; (g_2[0] >= 26); ++g_2[0])
;
}
return 2;
}
int main (int argc, char* argv[])
{
if (mprotect (g_2, sizeof(g_2), PROT_READ) == -1)
{
int e = errno;
error (e, e, "mprotect error %i", e);
}
foo ();
__builtin_printf("OK\n");
return 0;
}
/* EOF */
$ ~/gcc/trunk/inst/bin/gcc cond_store.c -O2 --param allow-store-data-races=0
$ ./a.out
OK
$ ~/gcc/trunk/inst/bin/gcc cond_store.c -O2 --param allow-store-data-races=1
$ ./a.out
Segmentation fault
The testcase fails the same at least with 4.9, 4.8 and 4.7. Therefore
I would suggest building kernels with this parameter set to zero. I
also agree with Jikos that the default should be changed for -O2. I
have run most of the SPEC 2k6 CPU benchmarks (gamess and dealII
failed, at -O2, not sure why) compiled with and without this option
and did not see any real difference between respective run-times.
====
Hopefully the default will be changed in newer gccs, but let's force
it for kernel builds so that we are on a safe side even when older
gcc are used.
Cc: Martin Jambor <mjambor@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Makefile b/Makefile
index 00a933b..613367f 100644
--- a/Makefile
+++ b/Makefile
@@ -585,6 +585,9 @@ else
KBUILD_CFLAGS += -O2
endif
+# Tell gcc to never replace conditional load with a non-conditional one
+KBUILD_CFLAGS += $(call cc-option,--param allow-store-data-races=0)
+
include $(srctree)/arch/$(SRCARCH)/Makefile
ifdef CONFIG_READABLE_ASM
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 13:23 [PATCH] tell gcc optimizer to never introduce new data races Jiri Kosina
@ 2014-06-10 14:53 ` Peter Zijlstra
2014-06-10 15:04 ` Marek Polacek
2014-06-10 17:46 ` Linus Torvalds
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2014-06-10 14:53 UTC (permalink / raw
To: Jiri Kosina
Cc: Linus Torvalds, Paul E. McKenney, Andrew Morton, Martin Jambor,
Petr Mladek, linux-kernel, gcc
[-- Attachment #1: Type: text/plain, Size: 327 bytes --]
On Tue, Jun 10, 2014 at 03:23:36PM +0200, Jiri Kosina wrote:
> +# Tell gcc to never replace conditional load with a non-conditional one
> +KBUILD_CFLAGS += $(call cc-option,--param allow-store-data-races=0)
> +
Why do we not want: -fmemory-model=safe? And should we not at the very
least also disable packed-store-data-races?
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 14:53 ` Peter Zijlstra
@ 2014-06-10 15:04 ` Marek Polacek
2014-06-10 15:13 ` Peter Zijlstra
0 siblings, 1 reply; 15+ messages in thread
From: Marek Polacek @ 2014-06-10 15:04 UTC (permalink / raw
To: Peter Zijlstra
Cc: Jiri Kosina, Linus Torvalds, Paul E. McKenney, Andrew Morton,
Martin Jambor, Petr Mladek, linux-kernel, gcc
On Tue, Jun 10, 2014 at 04:53:27PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 10, 2014 at 03:23:36PM +0200, Jiri Kosina wrote:
> > +# Tell gcc to never replace conditional load with a non-conditional one
> > +KBUILD_CFLAGS += $(call cc-option,--param allow-store-data-races=0)
> > +
>
> Why do we not want: -fmemory-model=safe? And should we not at the very
> least also disable packed-store-data-races?
Note that the option does not exist, even though it is mentioned in the
documentation.
Marek
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 15:04 ` Marek Polacek
@ 2014-06-10 15:13 ` Peter Zijlstra
2014-06-10 15:17 ` Jakub Jelinek
0 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2014-06-10 15:13 UTC (permalink / raw
To: Marek Polacek
Cc: Jiri Kosina, Linus Torvalds, Paul E. McKenney, Andrew Morton,
Martin Jambor, Petr Mladek, linux-kernel, gcc
[-- Attachment #1: Type: text/plain, Size: 645 bytes --]
On Tue, Jun 10, 2014 at 05:04:55PM +0200, Marek Polacek wrote:
> On Tue, Jun 10, 2014 at 04:53:27PM +0200, Peter Zijlstra wrote:
> > On Tue, Jun 10, 2014 at 03:23:36PM +0200, Jiri Kosina wrote:
> > > +# Tell gcc to never replace conditional load with a non-conditional one
> > > +KBUILD_CFLAGS += $(call cc-option,--param allow-store-data-races=0)
> > > +
> >
> > Why do we not want: -fmemory-model=safe? And should we not at the very
> > least also disable packed-store-data-races?
>
> Note that the option does not exist, even though it is mentioned in the
> documentation.
Urgh.. ok. Any word on the packed-store-data thing?
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 15:13 ` Peter Zijlstra
@ 2014-06-10 15:17 ` Jakub Jelinek
0 siblings, 0 replies; 15+ messages in thread
From: Jakub Jelinek @ 2014-06-10 15:17 UTC (permalink / raw
To: Peter Zijlstra
Cc: Marek Polacek, Jiri Kosina, Linus Torvalds, Paul E. McKenney,
Andrew Morton, Martin Jambor, Petr Mladek, linux-kernel, gcc
On Tue, Jun 10, 2014 at 05:13:29PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 10, 2014 at 05:04:55PM +0200, Marek Polacek wrote:
> > On Tue, Jun 10, 2014 at 04:53:27PM +0200, Peter Zijlstra wrote:
> > > On Tue, Jun 10, 2014 at 03:23:36PM +0200, Jiri Kosina wrote:
> > > > +# Tell gcc to never replace conditional load with a non-conditional one
> > > > +KBUILD_CFLAGS += $(call cc-option,--param allow-store-data-races=0)
> > > > +
> > >
> > > Why do we not want: -fmemory-model=safe? And should we not at the very
> > > least also disable packed-store-data-races?
> >
> > Note that the option does not exist, even though it is mentioned in the
> > documentation.
>
> Urgh.. ok. Any word on the packed-store-data thing?
That is recognized, undocumented and never used in the compiler (not in 4.7
or any later release till now). Most of the spots in the compiler that
could introduce data races were actually just changed, there is (already
since 4.7) just a single conditional on the --param allow-store-data-races=X
value.
Jakub
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 13:23 [PATCH] tell gcc optimizer to never introduce new data races Jiri Kosina
2014-06-10 14:53 ` Peter Zijlstra
@ 2014-06-10 17:46 ` Linus Torvalds
2014-06-10 18:04 ` Steven Noonan
2014-06-10 18:10 ` Jiri Kosina
2014-06-16 10:29 ` Dan Carpenter
2014-08-20 23:27 ` Jiri Kosina
3 siblings, 2 replies; 15+ messages in thread
From: Linus Torvalds @ 2014-06-10 17:46 UTC (permalink / raw
To: Jiri Kosina
Cc: Paul E. McKenney, Peter Zijlstra, Andrew Morton, Martin Jambor,
Petr Mladek, Linux Kernel Mailing List, gcc@gcc.gnu.org
On Tue, Jun 10, 2014 at 6:23 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> We have been chasing a memory corruption bug, which turned out to be
> caused by very old gcc (4.3.4), which happily turned conditional load into
> a non-conditional one, and that broke correctness (the condition was met
> only if lock was held) and corrupted memory.
Just out of interest, can you point to the particular kernel code that
caused this? I think that's more interesting than the example program
you show - which I'm sure is really nice for gcc developers as an
example, but from a kernel standpoint I think it's more important to
show the particular problems this caused for the kernel?
Linus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 17:46 ` Linus Torvalds
@ 2014-06-10 18:04 ` Steven Noonan
2014-06-10 18:54 ` Richard Biener
2014-06-10 18:10 ` Jiri Kosina
1 sibling, 1 reply; 15+ messages in thread
From: Steven Noonan @ 2014-06-10 18:04 UTC (permalink / raw
To: Linus Torvalds
Cc: Jiri Kosina, Paul E. McKenney, Peter Zijlstra, Andrew Morton,
Martin Jambor, Petr Mladek, Linux Kernel Mailing List,
gcc@gcc.gnu.org
On Tue, Jun 10, 2014 at 10:46 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 10, 2014 at 6:23 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> We have been chasing a memory corruption bug, which turned out to be
>> caused by very old gcc (4.3.4), which happily turned conditional load into
>> a non-conditional one, and that broke correctness (the condition was met
>> only if lock was held) and corrupted memory.
>
> Just out of interest, can you point to the particular kernel code that
> caused this? I think that's more interesting than the example program
> you show - which I'm sure is really nice for gcc developers as an
> example, but from a kernel standpoint I think it's more important to
> show the particular problems this caused for the kernel?
>
Jiri, is there a workaround for compilers that don't support '--param
allow-store-data-races=0'? For example:
$ gcc-4.5 -O2 -o cond_store cond_store.c && ./cond_store
Segmentation fault (core dumped)
$ gcc-4.5 -O2 --param allow-store-data-races=0 -o cond_store
cond_store.c && ./cond_store
cc1: error: invalid parameter ‘allow-store-data-races’
$ gcc-4.5 -v
Using built-in specs.
COLLECT_GCC=gcc-4.5
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/4.5.4/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --libdir=/usr/lib --libexecdir=/usr/lib
--program-suffix=-4.5 --enable-shared
--enable-languages=c,c++,fortran,objc,obj-c++ --enable-__cxa_atexit
--disable-libstdcxx-pch --disable-multilib --disable-libgomp
--disable-libmudflap --disable-libssp --enable-clocale=gnu
--with-tune=generic --with-cloog --with-ppl --with-system-zlib
Thread model: posix
gcc version 4.5.4 (GCC)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 17:46 ` Linus Torvalds
2014-06-10 18:04 ` Steven Noonan
@ 2014-06-10 18:10 ` Jiri Kosina
1 sibling, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2014-06-10 18:10 UTC (permalink / raw
To: Linus Torvalds
Cc: Paul E. McKenney, Peter Zijlstra, Andrew Morton, Martin Jambor,
Petr Mladek, Linux Kernel Mailing List, gcc@gcc.gnu.org
On Tue, 10 Jun 2014, Linus Torvalds wrote:
> > We have been chasing a memory corruption bug, which turned out to be
> > caused by very old gcc (4.3.4), which happily turned conditional load into
> > a non-conditional one, and that broke correctness (the condition was met
> > only if lock was held) and corrupted memory.
>
> Just out of interest, can you point to the particular kernel code that
> caused this? I think that's more interesting than the example program
> you show - which I'm sure is really nice for gcc developers as an
> example, but from a kernel standpoint I think it's more important to
> show the particular problems this caused for the kernel?
Well, as I said, that was with gcc 4.3.4, and GCC people expressed
themselves that that was a slightly different optimization. It made me
nervous enough though to ask whether it's absolutely positively not going
to happen with newer gccs, and the code snippet quoted in the original
mail came back as a response.
The code in question was out-of-tree printk-in-NMI (yeah, surprise
suprise, once again) patch written by Petr Mladek, let me quote his
comment from our internal bugzilla:
===
I have spent few days investigating inconsistent state of kernel ring buffer.
It went out that it was caused by speculative store generated by
gcc-4.3.4.
The problem is in assembly generated for make_free_space(). The functions is
called the following way:
+ vprintk_emit();
+ log = MAIN_LOG; // with logbuf_lock
or
log = NMI_LOG; // with nmi_logbuf_lock
cont_add(log, ...);
+ cont_flush(log, ...);
+ log_store(log, ...);
+ log_make_free_space(log, ...);
If called with log = NMI_LOG then only nmi_log_* global variables are safe to
modify but the generated code does store also into (main_)log_* global
variables:
<log_make_free_space>:
55 push %rbp
89 f6 mov %esi,%esi
48 8b 05 03 99 51 01 mov 0x1519903(%rip),%rax # ffffffff82620868 <nmi_log_next_id>
44 8b 1d ec 98 51 01 mov 0x15198ec(%rip),%r11d # ffffffff82620858 <log_next_idx>
8b 35 36 60 14 01 mov 0x1146036(%rip),%esi # ffffffff8224cfa8 <log_buf_len>
44 8b 35 33 60 14 01 mov 0x1146033(%rip),%r14d # ffffffff8224cfac <nmi_log_buf_len>
4c 8b 2d d0 98 51 01 mov 0x15198d0(%rip),%r13 # ffffffff82620850 <log_next_seq>
4c 8b 25 11 61 14 01 mov 0x1146111(%rip),%r12 # ffffffff8224d098 <log_buf>
49 89 c2 mov %rax,%r10
48 21 c2 and %rax,%rdx
48 8b 1d 0c 99 55 01 mov 0x155990c(%rip),%rbx # ffffffff826608a0 <nmi_log_buf>
49 c1 ea 20 shr $0x20,%r10
48 89 55 d0 mov %rdx,-0x30(%rbp)
44 29 de sub %r11d,%esi
45 29 d6 sub %r10d,%r14d
4c 8b 0d 97 98 51 01 mov 0x1519897(%rip),%r9 # ffffffff82620840 <log_first_seq>
eb 7e jmp ffffffff81107029 <log_make_free_space+0xe9>
[...]
85 ff test %edi,%edi # edi = 1 for NMI_LOG
4c 89 e8 mov %r13,%rax
4c 89 ca mov %r9,%rdx
74 0a je ffffffff8110703d <log_make_free_space+0xfd>
8b 15 27 98 51 01 mov 0x1519827(%rip),%edx # ffffffff82620860 <nmi_log_first_id>
48 8b 45 d0 mov -0x30(%rbp),%rax
48 39 c2 cmp %rax,%rdx # end of loop
0f 84 da 00 00 00 je ffffffff81107120 <log_make_free_space+0x1e0>
[...]
85 ff test %edi,%edi # edi = 1 for NMI_LOG
4c 89 0d 17 97 51 01 mov %r9,0x1519717(%rip) # ffffffff82620840 <log_first_seq>
^^^^^^^^^^^^^^^^^^^^^^^^^^
KABOOOM
74 35 je ffffffff81107160 <log_make_free_space+0x220>
It stores log_first_seq when edi == NMI_LOG. This instructions are used also
when edi == MAIN_LOG but the store is done speculatively before the condition
is decided. It is unsafe because we do not have "logbuf_lock" in NMI context
and some other process migh modify "log_first_seq" in parallel.
===
I believe that the best course of action is both
- building kernel (and anything multi-threaded, I guess) with that
optimization turned off
- persuade gcc folks to change the default for future releases
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 18:04 ` Steven Noonan
@ 2014-06-10 18:54 ` Richard Biener
0 siblings, 0 replies; 15+ messages in thread
From: Richard Biener @ 2014-06-10 18:54 UTC (permalink / raw
To: Steven Noonan, Linus Torvalds
Cc: Jiri Kosina, Paul E. McKenney, Peter Zijlstra, Andrew Morton,
Martin Jambor, Petr Mladek, Linux Kernel Mailing List,
gcc@gcc.gnu.org
On June 10, 2014 8:04:13 PM CEST, Steven Noonan <steven@uplinklabs.net> wrote:
>On Tue, Jun 10, 2014 at 10:46 AM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>> On Tue, Jun 10, 2014 at 6:23 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>>> We have been chasing a memory corruption bug, which turned out to be
>>> caused by very old gcc (4.3.4), which happily turned conditional
>load into
>>> a non-conditional one, and that broke correctness (the condition was
>met
>>> only if lock was held) and corrupted memory.
>>
>> Just out of interest, can you point to the particular kernel code
>that
>> caused this? I think that's more interesting than the example program
>> you show - which I'm sure is really nice for gcc developers as an
>> example, but from a kernel standpoint I think it's more important to
>> show the particular problems this caused for the kernel?
>>
>
>Jiri, is there a workaround for compilers that don't support '--param
>allow-store-data-races=0'? For example:
The optimization that purposely performs the undesired transform is loop store motion which is part of the tree loop invariant motion optimization. You can disable that with -fno-tree-loop-im.
That the bug didn't appear with newer compilers was due to lucky decisions to not inline a particular function.
Richard.
>$ gcc-4.5 -O2 -o cond_store cond_store.c && ./cond_store
>Segmentation fault (core dumped)
>
>$ gcc-4.5 -O2 --param allow-store-data-races=0 -o cond_store
>cond_store.c && ./cond_store
>cc1: error: invalid parameter ‘allow-store-data-races’
>
>$ gcc-4.5 -v
>Using built-in specs.
>COLLECT_GCC=gcc-4.5
>COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/4.5.4/lto-wrapper
>Target: x86_64-unknown-linux-gnu
>Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
>--infodir=/usr/share/info --libdir=/usr/lib --libexecdir=/usr/lib
>--program-suffix=-4.5 --enable-shared
>--enable-languages=c,c++,fortran,objc,obj-c++ --enable-__cxa_atexit
>--disable-libstdcxx-pch --disable-multilib --disable-libgomp
>--disable-libmudflap --disable-libssp --enable-clocale=gnu
>--with-tune=generic --with-cloog --with-ppl --with-system-zlib
>Thread model: posix
>gcc version 4.5.4 (GCC)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 13:23 [PATCH] tell gcc optimizer to never introduce new data races Jiri Kosina
2014-06-10 14:53 ` Peter Zijlstra
2014-06-10 17:46 ` Linus Torvalds
@ 2014-06-16 10:29 ` Dan Carpenter
2014-06-16 10:52 ` Andreas Schwab
2014-06-17 7:58 ` Jiri Kosina
2014-08-20 23:27 ` Jiri Kosina
3 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2014-06-16 10:29 UTC (permalink / raw
To: Jiri Kosina
Cc: Linus Torvalds, Paul E. McKenney, Peter Zijlstra, Andrew Morton,
Martin Jambor, Petr Mladek, linux-kernel, gcc, linux-sparse
Adding "--param allow-store-data-races=0" to the GCC options for the
kernel breaks C=1 because Sparse isn't expecting a GCC option with that
format.
It thinks allow-store-data-races=0 is the name of the file we are trying
to test. Try use Sparse on linux-next to see the problem.
$ make C=2 mm/slab_common.o
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
CHECK scripts/mod/empty.c
No such file: allow-store-data-races=0
make[2]: *** [scripts/mod/empty.o] Error 1
make[1]: *** [scripts/mod] Error 2
make: *** [scripts] Error 2
$
regards,
dan carpenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-16 10:29 ` Dan Carpenter
@ 2014-06-16 10:52 ` Andreas Schwab
2014-06-16 11:20 ` Jiri Kosina
2014-06-16 14:37 ` Mark Brown
2014-06-17 7:58 ` Jiri Kosina
1 sibling, 2 replies; 15+ messages in thread
From: Andreas Schwab @ 2014-06-16 10:52 UTC (permalink / raw
To: Dan Carpenter
Cc: Jiri Kosina, Linus Torvalds, Paul E. McKenney, Peter Zijlstra,
Andrew Morton, Martin Jambor, Petr Mladek, linux-kernel, gcc,
linux-sparse
Dan Carpenter <dan.carpenter@oracle.com> writes:
> Adding "--param allow-store-data-races=0" to the GCC options for the
> kernel breaks C=1 because Sparse isn't expecting a GCC option with that
> format.
Please try --param=allow-store-data-races=0 instead.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-16 10:52 ` Andreas Schwab
@ 2014-06-16 11:20 ` Jiri Kosina
2014-06-16 14:37 ` Mark Brown
1 sibling, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2014-06-16 11:20 UTC (permalink / raw
To: Andreas Schwab
Cc: Dan Carpenter, Linus Torvalds, Paul E. McKenney, Peter Zijlstra,
Andrew Morton, Martin Jambor, Petr Mladek, linux-kernel, gcc,
linux-sparse
On Mon, 16 Jun 2014, Andreas Schwab wrote:
> > Adding "--param allow-store-data-races=0" to the GCC options for the
> > kernel breaks C=1 because Sparse isn't expecting a GCC option with that
> > format.
>
> Please try --param=allow-store-data-races=0 instead.
How reliable is this format across GCC versions? GCC manpage doesn't seem
to list it as a valid alternative.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-16 10:52 ` Andreas Schwab
2014-06-16 11:20 ` Jiri Kosina
@ 2014-06-16 14:37 ` Mark Brown
1 sibling, 0 replies; 15+ messages in thread
From: Mark Brown @ 2014-06-16 14:37 UTC (permalink / raw
To: Andreas Schwab
Cc: Dan Carpenter, Jiri Kosina, Linus Torvalds, Paul E. McKenney,
Peter Zijlstra, Andrew Morton, Martin Jambor, Petr Mladek,
linux-kernel, gcc, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 363 bytes --]
On Mon, Jun 16, 2014 at 12:52:10PM +0200, Andreas Schwab wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > Adding "--param allow-store-data-races=0" to the GCC options for the
> > kernel breaks C=1 because Sparse isn't expecting a GCC option with that
> > format.
> Please try --param=allow-store-data-races=0 instead.
That appears to work for me.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-16 10:29 ` Dan Carpenter
2014-06-16 10:52 ` Andreas Schwab
@ 2014-06-17 7:58 ` Jiri Kosina
1 sibling, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2014-06-17 7:58 UTC (permalink / raw
To: Dan Carpenter
Cc: Linus Torvalds, Paul E. McKenney, Peter Zijlstra, Andrew Morton,
Martin Jambor, Petr Mladek, linux-kernel, gcc, linux-sparse
On Mon, 16 Jun 2014, Dan Carpenter wrote:
> Adding "--param allow-store-data-races=0" to the GCC options for the
> kernel breaks C=1 because Sparse isn't expecting a GCC option with that
> format.
>
> It thinks allow-store-data-races=0 is the name of the file we are trying
> to test. Try use Sparse on linux-next to see the problem.
Alright, no word from gcc folks, so let's hope the undocumented format of
the parameter works better ... sigh.
Andrew, please use the updated one below.
From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] ./Makefile: tell gcc optimizer to never introduce new data races
We have been chasing a memory corruption bug, which turned out to be
caused by very old gcc (4.3.4), which happily turned conditional load into
a non-conditional one, and that broke correctness (the condition was met
only if lock was held) and corrupted memory.
This particular problem with that particular code did not happen when
never gccs were used. I've brought this up with our gcc folks, as I wanted
to make sure that this can't really happen again, and it turns out it
actually can.
Quoting Martin Jambor <mjambor@suse.cz>:
====
More current GCCs are more careful when it comes to replacing a
conditional load with a non-conditional one, most notably they check
that a store happens in each iteration of _a_ loop but they assume
loops are executed. They also perform a simple check whether the
store cannot trap which currently passes only for non-const
variables. A simple testcase demonstrating it on an x86_64 is for
example the following:
$ cat cond_store.c
int g_1 = 1;
int g_2[1024] __attribute__((section ("safe_section"), aligned (4096)));
int c = 4;
int __attribute__ ((noinline))
foo (void)
{
int l;
for (l = 0; (l != 4); l++) {
if (g_1)
return l;
for (g_2[0] = 0; (g_2[0] >= 26); ++g_2[0])
;
}
return 2;
}
int main (int argc, char* argv[])
{
if (mprotect (g_2, sizeof(g_2), PROT_READ) == -1)
{
int e = errno;
error (e, e, "mprotect error %i", e);
}
foo ();
__builtin_printf("OK\n");
return 0;
}
/* EOF */
$ ~/gcc/trunk/inst/bin/gcc cond_store.c -O2 --param allow-store-data-races=0
$ ./a.out
OK
$ ~/gcc/trunk/inst/bin/gcc cond_store.c -O2 --param allow-store-data-races=1
$ ./a.out
Segmentation fault
The testcase fails the same at least with 4.9, 4.8 and 4.7. Therefore
I would suggest building kernels with this parameter set to zero. I
also agree with Jikos that the default should be changed for -O2. I
have run most of the SPEC 2k6 CPU benchmarks (gamess and dealII
failed, at -O2, not sure why) compiled with and without this option
and did not see any real difference between respective run-times.
====
Hopefully the default will be changed in newer gccs, but let's force
it for kernel builds so that we are on a safe side even when older
gcc are used.
The code in question was out-of-tree printk-in-NMI (yeah, surprise
suprise, once again) patch written by Petr Mladek, let me quote his
comment from our internal bugzilla:
===
I have spent few days investigating inconsistent state of kernel ring buffer.
It went out that it was caused by speculative store generated by
gcc-4.3.4.
The problem is in assembly generated for make_free_space(). The functions is
called the following way:
+ vprintk_emit();
+ log = MAIN_LOG; // with logbuf_lock
or
log = NMI_LOG; // with nmi_logbuf_lock
cont_add(log, ...);
+ cont_flush(log, ...);
+ log_store(log, ...);
+ log_make_free_space(log, ...);
If called with log = NMI_LOG then only nmi_log_* global variables are safe to
modify but the generated code does store also into (main_)log_* global
variables:
<log_make_free_space>:
55 push %rbp
89 f6 mov %esi,%esi
48 8b 05 03 99 51 01 mov 0x1519903(%rip),%rax # ffffffff82620868 <nmi_log_next_id>
44 8b 1d ec 98 51 01 mov 0x15198ec(%rip),%r11d # ffffffff82620858 <log_next_idx>
8b 35 36 60 14 01 mov 0x1146036(%rip),%esi # ffffffff8224cfa8 <log_buf_len>
44 8b 35 33 60 14 01 mov 0x1146033(%rip),%r14d # ffffffff8224cfac <nmi_log_buf_len>
4c 8b 2d d0 98 51 01 mov 0x15198d0(%rip),%r13 # ffffffff82620850 <log_next_seq>
4c 8b 25 11 61 14 01 mov 0x1146111(%rip),%r12 # ffffffff8224d098 <log_buf>
49 89 c2 mov %rax,%r10
48 21 c2 and %rax,%rdx
48 8b 1d 0c 99 55 01 mov 0x155990c(%rip),%rbx # ffffffff826608a0 <nmi_log_buf>
49 c1 ea 20 shr $0x20,%r10
48 89 55 d0 mov %rdx,-0x30(%rbp)
44 29 de sub %r11d,%esi
45 29 d6 sub %r10d,%r14d
4c 8b 0d 97 98 51 01 mov 0x1519897(%rip),%r9 # ffffffff82620840 <log_first_seq>
eb 7e jmp ffffffff81107029 <log_make_free_space+0xe9>
[...]
85 ff test %edi,%edi # edi = 1 for NMI_LOG
4c 89 e8 mov %r13,%rax
4c 89 ca mov %r9,%rdx
74 0a je ffffffff8110703d <log_make_free_space+0xfd>
8b 15 27 98 51 01 mov 0x1519827(%rip),%edx # ffffffff82620860 <nmi_log_first_id>
48 8b 45 d0 mov -0x30(%rbp),%rax
48 39 c2 cmp %rax,%rdx # end of loop
0f 84 da 00 00 00 je ffffffff81107120 <log_make_free_space+0x1e0>
[...]
85 ff test %edi,%edi # edi = 1 for NMI_LOG
4c 89 0d 17 97 51 01 mov %r9,0x1519717(%rip) # ffffffff82620840 <log_first_seq>
^^^^^^^^^^^^^^^^^^^^^^^^^^
KABOOOM
74 35 je ffffffff81107160 <log_make_free_space+0x220>
It stores log_first_seq when edi == NMI_LOG. This instructions are used also
when edi == MAIN_LOG but the store is done speculatively before the condition
is decided. It is unsafe because we do not have "logbuf_lock" in NMI context
and some other process migh modify "log_first_seq" in parallel.
===
I believe that the best course of action is both
- building kernel (and anything multi-threaded, I guess) with that
optimization turned off
- persuade gcc folks to change the default for future releases
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Cc: Martin Jambor <mjambor@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Marek Polacek <polacek@redhat.com>
Cc: Jakub Jelinek <jakub@redhat.com>
Cc: Steven Noonan <steven@uplinklabs.net>
Cc: Richard Biener <richard.guenther@gmail.com>
---
Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Makefile b/Makefile
index 00a933b..255e56d 100644
--- a/Makefile
+++ b/Makefile
@@ -587,6 +587,9 @@ endif
include $(srctree)/arch/$(SRCARCH)/Makefile
+# Tell gcc to never replace conditional load with a non-conditional one
+KBUILD_CFLAGS += $(call cc-option,--param=allow-store-data-races=0)
+
ifdef CONFIG_READABLE_ASM
# Disable optimizations that make assembler listings hard to read.
# reorder blocks reorders the control in the function
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] tell gcc optimizer to never introduce new data races
2014-06-10 13:23 [PATCH] tell gcc optimizer to never introduce new data races Jiri Kosina
` (2 preceding siblings ...)
2014-06-16 10:29 ` Dan Carpenter
@ 2014-08-20 23:27 ` Jiri Kosina
3 siblings, 0 replies; 15+ messages in thread
From: Jiri Kosina @ 2014-08-20 23:27 UTC (permalink / raw
To: Linus Torvalds
Cc: Paul E. McKenney, Peter Zijlstra, Andrew Morton, Martin Jambor,
Petr Mladek, linux-kernel, gcc
On Tue, 10 Jun 2014, Jiri Kosina wrote:
> We have been chasing a memory corruption bug, which turned out to be
> caused by very old gcc (4.3.4), which happily turned conditional load into
> a non-conditional one, and that broke correctness (the condition was met
> only if lock was held) and corrupted memory.
>
> This particular problem with that particular code did not happen when
> never gccs were used. I've brought this up with our gcc folks, as I wanted
> to make sure that this can't really happen again, and it turns out it
> actually can.
For the record (and for mailinglist archives to catch this information) --
the default has been changed for gcc 4.10 to a sane value (0).
I'd however suggest we keep changes made by commit 69102311a ("./Makefile:
tell gcc optimizer to never introduce new data races") for rather a long
time.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-08-20 23:28 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-10 13:23 [PATCH] tell gcc optimizer to never introduce new data races Jiri Kosina
2014-06-10 14:53 ` Peter Zijlstra
2014-06-10 15:04 ` Marek Polacek
2014-06-10 15:13 ` Peter Zijlstra
2014-06-10 15:17 ` Jakub Jelinek
2014-06-10 17:46 ` Linus Torvalds
2014-06-10 18:04 ` Steven Noonan
2014-06-10 18:54 ` Richard Biener
2014-06-10 18:10 ` Jiri Kosina
2014-06-16 10:29 ` Dan Carpenter
2014-06-16 10:52 ` Andreas Schwab
2014-06-16 11:20 ` Jiri Kosina
2014-06-16 14:37 ` Mark Brown
2014-06-17 7:58 ` Jiri Kosina
2014-08-20 23:27 ` Jiri Kosina
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.