LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Will Deacon <will@kernel.org>
To: linux-kernel@vger.kernel.org
Cc: linux-arch@vger.kernel.org, kernel-team@android.com,
	Will Deacon <will@kernel.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Peter Zijlstra <peterz@infradead.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Segher Boessenkool <segher@kernel.crashing.org>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>
Subject: [RFC PATCH 5/8] READ_ONCE: Enforce atomicity for {READ,WRITE}_ONCE() memory accesses
Date: Fri, 10 Jan 2020 16:56:33 +0000
Message-ID: <20200110165636.28035-6-will@kernel.org> (raw)
In-Reply-To: <20200110165636.28035-1-will@kernel.org>

{READ,WRITE}_ONCE() cannot guarantee atomicity for arbitrary data sizes.
This can be surprising to callers that might incorrectly be expecting
atomicity for accesses to aggregate structures, although there are other
callers where tearing is actually permissable (e.g. if they are using
something akin to sequence locking to protect the access).

Linus sayeth:

  | We could also look at being stricter for the normal READ/WRITE_ONCE(),
  | and require that they are
  |
  | (a) regular integer types
  |
  | (b) fit in an atomic word
  |
  | We actually did (b) for a while, until we noticed that we do it on
  | loff_t's etc and relaxed the rules. But maybe we could have a
  | "non-atomic" version of READ/WRITE_ONCE() that is used for the
  | questionable cases?

The slight snag is that we also have to support 64-bit accesses on 32-bit
architectures, as these appear to be widespread and tend to work out ok
if either the architecture supports atomic 64-bit accesses (x86, armv7)
or if the variable being accesses represents a virtual address and
therefore only requires 32-bit atomicity in practice.

