linux-kernel - Re: [PATCH] kernel: fix data race in put

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]

Message-ID: <20150918115637.GM3604@twins.programming.kicks-ass.net>
Date:	Fri, 18 Sep 2015 13:56:37 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Will Deacon <will.deacon@....com>, Oleg Nesterov <oleg@...hat.com>,
	"ebiederm@...ssion.com" <ebiederm@...ssion.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	"mhocko@...e.cz" <mhocko@...e.cz>,
	LKML <linux-kernel@...r.kernel.org>,
	"ktsan@...glegroups.com" <ktsan@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Hans Boehm <hboehm@...gle.com>
Subject: Re: [PATCH] kernel: fix data race in put_pid

On Fri, Sep 18, 2015 at 01:50:01PM +0200, Dmitry Vyukov wrote:
> > +#ifndef atomic_read_ctrl
> > +static inline int atomic_read_ctrl(atomic_t *v)
> > +{
> > +       int val = atomic_read(v);
> > +       smp_read_barrier_depends(); /* Enforce control dependency. */
> > +       return val;
> > +}
> > +#endif
> > +
> >  /*
> >   * Relaxed variants of xchg, cmpxchg and some atomic operations.
> >   *
> 
> Looks good to me.
> Should we add atomic64_read_ctrl for completeness? I have not seen
> cases where it was needed, though.

Sure, and while doing another spin, let me go update the documentation
too.

---
Subject: atomic: Implement atomic_read_ctrl()
From: Peter Zijlstra <peterz@...radead.org>
Date: Fri, 18 Sep 2015 13:22:52 +0200

Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
more conveniently use atomics in control dependencies.

Since we can assume atomic_read() implies a READ_ONCE(), we must only
emit an extra smp_read_barrier_depends() in order to upgrade to
READ_ONCE_CTRL() semantics.

Cc: oleg@...hat.com
Cc: torvalds@...ux-foundation.org
Cc: will.deacon@....com
Cc: paulmck@...ux.vnet.ibm.com
Requested-by: Dmitry Vyukov <dvyukov@...gle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
 Documentation/memory-barriers.txt |   17 +++++++++--------
 include/linux/atomic.h            |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 8 deletions(-)

--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -637,7 +637,8 @@ to optimize the original example by elim
 	b = p;  /* BUG: Compiler and CPU can both reorder!!! */
 
 Finally, the READ_ONCE_CTRL() includes an smp_read_barrier_depends()
-that DEC Alpha needs in order to respect control depedencies.
+that DEC Alpha needs in order to respect control depedencies. Alternatively
+use one of atomic{,64}_read_ctrl().
 
 So don't leave out the READ_ONCE_CTRL().
 
@@ -796,9 +797,9 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
 
 In summary:
 
-  (*) Control dependencies must be headed by READ_ONCE_CTRL().
-      Or, as a much less preferable alternative, interpose
-      smp_read_barrier_depends() between a READ_ONCE() and the
+  (*) Control dependencies must be headed by READ_ONCE_CTRL(),
+      atomic{,64}_read_ctrl(). Or, as a much less preferable alternative,
+      interpose smp_read_barrier_depends() between a READ_ONCE() and the
       control-dependent write.
 
   (*) Control dependencies can order prior loads against later stores.
@@ -820,10 +821,10 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
       and WRITE_ONCE() can help to preserve the needed conditional.
 
   (*) Control dependencies require that the compiler avoid reordering the
-      dependency into nonexistence.  Careful use of READ_ONCE_CTRL()
-      or smp_read_barrier_depends() can help to preserve your control
-      dependency.  Please see the Compiler Barrier section for more
-      information.
+      dependency into nonexistence.  Careful use of READ_ONCE_CTRL(),
+      atomic{,64}_read_ctrl() or smp_read_barrier_depends() can help to
+      preserve your control dependency.  Please see the Compiler Barrier
+      section for more information.
 
   (*) Control dependencies pair normally with other types of barriers.
 
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -4,6 +4,24 @@
 #include <asm/atomic.h>
 #include <asm/barrier.h>
 
+#ifndef atomic_read_ctrl
+static inline int atomic_read_ctrl(atomic_t *v)
+{
+	int val = atomic_read(v);
+	smp_read_barrier_depends(); /* Enforce control dependency. */
+	return val;
+}
+#endif
+
+#ifndef atomic64_read_ctrl
+static inline int atomic64_read_ctrl(atomic64_t *v)
+{
+	int val = atomic64_read(v);
+	smp_read_barrier_depends(); /* Enforce control dependency. */
+	return val;
+}
+#endif
+
 /*
  * Relaxed variants of xchg, cmpxchg and some atomic operations.
  *
--
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/