Message-ID: <alpine.LNX.2.00.1406101517300.9378@pobox.suse.cz>
Date:	Tue, 10 Jun 2014 15:23:36 +0200 (CEST)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>
cc:	Martin Jambor <mjambor@...e.cz>, Petr Mladek <pmladek@...e.cz>,
	linux-kernel@...r.kernel.org, gcc@....gnu.org
Subject: [PATCH] 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@...e.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@...e.cz>
Cc: Petr Mladek <pmladek@...e.cz>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Signed-off-by: Jiri Kosina <jkosina@...e.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
--
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/

Powered by blists - more mailing lists