Take a step in that direction by introducing a variant of
'compiletime_assert_atomic_type()' and use it to check the pointer
argument to {READ,WRITE}_ONCE(). Expose __{READ,WRITE_ONCE}() variants
which are allowed to tear and convert the two broken callers over to the
new macros.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/xen/time.c       |  2 +-
 include/linux/compiler.h | 37 +++++++++++++++++++++++++++++++++----
 net/xdp/xsk_queue.h      |  2 +-
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/time.c b/drivers/xen/time.c
index 0968859c29d0..108edbcbc040 100644
--- a/drivers/xen/time.c
+++ b/drivers/xen/time.c
@@ -64,7 +64,7 @@ static void xen_get_runstate_snapshot_cpu_delta(
 	do {
 		state_time = get64(&state->state_entry_time);
 		rmb();	/* Hypervisor might update data. */
-		*res = READ_ONCE(*state);
+		*res = __READ_ONCE(*state);
 		rmb();	/* Hypervisor might update data. */
 	} while (get64(&state->state_entry_time) != state_time ||
 		 (state_time & XEN_RUNSTATE_UPDATE));
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 44974d658f30..863180641336 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -198,24 +198,43 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 #include <asm/barrier.h>
 #include <linux/kasan-checks.h>
 
+/*
+ * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
+ * atomicity or dependency ordering guarantees. Note that this may result
+ * in tears!
+ */
+#define __READ_ONCE(x)	(*(volatile typeof(x) *)&(x))
+
+#define __READ_ONCE_SCALAR(x)						\
+({									\
+	typeof(x) __x = __READ_ONCE(x);					\
+	smp_read_barrier_depends();					\
+	__x;								\
+})
+
 /*
  * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
  * to hide memory access from KASAN.
  */
 #define READ_ONCE_NOCHECK(x)						\
 ({									\
-	typeof(x) __x = *(volatile typeof(x) *)&(x);			\
-	smp_read_barrier_depends();					\
-	__x;								\
+	compiletime_assert_rwonce_type(x);				\
+	__READ_ONCE_SCALAR(x);						\
 })
 
 #define READ_ONCE(x)	READ_ONCE_NOCHECK(x)
 
-#define WRITE_ONCE(x, val)				\
+#define __WRITE_ONCE(x, val)				\
 do {							\
 	*(volatile typeof(x) *)&(x) = (val);		\
 } while (0)
 
+#define WRITE_ONCE(x, val)				\
+do {							\
+	compiletime_assert_rwonce_type(x);		\
+	__WRITE_ONCE(x, val);				\
+} while (0)
+
 #ifdef CONFIG_KASAN
 /*
  * We can't declare function 'inline' because __no_sanitize_address conflicts
@@ -299,6 +318,16 @@ static inline void *offset_to_ptr(const int *off)
 	compiletime_assert(__native_word(t),				\
 		"Need native word sized stores/loads for atomicity.")
 
+/*
+ * Yes, this permits 64-bit accesses on 32-bit architectures. These will
+ * actually be atomic in many cases (namely x86), but for others we rely on
+ * the access being split into 2x32-bit accesses for a 32-bit quantity (e.g.
+ * a virtual address) and a strong prevailing wind.
+ */
+#define compiletime_assert_rwonce_type(t)					\
+	compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),	\
+		"Unsupported access size for {READ,WRITE}_ONCE().")
+
 /* &a[0] degrades to a pointer: a different type from an array */
 #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
 
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index eddae4688862..2b55c1c7b2b6 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -304,7 +304,7 @@ static inline struct xdp_desc *xskq_validate_desc(struct xsk_queue *q,
 		struct xdp_rxtx_ring *ring = (struct xdp_rxtx_ring *)q->ring;
 		unsigned int idx = q->cons_tail & q->ring_mask;
 
-		*desc = READ_ONCE(ring->desc[idx]);
+		*desc = __READ_ONCE(ring->desc[idx]);
 		if (xskq_is_valid_desc(q, desc, umem))
 			return desc;
 
-- 
2.25.0.rc1.283.g88dfdc4193-goog


  parent reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 16:56 [RFC PATCH 0/8] Rework READ_ONCE() to improve codegen Will Deacon
2020-01-10 16:56 ` [RFC PATCH 1/8] compiler/gcc: Emit build-time warning for GCC prior to version 4.8 Will Deacon
2020-01-10 17:35   ` Arnd Bergmann
2020-01-10 17:53     ` Joe Perches
2020-01-13 14:39       ` Arnd Bergmann
2020-01-13 15:35         ` Masahiro Yamada
2020-01-13 14:27     ` Will Deacon
2020-01-14 21:39     ` Nick Desaulniers
2020-01-15 10:35       ` David Laight
2020-01-15 10:49       ` Arnd Bergmann
2020-01-10 16:56 ` [RFC PATCH 2/8] netfilter: Avoid assigning 'const' pointer to non-const pointer Will Deacon
2020-01-10 16:56 ` [RFC PATCH 3/8] fault_inject: Don't rely on "return value" from WRITE_ONCE() Will Deacon
2020-01-10 16:56 ` [RFC PATCH 4/8] READ_ONCE: Simplify implementations of {READ,WRITE}_ONCE() Will Deacon
2020-01-10 16:56 ` Will Deacon [this message]
2020-01-10 19:24   ` [RFC PATCH 5/8] READ_ONCE: Enforce atomicity for {READ,WRITE}_ONCE() memory accesses Arnd Bergmann
2020-01-13 16:16     ` Will Deacon
2020-01-10 16:56 ` [RFC PATCH 6/8] READ_ONCE: Drop pointer qualifiers when reading from scalar types Will Deacon
2020-01-10 18:54   ` Linus Torvalds
2020-01-13 14:59     ` Will Deacon
2020-01-13 17:42       ` Luc Van Oostenryck
2020-01-13 19:31       ` Linus Torvalds
2020-01-10 16:56 ` [RFC PATCH 7/8] locking/barriers: Use '__unqual_scalar_typeof' for load-acquire macros Will Deacon
2020-01-10 19:42   ` Arnd Bergmann
2020-01-13 15:01     ` Will Deacon
2020-01-10 16:56 ` [RFC PATCH 8/8] arm64: barrier: Use '__unqual_scalar_typeof' for acquire/release macros Will Deacon
2020-01-10 17:45   ` Mark Rutland
2020-01-10 17:58 ` [RFC PATCH 0/8] Rework READ_ONCE() to improve codegen Arnd Bergmann
2020-01-10 19:46 ` Arnd Bergmann
2020-01-10 20:14   ` Linus Torvalds
2020-01-13 13:03     ` Arnd Bergmann
2020-01-13 11:20 ` David Laight
2020-01-13 12:40 ` Christian Borntraeger

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200110165636.28035-6-will@kernel.org \
    --to=will@kernel.org \
    --cc=arnd@arndb.de \
    --cc=borntraeger@de.ibm.com \
    --cc=kernel-team@android.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=mpe@ellerman.id.au \
    --cc=peterz@infradead.org \
    --cc=segher@kernel.crashing.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git