Hi,
It's me again. This is version four of the READ_ONCE() codegen improvement
patches that I previously posted here:
RFC: https://lore.kernel.org/lkml/[email protected]
v2: https://lore.kernel.org/lkml/[email protected]
v3: https://lore.kernel.org/lkml/[email protected]/
Changes since v3 include:
* Drop the patch warning for GCC versions prior to 4.8
* Move the patch raising the minimum GCC version earlier in the series
* Reintroduce the old READ_ONCE_NOCHECK behaviour so that it can continue
to be used by stack unwinders
As before, this will break the build for sparc32 without the changes here:
https://lore.kernel.org/lkml/[email protected]
and boy are they proving to be popular (I'm interpreting the silence as
monumental joy).
Ta,
Will
Cc: Mark Rutland <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Segher Boessenkool <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: Luc Van Oostenryck <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Peter Oberparleiter <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Nick Desaulniers <[email protected]>
--->8
Will Deacon (11):
compiler/gcc: Raise minimum GCC version for kernel builds to 4.8
netfilter: Avoid assigning 'const' pointer to non-const pointer
net: tls: Avoid assigning 'const' pointer to non-const pointer
fault_inject: Don't rely on "return value" from WRITE_ONCE()
arm64: csum: Disable KASAN for do_csum()
READ_ONCE: Simplify implementations of {READ,WRITE}_ONCE()
READ_ONCE: Enforce atomicity for {READ,WRITE}_ONCE() memory accesses
READ_ONCE: Drop pointer qualifiers when reading from scalar types
locking/barriers: Use '__unqual_scalar_typeof' for load-acquire macros
arm64: barrier: Use '__unqual_scalar_typeof' for acquire/release
macros
gcov: Remove old GCC 3.4 support
Documentation/process/changes.rst | 2 +-
arch/arm/crypto/Kconfig | 12 +-
arch/arm64/include/asm/barrier.h | 16 +-
arch/arm64/lib/csum.c | 20 +-
crypto/Kconfig | 1 -
drivers/xen/time.c | 2 +-
include/asm-generic/barrier.h | 16 +-
include/linux/compiler-gcc.h | 5 +-
include/linux/compiler.h | 145 ++++----
include/linux/compiler_types.h | 21 ++
init/Kconfig | 1 -
kernel/gcov/Kconfig | 24 --
kernel/gcov/Makefile | 3 +-
kernel/gcov/gcc_3_4.c | 573 ------------------------------
lib/fault-inject.c | 4 +-
net/netfilter/core.c | 2 +-
net/tls/tls_main.c | 2 +-
scripts/gcc-plugins/Kconfig | 2 +-
18 files changed, 132 insertions(+), 719 deletions(-)
delete mode 100644 kernel/gcov/gcc_3_4.c
--
2.26.1.301.g55bc3eb7cb9-goog
tls_build_proto() uses WRITE_ONCE() to assign a 'const' pointer to a
'non-const' pointer. Cleanups to the implementation of WRITE_ONCE() mean
that this will give rise to a compiler warning, just like a plain old
assignment would do:
| net/tls/tls_main.c: In function ‘tls_build_proto’:
| ./include/linux/compiler.h:229:30: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
| net/tls/tls_main.c:640:4: note: in expansion of macro ‘smp_store_release’
| 640 | smp_store_release(&saved_tcpv6_prot, prot);
| | ^~~~~~~~~~~~~~~~~
Drop the const qualifier from the local 'prot' variable, as it isn't
needed.
Cc: Boris Pismenny <[email protected]>
Cc: Aviad Yehezkel <[email protected]>
Cc: John Fastabend <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
net/tls/tls_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 0e989005bdc2..ec10041c6b7d 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -629,7 +629,7 @@ struct tls_context *tls_ctx_create(struct sock *sk)
static void tls_build_proto(struct sock *sk)
{
int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
- const struct proto *prot = READ_ONCE(sk->sk_prot);
+ struct proto *prot = READ_ONCE(sk->sk_prot);
/* Build IPv6 TLS whenever the address of tcpv6 _prot changes */
if (ip_ver == TLSV6 &&
--
2.26.1.301.g55bc3eb7cb9-goog
The kernel requires at least GCC 4.8 in order to build, and so there is
no need to cater for the pre-4.7 gcov format.
Remove the obsolete code.
Acked-by: Peter Oberparleiter <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
kernel/gcov/Kconfig | 24 --
kernel/gcov/Makefile | 3 +-
kernel/gcov/gcc_3_4.c | 573 ------------------------------------------
3 files changed, 1 insertion(+), 599 deletions(-)
delete mode 100644 kernel/gcov/gcc_3_4.c
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 3941a9c48f83..feaad597b3f4 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -51,28 +51,4 @@ config GCOV_PROFILE_ALL
larger and run slower. Also be sure to exclude files from profiling
which are not linked to the kernel image to prevent linker errors.
-choice
- prompt "Specify GCOV format"
- depends on GCOV_KERNEL
- depends on CC_IS_GCC
- ---help---
- The gcov format is usually determined by the GCC version, and the
- default is chosen according to your GCC version. However, there are
- exceptions where format changes are integrated in lower-version GCCs.
- In such a case, change this option to adjust the format used in the
- kernel accordingly.
-
-config GCOV_FORMAT_3_4
- bool "GCC 3.4 format"
- depends on GCC_VERSION < 40700
- ---help---
- Select this option to use the format defined by GCC 3.4.
-
-config GCOV_FORMAT_4_7
- bool "GCC 4.7 format"
- ---help---
- Select this option to use the format defined by GCC 4.7.
-
-endchoice
-
endmenu
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index d66a74b0f100..16f8ecc7d882 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -2,6 +2,5 @@
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
-obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
-obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
+obj-$(CONFIG_CC_IS_GCC) += gcc_base.o gcc_4_7.o
obj-$(CONFIG_CC_IS_CLANG) += clang.o
diff --git a/kernel/gcov/gcc_3_4.c b/kernel/gcov/gcc_3_4.c
deleted file mode 100644
index acb83558e5df..000000000000
--- a/kernel/gcov/gcc_3_4.c
+++ /dev/null
@@ -1,573 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * This code provides functions to handle gcc's profiling data format
- * introduced with gcc 3.4. Future versions of gcc may change the gcov
- * format (as happened before), so all format-specific information needs
- * to be kept modular and easily exchangeable.
- *
- * This file is based on gcc-internal definitions. Functions and data
- * structures are defined to be compatible with gcc counterparts.
- * For a better understanding, refer to gcc source: gcc/gcov-io.h.
- *
- * Copyright IBM Corp. 2009
- * Author(s): Peter Oberparleiter <[email protected]>
- *
- * Uses gcc-internal data definitions.
- */
-
-#include <linux/errno.h>
-#include <linux/slab.h>
-#include <linux/string.h>
-#include <linux/seq_file.h>
-#include <linux/vmalloc.h>
-#include "gcov.h"
-
-#define GCOV_COUNTERS 5
-
-static struct gcov_info *gcov_info_head;
-
-/**
- * struct gcov_fn_info - profiling meta data per function
- * @ident: object file-unique function identifier
- * @checksum: function checksum
- * @n_ctrs: number of values per counter type belonging to this function
- *
- * This data is generated by gcc during compilation and doesn't change
- * at run-time.
- */
-struct gcov_fn_info {
- unsigned int ident;
- unsigned int checksum;
- unsigned int n_ctrs[];
-};
-
-/**
- * struct gcov_ctr_info - profiling data per counter type
- * @num: number of counter values for this type
- * @values: array of counter values for this type
- * @merge: merge function for counter values of this type (unused)
- *
- * This data is generated by gcc during compilation and doesn't change
- * at run-time with the exception of the values array.
- */
-struct gcov_ctr_info {
- unsigned int num;
- gcov_type *values;
- void (*merge)(gcov_type *, unsigned int);
-};
-
-/**
- * struct gcov_info - profiling data per object file
- * @version: gcov version magic indicating the gcc version used for compilation
- * @next: list head for a singly-linked list
- * @stamp: time stamp
- * @filename: name of the associated gcov data file
- * @n_functions: number of instrumented functions
- * @functions: function data
- * @ctr_mask: mask specifying which counter types are active
- * @counts: counter data per counter type
- *
- * This data is generated by gcc during compilation and doesn't change
- * at run-time with the exception of the next pointer.
- */
-struct gcov_info {
- unsigned int version;
- struct gcov_info *next;
- unsigned int stamp;
- const char *filename;
- unsigned int n_functions;
- const struct gcov_fn_info *functions;
- unsigned int ctr_mask;
- struct gcov_ctr_info counts[];
-};
-
-/**
- * gcov_info_filename - return info filename
- * @info: profiling data set
- */
-const char *gcov_info_filename(struct gcov_info *info)
-{
- return info->filename;
-}
-
-/**
- * gcov_info_version - return info version
- * @info: profiling data set
- */
-unsigned int gcov_info_version(struct gcov_info *info)
-{
- return info->version;
-}
-
-/**
- * gcov_info_next - return next profiling data set
- * @info: profiling data set
- *
- * Returns next gcov_info following @info or first gcov_info in the chain if
- * @info is %NULL.
- */
-struct gcov_info *gcov_info_next(struct gcov_info *info)
-{
- if (!info)
- return gcov_info_head;
-
- return info->next;
-}
-
-/**
- * gcov_info_link - link/add profiling data set to the list
- * @info: profiling data set
- */
-void gcov_info_link(struct gcov_info *info)
-{
- info->next = gcov_info_head;
- gcov_info_head = info;
-}
-
-/**
- * gcov_info_unlink - unlink/remove profiling data set from the list
- * @prev: previous profiling data set
- * @info: profiling data set
- */
-void gcov_info_unlink(struct gcov_info *prev, struct gcov_info *info)
-{
- if (prev)
- prev->next = info->next;
- else
- gcov_info_head = info->next;
-}
-
-/**
- * gcov_info_within_module - check if a profiling data set belongs to a module
- * @info: profiling data set
- * @mod: module
- *
- * Returns true if profiling data belongs module, false otherwise.
- */
-bool gcov_info_within_module(struct gcov_info *info, struct module *mod)
-{
- return within_module((unsigned long)info, mod);
-}
-
-/* Symbolic links to be created for each profiling data file. */
-const struct gcov_link gcov_link[] = {
- { OBJ_TREE, "gcno" }, /* Link to .gcno file in $(objtree). */
- { 0, NULL},
-};
-
-/*
- * Determine whether a counter is active. Based on gcc magic. Doesn't change
- * at run-time.
- */
-static int counter_active(struct gcov_info *info, unsigned int type)
-{
- return (1 << type) & info->ctr_mask;
-}
-
-/* Determine number of active counters. Based on gcc magic. */
-static unsigned int num_counter_active(struct gcov_info *info)
-{
- unsigned int i;
- unsigned int result = 0;
-
- for (i = 0; i < GCOV_COUNTERS; i++) {
- if (counter_active(info, i))
- result++;
- }
- return result;
-}
-
-/**
- * gcov_info_reset - reset profiling data to zero
- * @info: profiling data set
- */
-void gcov_info_reset(struct gcov_info *info)
-{
- unsigned int active = num_counter_active(info);
- unsigned int i;
-
- for (i = 0; i < active; i++) {
- memset(info->counts[i].values, 0,
- info->counts[i].num * sizeof(gcov_type));
- }
-}
-
-/**
- * gcov_info_is_compatible - check if profiling data can be added
- * @info1: first profiling data set
- * @info2: second profiling data set
- *
- * Returns non-zero if profiling data can be added, zero otherwise.
- */
-int gcov_info_is_compatible(struct gcov_info *info1, struct gcov_info *info2)
-{
- return (info1->stamp == info2->stamp);
-}
-
-/**
- * gcov_info_add - add up profiling data
- * @dest: profiling data set to which data is added
- * @source: profiling data set which is added
- *
- * Adds profiling counts of @source to @dest.
- */
-void gcov_info_add(struct gcov_info *dest, struct gcov_info *source)
-{
- unsigned int i;
- unsigned int j;
-
- for (i = 0; i < num_counter_active(dest); i++) {
- for (j = 0; j < dest->counts[i].num; j++) {
- dest->counts[i].values[j] +=
- source->counts[i].values[j];
- }
- }
-}
-
-/* Get size of function info entry. Based on gcc magic. */
-static size_t get_fn_size(struct gcov_info *info)
-{
- size_t size;
-
- size = sizeof(struct gcov_fn_info) + num_counter_active(info) *
- sizeof(unsigned int);
- if (__alignof__(struct gcov_fn_info) > sizeof(unsigned int))
- size = ALIGN(size, __alignof__(struct gcov_fn_info));
- return size;
-}
-
-/* Get address of function info entry. Based on gcc magic. */
-static struct gcov_fn_info *get_fn_info(struct gcov_info *info, unsigned int fn)
-{
- return (struct gcov_fn_info *)
- ((char *) info->functions + fn * get_fn_size(info));
-}
-
-/**
- * gcov_info_dup - duplicate profiling data set
- * @info: profiling data set to duplicate
- *
- * Return newly allocated duplicate on success, %NULL on error.
- */
-struct gcov_info *gcov_info_dup(struct gcov_info *info)
-{
- struct gcov_info *dup;
- unsigned int i;
- unsigned int active;
-
- /* Duplicate gcov_info. */
- active = num_counter_active(info);
- dup = kzalloc(struct_size(dup, counts, active), GFP_KERNEL);
- if (!dup)
- return NULL;
- dup->version = info->version;
- dup->stamp = info->stamp;
- dup->n_functions = info->n_functions;
- dup->ctr_mask = info->ctr_mask;
- /* Duplicate filename. */
- dup->filename = kstrdup(info->filename, GFP_KERNEL);
- if (!dup->filename)
- goto err_free;
- /* Duplicate table of functions. */
- dup->functions = kmemdup(info->functions, info->n_functions *
- get_fn_size(info), GFP_KERNEL);
- if (!dup->functions)
- goto err_free;
- /* Duplicate counter arrays. */
- for (i = 0; i < active ; i++) {
- struct gcov_ctr_info *ctr = &info->counts[i];
- size_t size = ctr->num * sizeof(gcov_type);
-
- dup->counts[i].num = ctr->num;
- dup->counts[i].merge = ctr->merge;
- dup->counts[i].values = vmalloc(size);
- if (!dup->counts[i].values)
- goto err_free;
- memcpy(dup->counts[i].values, ctr->values, size);
- }
- return dup;
-
-err_free:
- gcov_info_free(dup);
- return NULL;
-}
-
-/**
- * gcov_info_free - release memory for profiling data set duplicate
- * @info: profiling data set duplicate to free
- */
-void gcov_info_free(struct gcov_info *info)
-{
- unsigned int active = num_counter_active(info);
- unsigned int i;
-
- for (i = 0; i < active ; i++)
- vfree(info->counts[i].values);
- kfree(info->functions);
- kfree(info->filename);
- kfree(info);
-}
-
-/**
- * struct type_info - iterator helper array
- * @ctr_type: counter type
- * @offset: index of the first value of the current function for this type
- *
- * This array is needed to convert the in-memory data format into the in-file
- * data format:
- *
- * In-memory:
- * for each counter type
- * for each function
- * values
- *
- * In-file:
- * for each function
- * for each counter type
- * values
- *
- * See gcc source gcc/gcov-io.h for more information on data organization.
- */
-struct type_info {
- int ctr_type;
- unsigned int offset;
-};
-
-/**
- * struct gcov_iterator - specifies current file position in logical records
- * @info: associated profiling data
- * @record: record type
- * @function: function number
- * @type: counter type
- * @count: index into values array
- * @num_types: number of counter types
- * @type_info: helper array to get values-array offset for current function
- */
-struct gcov_iterator {
- struct gcov_info *info;
-
- int record;
- unsigned int function;
- unsigned int type;
- unsigned int count;
-
- int num_types;
- struct type_info type_info[];
-};
-
-static struct gcov_fn_info *get_func(struct gcov_iterator *iter)
-{
- return get_fn_info(iter->info, iter->function);
-}
-
-static struct type_info *get_type(struct gcov_iterator *iter)
-{
- return &iter->type_info[iter->type];
-}
-
-/**
- * gcov_iter_new - allocate and initialize profiling data iterator
- * @info: profiling data set to be iterated
- *
- * Return file iterator on success, %NULL otherwise.
- */
-struct gcov_iterator *gcov_iter_new(struct gcov_info *info)
-{
- struct gcov_iterator *iter;
-
- iter = kzalloc(struct_size(iter, type_info, num_counter_active(info)),
- GFP_KERNEL);
- if (iter)
- iter->info = info;
-
- return iter;
-}
-
-/**
- * gcov_iter_free - release memory for iterator
- * @iter: file iterator to free
- */
-void gcov_iter_free(struct gcov_iterator *iter)
-{
- kfree(iter);
-}
-
-/**
- * gcov_iter_get_info - return profiling data set for given file iterator
- * @iter: file iterator
- */
-struct gcov_info *gcov_iter_get_info(struct gcov_iterator *iter)
-{
- return iter->info;
-}
-
-/**
- * gcov_iter_start - reset file iterator to starting position
- * @iter: file iterator
- */
-void gcov_iter_start(struct gcov_iterator *iter)
-{
- int i;
-
- iter->record = 0;
- iter->function = 0;
- iter->type = 0;
- iter->count = 0;
- iter->num_types = 0;
- for (i = 0; i < GCOV_COUNTERS; i++) {
- if (counter_active(iter->info, i)) {
- iter->type_info[iter->num_types].ctr_type = i;
- iter->type_info[iter->num_types++].offset = 0;
- }
- }
-}
-
-/* Mapping of logical record number to actual file content. */
-#define RECORD_FILE_MAGIC 0
-#define RECORD_GCOV_VERSION 1
-#define RECORD_TIME_STAMP 2
-#define RECORD_FUNCTION_TAG 3
-#define RECORD_FUNCTON_TAG_LEN 4
-#define RECORD_FUNCTION_IDENT 5
-#define RECORD_FUNCTION_CHECK 6
-#define RECORD_COUNT_TAG 7
-#define RECORD_COUNT_LEN 8
-#define RECORD_COUNT 9
-
-/**
- * gcov_iter_next - advance file iterator to next logical record
- * @iter: file iterator
- *
- * Return zero if new position is valid, non-zero if iterator has reached end.
- */
-int gcov_iter_next(struct gcov_iterator *iter)
-{
- switch (iter->record) {
- case RECORD_FILE_MAGIC:
- case RECORD_GCOV_VERSION:
- case RECORD_FUNCTION_TAG:
- case RECORD_FUNCTON_TAG_LEN:
- case RECORD_FUNCTION_IDENT:
- case RECORD_COUNT_TAG:
- /* Advance to next record */
- iter->record++;
- break;
- case RECORD_COUNT:
- /* Advance to next count */
- iter->count++;
- /* fall through */
- case RECORD_COUNT_LEN:
- if (iter->count < get_func(iter)->n_ctrs[iter->type]) {
- iter->record = 9;
- break;
- }
- /* Advance to next counter type */
- get_type(iter)->offset += iter->count;
- iter->count = 0;
- iter->type++;
- /* fall through */
- case RECORD_FUNCTION_CHECK:
- if (iter->type < iter->num_types) {
- iter->record = 7;
- break;
- }
- /* Advance to next function */
- iter->type = 0;
- iter->function++;
- /* fall through */
- case RECORD_TIME_STAMP:
- if (iter->function < iter->info->n_functions)
- iter->record = 3;
- else
- iter->record = -1;
- break;
- }
- /* Check for EOF. */
- if (iter->record == -1)
- return -EINVAL;
- else
- return 0;
-}
-
-/**
- * seq_write_gcov_u32 - write 32 bit number in gcov format to seq_file
- * @seq: seq_file handle
- * @v: value to be stored
- *
- * Number format defined by gcc: numbers are recorded in the 32 bit
- * unsigned binary form of the endianness of the machine generating the
- * file.
- */
-static int seq_write_gcov_u32(struct seq_file *seq, u32 v)
-{
- return seq_write(seq, &v, sizeof(v));
-}
-
-/**
- * seq_write_gcov_u64 - write 64 bit number in gcov format to seq_file
- * @seq: seq_file handle
- * @v: value to be stored
- *
- * Number format defined by gcc: numbers are recorded in the 32 bit
- * unsigned binary form of the endianness of the machine generating the
- * file. 64 bit numbers are stored as two 32 bit numbers, the low part
- * first.
- */
-static int seq_write_gcov_u64(struct seq_file *seq, u64 v)
-{
- u32 data[2];
-
- data[0] = (v & 0xffffffffUL);
- data[1] = (v >> 32);
- return seq_write(seq, data, sizeof(data));
-}
-
-/**
- * gcov_iter_write - write data for current pos to seq_file
- * @iter: file iterator
- * @seq: seq_file handle
- *
- * Return zero on success, non-zero otherwise.
- */
-int gcov_iter_write(struct gcov_iterator *iter, struct seq_file *seq)
-{
- int rc = -EINVAL;
-
- switch (iter->record) {
- case RECORD_FILE_MAGIC:
- rc = seq_write_gcov_u32(seq, GCOV_DATA_MAGIC);
- break;
- case RECORD_GCOV_VERSION:
- rc = seq_write_gcov_u32(seq, iter->info->version);
- break;
- case RECORD_TIME_STAMP:
- rc = seq_write_gcov_u32(seq, iter->info->stamp);
- break;
- case RECORD_FUNCTION_TAG:
- rc = seq_write_gcov_u32(seq, GCOV_TAG_FUNCTION);
- break;
- case RECORD_FUNCTON_TAG_LEN:
- rc = seq_write_gcov_u32(seq, 2);
- break;
- case RECORD_FUNCTION_IDENT:
- rc = seq_write_gcov_u32(seq, get_func(iter)->ident);
- break;
- case RECORD_FUNCTION_CHECK:
- rc = seq_write_gcov_u32(seq, get_func(iter)->checksum);
- break;
- case RECORD_COUNT_TAG:
- rc = seq_write_gcov_u32(seq,
- GCOV_TAG_FOR_COUNTER(get_type(iter)->ctr_type));
- break;
- case RECORD_COUNT_LEN:
- rc = seq_write_gcov_u32(seq,
- get_func(iter)->n_ctrs[iter->type] * 2);
- break;
- case RECORD_COUNT:
- rc = seq_write_gcov_u64(seq,
- iter->info->counts[iter->type].
- values[iter->count + get_type(iter)->offset]);
- break;
- }
- return rc;
-}
--
2.26.1.301.g55bc3eb7cb9-goog
Passing a volatile-qualified pointer to READ_ONCE() is an absolute
trainwreck for code generation: the use of 'typeof()' to define a
temporary variable inside the macro means that the final evaluation in
macro scope ends up forcing a read back from the stack. When stack
protector is enabled (the default for arm64, at least), this causes
the compiler to vomit up all sorts of junk.
Unfortunately, dropping pointer qualifiers inside the macro poses quite
a challenge, especially since the pointed-to type is permitted to be an
aggregate, and this is relied upon by mm/ code accessing things like
'pmd_t'. Based on numerous hacks and discussions on the mailing list,
this is the best I've managed to come up with.
Introduce '__unqual_scalar_typeof()' which takes an expression and, if
the expression is an optionally qualified 8, 16, 32 or 64-bit scalar
type, evaluates to the unqualified type. Other input types, including
aggregates, remain unchanged. Hopefully READ_ONCE() on volatile aggregate
pointers isn't something we do on a fast-path.
Cc: Peter Zijlstra <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Reported-by: Michael Ellerman <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
include/linux/compiler.h | 6 +++---
include/linux/compiler_types.h | 21 +++++++++++++++++++++
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 50bb2461648f..c363d8debc43 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -203,13 +203,13 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
* atomicity or dependency ordering guarantees. Note that this may result
* in tears!
*/
-#define __READ_ONCE(x) (*(const volatile typeof(x) *)&(x))
+#define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
#define __READ_ONCE_SCALAR(x) \
({ \
- typeof(x) __x = __READ_ONCE(x); \
+ __unqual_scalar_typeof(x) __x = __READ_ONCE(x); \
smp_read_barrier_depends(); \
- __x; \
+ (typeof(x))__x; \
})
#define READ_ONCE(x) \
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e970f97a7fcb..233066c92f6f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -210,6 +210,27 @@ struct ftrace_likely_data {
/* Are two types/vars the same type (ignoring qualifiers)? */
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
+/*
+ * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
+ * non-scalar types unchanged.
+ *
+ * We build this out of a couple of helper macros in a vain attempt to
+ * help you keep your lunch down while reading it.
+ */
+#define __pick_scalar_type(x, type, otherwise) \
+ __builtin_choose_expr(__same_type(x, type), (type)0, otherwise)
+
+#define __pick_integer_type(x, type, otherwise) \
+ __pick_scalar_type(x, unsigned type, \
+ __pick_scalar_type(x, signed type, otherwise))
+
+#define __unqual_scalar_typeof(x) typeof( \
+ __pick_integer_type(x, char, \
+ __pick_integer_type(x, short, \
+ __pick_integer_type(x, int, \
+ __pick_integer_type(x, long, \
+ __pick_integer_type(x, long long, x))))))
+
/* Is this type a native word size -- useful for atomic operations */
#define __native_word(t) \
(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
--
2.26.1.301.g55bc3eb7cb9-goog
On Wed, Apr 22, 2020 at 12:16 AM Will Deacon <[email protected]> wrote:
>
> The kernel requires at least GCC 4.8 in order to build, and so there is
> no need to cater for the pre-4.7 gcov format.
>
> Remove the obsolete code.
>
> Acked-by: Peter Oberparleiter <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
Reviewed-by: Masahiro Yamada <[email protected]>
Thanks!
--
Best Regards
Masahiro Yamada
On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
>
> It's me again. This is version four of the READ_ONCE() codegen improvement
> patches [...]
Let's just plan on biting the bullet and do this for 5.8. I'm assuming
that I'll juet get a pull request from you?
> (I'm interpreting the silence as monumental joy)
By now I think we can take that for granted.
Because "monumental joy" is certainly exactly what I felt re-reading
that "unqualified scalar type" macro.
Or maybe it was just my breakfast trying to say "Hi!".
Linus
On Tue, Apr 21, 2020 at 11:42:56AM -0700, Linus Torvalds wrote:
> On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
> >
> > It's me again. This is version four of the READ_ONCE() codegen improvement
> > patches [...]
>
> Let's just plan on biting the bullet and do this for 5.8. I'm assuming
> that I'll juet get a pull request from you?
Sure thing, thanks. I'll get it into -next along with the arm64 bits for
5.8, but I'll send it as a separate pull when the time comes. I'll also
include the sparc32 changes because otherwise the build falls apart and
we'll get an army of angry robots yelling at us (they seem to form the
majority of the active sparc32 user base afaict).
> > (I'm interpreting the silence as monumental joy)
>
> By now I think we can take that for granted.
>
> Because "monumental joy" is certainly exactly what I felt re-reading
> that "unqualified scalar type" macro.
>
> Or maybe it was just my breakfast trying to say "Hi!".
Haha! It's definitely best viewed on an empty stomach, but the comment
does give you ample warning.
Will
On 21/04/2020 17.15, Will Deacon wrote:
> Passing a volatile-qualified pointer to READ_ONCE() is an absolute
> trainwreck for code generation: the use of 'typeof()' to define a
> temporary variable inside the macro means that the final evaluation in
> macro scope ends up forcing a read back from the stack. When stack
> protector is enabled (the default for arm64, at least), this causes
> the compiler to vomit up all sorts of junk.
>
> Unfortunately, dropping pointer qualifiers inside the macro poses quite
> a challenge, especially since the pointed-to type is permitted to be an
> aggregate, and this is relied upon by mm/ code accessing things like
> 'pmd_t'. Based on numerous hacks and discussions on the mailing list,
> this is the best I've managed to come up with.
Hm, maybe this can be brought to work, only very lightly tested. It
basically abuses what -Wignored-qualifiers points out:
warning: type qualifiers ignored on function return type
Example showing the idea:
const int c(void);
volatile int v(void);
int hack(int x, int y)
{
typeof(c()) a = x;
typeof(v()) b = y;
a += b;
b += a;
a += b;
return a;
}
Since that compiles, a cannot be const-qualified, and the generated code
certainly suggests that b is not volatile-qualified. So something like
#define unqual_type(x) _unqual_type(x, unique_id_dance)
#define _unqual_type(x, id) typeof( ({
typeof(x) id(void);
id();
}) )
and perhaps some _Pragma("GCC diagnostic push")/_Pragma("GCC diagnostic
ignored -Wignored-qualifiers")/_Pragma("GCC diagnostic pop") could
prevent the warning (which is in -Wextra, so I don't think it would
appear in a normal build anyway).
No idea how well any of this would work across gcc versions or with clang.
Rasmus
On Wed, Apr 22, 2020 at 09:18:39AM +0100, Will Deacon wrote:
> On Tue, Apr 21, 2020 at 11:42:56AM -0700, Linus Torvalds wrote:
> > On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
> > >
> > > It's me again. This is version four of the READ_ONCE() codegen improvement
> > > patches [...]
> >
> > Let's just plan on biting the bullet and do this for 5.8. I'm assuming
> > that I'll juet get a pull request from you?
>
> Sure thing, thanks. I'll get it into -next along with the arm64 bits for
> 5.8, but I'll send it as a separate pull when the time comes. I'll also
> include the sparc32 changes because otherwise the build falls apart and
> we'll get an army of angry robots yelling at us (they seem to form the
> majority of the active sparc32 user base afaict).
So I'm obviously all for these patches; do note however that it collides
most mighty with the KCSAN stuff, which I believe is still pending.
Hi!
On Wed, Apr 22, 2020 at 12:25:03PM +0200, Rasmus Villemoes wrote:
> On 21/04/2020 17.15, Will Deacon wrote:
> > Unfortunately, dropping pointer qualifiers inside the macro poses quite
> > a challenge, especially since the pointed-to type is permitted to be an
> > aggregate, and this is relied upon by mm/ code accessing things like
> > 'pmd_t'. Based on numerous hacks and discussions on the mailing list,
> > this is the best I've managed to come up with.
>
> Hm, maybe this can be brought to work, only very lightly tested. It
> basically abuses what -Wignored-qualifiers points out:
>
> warning: type qualifiers ignored on function return type
>
> Example showing the idea:
>
> const int c(void);
> volatile int v(void);
>
> int hack(int x, int y)
> {
> typeof(c()) a = x;
> typeof(v()) b = y;
>
> a += b;
> b += a;
> a += b;
> return a;
> }
Nasty. I like it :-)
> Since that compiles, a cannot be const-qualified, and the generated code
> certainly suggests that b is not volatile-qualified. So something like
>
> #define unqual_type(x) _unqual_type(x, unique_id_dance)
> #define _unqual_type(x, id) typeof( ({
> typeof(x) id(void);
> id();
> }) )
>
> and perhaps some _Pragma("GCC diagnostic push")/_Pragma("GCC diagnostic
> ignored -Wignored-qualifiers")/_Pragma("GCC diagnostic pop") could
> prevent the warning (which is in -Wextra, so I don't think it would
> appear in a normal build anyway).
>
> No idea how well any of this would work across gcc versions or with clang.
https://gcc.gnu.org/legacy-ml/gcc-patches/2016-05/msg01054.html
This is defined to work this way in ISO C since C11.
But, it doesn't work with GCC before GCC 7 :-(
Segher
On Wed, Apr 22, 2020 at 01:37:21PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 22, 2020 at 09:18:39AM +0100, Will Deacon wrote:
> > On Tue, Apr 21, 2020 at 11:42:56AM -0700, Linus Torvalds wrote:
> > > On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
> > > >
> > > > It's me again. This is version four of the READ_ONCE() codegen improvement
> > > > patches [...]
> > >
> > > Let's just plan on biting the bullet and do this for 5.8. I'm assuming
> > > that I'll juet get a pull request from you?
> >
> > Sure thing, thanks. I'll get it into -next along with the arm64 bits for
> > 5.8, but I'll send it as a separate pull when the time comes. I'll also
> > include the sparc32 changes because otherwise the build falls apart and
> > we'll get an army of angry robots yelling at us (they seem to form the
> > majority of the active sparc32 user base afaict).
>
> So I'm obviously all for these patches; do note however that it collides
> most mighty with the KCSAN stuff, which I believe is still pending.
That stuff has been pending for the last two releases afaict :/
Anyway, I'm happy to either provide a branch with this series on, or do
the merge myself, or send this again based on something else. What works
best for you? The only thing I'd obviously like to avoid is tightly
coupling this to KCSAN if there's a chance of it missing the merge window
again.
Cheers,
Will
On Wed, Apr 22, 2020 at 06:48:07AM -0500, Segher Boessenkool wrote:
> On Wed, Apr 22, 2020 at 12:25:03PM +0200, Rasmus Villemoes wrote:
> > On 21/04/2020 17.15, Will Deacon wrote:
> > > Unfortunately, dropping pointer qualifiers inside the macro poses quite
> > > a challenge, especially since the pointed-to type is permitted to be an
> > > aggregate, and this is relied upon by mm/ code accessing things like
> > > 'pmd_t'. Based on numerous hacks and discussions on the mailing list,
> > > this is the best I've managed to come up with.
> >
> > Hm, maybe this can be brought to work, only very lightly tested. It
> > basically abuses what -Wignored-qualifiers points out:
> >
> > warning: type qualifiers ignored on function return type
> >
> > Example showing the idea:
> >
> > const int c(void);
> > volatile int v(void);
> >
> > int hack(int x, int y)
> > {
> > typeof(c()) a = x;
> > typeof(v()) b = y;
> >
> > a += b;
> > b += a;
> > a += b;
> > return a;
> > }
>
> Nasty. I like it :-)
>
> > Since that compiles, a cannot be const-qualified, and the generated code
> > certainly suggests that b is not volatile-qualified. So something like
> >
> > #define unqual_type(x) _unqual_type(x, unique_id_dance)
> > #define _unqual_type(x, id) typeof( ({
> > typeof(x) id(void);
> > id();
> > }) )
> >
> > and perhaps some _Pragma("GCC diagnostic push")/_Pragma("GCC diagnostic
> > ignored -Wignored-qualifiers")/_Pragma("GCC diagnostic pop") could
> > prevent the warning (which is in -Wextra, so I don't think it would
> > appear in a normal build anyway).
> >
> > No idea how well any of this would work across gcc versions or with clang.
>
> https://gcc.gnu.org/legacy-ml/gcc-patches/2016-05/msg01054.html
>
> This is defined to work this way in ISO C since C11.
>
> But, it doesn't work with GCC before GCC 7 :-(
Damn, that's quite a cool hack! Maybe we'll be able to implement it in a
few years time ;)
WIll
On Tue, Apr 21, 2020 at 04:15:34PM +0100, Will Deacon wrote:
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index e970f97a7fcb..233066c92f6f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -210,6 +210,27 @@ struct ftrace_likely_data {
> /* Are two types/vars the same type (ignoring qualifiers)? */
> #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>
> +/*
> + * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
> + * non-scalar types unchanged.
> + *
> + * We build this out of a couple of helper macros in a vain attempt to
> + * help you keep your lunch down while reading it.
> + */
> +#define __pick_scalar_type(x, type, otherwise) \
> + __builtin_choose_expr(__same_type(x, type), (type)0, otherwise)
> +
> +#define __pick_integer_type(x, type, otherwise) \
> + __pick_scalar_type(x, unsigned type, \
> + __pick_scalar_type(x, signed type, otherwise))
> +
> +#define __unqual_scalar_typeof(x) typeof( \
> + __pick_integer_type(x, char, \
Rasmus pointed out to me that 'char' is not __builtin_types_compatible_p()
with either 'signed char' or 'unsigned char', so I'll roll in the diff below
to handle this case.
Will
--->8
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 233066c92f6f..6ed0612bc143 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -220,9 +220,14 @@ struct ftrace_likely_data {
#define __pick_scalar_type(x, type, otherwise) \
__builtin_choose_expr(__same_type(x, type), (type)0, otherwise)
+/*
+ * 'char' is not type-compatible with either 'signed char' or 'unsigned char',
+ * so we include the naked type here as well as the signed/unsigned variants.
+ */
#define __pick_integer_type(x, type, otherwise) \
- __pick_scalar_type(x, unsigned type, \
- __pick_scalar_type(x, signed type, otherwise))
+ __pick_scalar_type(x, type, \
+ __pick_scalar_type(x, unsigned type, \
+ __pick_scalar_type(x, signed type, otherwise)))
#define __unqual_scalar_typeof(x) typeof( \
__pick_integer_type(x, char, \
Hi Peter,
[+KCSAN folks]
On Wed, Apr 22, 2020 at 01:26:27PM +0100, Will Deacon wrote:
> On Wed, Apr 22, 2020 at 01:37:21PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 22, 2020 at 09:18:39AM +0100, Will Deacon wrote:
> > > On Tue, Apr 21, 2020 at 11:42:56AM -0700, Linus Torvalds wrote:
> > > > On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
> > > > >
> > > > > It's me again. This is version four of the READ_ONCE() codegen improvement
> > > > > patches [...]
> > > >
> > > > Let's just plan on biting the bullet and do this for 5.8. I'm assuming
> > > > that I'll juet get a pull request from you?
> > >
> > > Sure thing, thanks. I'll get it into -next along with the arm64 bits for
> > > 5.8, but I'll send it as a separate pull when the time comes. I'll also
> > > include the sparc32 changes because otherwise the build falls apart and
> > > we'll get an army of angry robots yelling at us (they seem to form the
> > > majority of the active sparc32 user base afaict).
> >
> > So I'm obviously all for these patches; do note however that it collides
> > most mighty with the KCSAN stuff, which I believe is still pending.
>
> That stuff has been pending for the last two releases afaict :/
>
> Anyway, I'm happy to either provide a branch with this series on, or do
> the merge myself, or send this again based on something else. What works
> best for you? The only thing I'd obviously like to avoid is tightly
> coupling this to KCSAN if there's a chance of it missing the merge window
> again.
FWIW, I had a go at rebasing onto linux-next, just to get an idea for how
bad it is. It's fairly bad, and I don't think it's fair to inflict it on
sfr. I've included the interesting part of the resulting compiler.h below
for you and the KCSAN crowd to take a look at (yes, there's room for
subsequent cleanup, but I was focussing on the conflict resolution for now).
So, I think the best bet is either for my changes to go into -tip on top
of the KCSAN stuff, or for the KCSAN stuff to be dropped from -next (it's
been there since at least January). Do you know if they are definitely
supposed to be going in for 5.8?
Any other ideas?
Cheers,
Will
--->8
/*
* Prevent the compiler from merging or refetching reads or writes. The
* compiler is also forbidden from reordering successive instances of
* READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
* particular ordering. One way to make the compiler aware of ordering is to
* put the two invocations of READ_ONCE or WRITE_ONCE in different C
* statements.
*
* These two macros will also work on aggregate data types like structs or
* unions.
*
* Their two major use cases are: (1) Mediating communication between
* process-level code and irq/NMI handlers, all running on the same CPU,
* and (2) Ensuring that the compiler does not fold, spindle, or otherwise
* mutilate accesses that either do not require ordering or that interact
* with an explicit memory barrier or atomic instruction that provides the
* required ordering.
*/
#include <asm/barrier.h>
#include <linux/kasan-checks.h>
#include <linux/kcsan-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) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
#define __READ_ONCE_SCALAR(x) \
({ \
typeof(x) *__xp = &(x); \
kcsan_check_atomic_read(__xp, sizeof(*__xp)); \
kcsan_disable_current(); \
({ \
__unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp); \
kcsan_enable_current(); \
smp_read_barrier_depends(); \
(typeof(x))__x; \
}); \
})
#define READ_ONCE(x) \
({ \
compiletime_assert_rwonce_type(x); \
__READ_ONCE_SCALAR(x); \
})
#define __WRITE_ONCE(x, val) \
do { \
*(volatile typeof(x) *)&(x) = (val); \
} while (0)
#define __WRITE_ONCE_SCALAR(x, val) \
do { \
typeof(x) *__xp = &(x); \
kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
kcsan_disable_current(); \
__WRITE_ONCE(*__xp, val); \
kcsan_enable_current(); \
} while (0)
#define WRITE_ONCE(x, val) \
do { \
compiletime_assert_rwonce_type(x); \
__WRITE_ONCE_SCALAR(x, val); \
} while (0)
#ifdef CONFIG_KASAN
/*
* We can't declare function 'inline' because __no_sanitize_address conflicts
* with inlining. Attempt to inline it may cause a build failure.
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
* '__maybe_unused' allows us to avoid defined-but-not-used warnings.
*/
# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
# define __no_sanitize_or_inline __no_kasan_or_inline
#else
# define __no_kasan_or_inline __always_inline
#endif
#define __no_kcsan __no_sanitize_thread
#ifdef __SANITIZE_THREAD__
/*
* Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
* compilation units where instrumentation is disabled. The attribute 'noinline'
* is required for older compilers, where implicit inlining of very small
* functions renders __no_sanitize_thread ineffective.
*/
# define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
# define __no_sanitize_or_inline __no_kcsan_or_inline
#else
# define __no_kcsan_or_inline __always_inline
#endif
#ifndef __no_sanitize_or_inline
#define __no_sanitize_or_inline __always_inline
#endif
static __no_sanitize_or_inline
unsigned long __read_once_word_nocheck(const void *addr)
{
return __READ_ONCE(*(unsigned long *)addr);
}
/*
* Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need to load a
* word from memory atomically but without telling KASAN/KCSAN. This is
* usually used by unwinding code when walking the stack of a running process.
*/
#define READ_ONCE_NOCHECK(x) \
({ \
unsigned long __x = __read_once_word_nocheck(&(x)); \
smp_read_barrier_depends(); \
__x; \
})
static __no_kasan_or_inline
unsigned long read_word_at_a_time(const void *addr)
{
kasan_check_read(addr, 1);
return *(unsigned long *)addr;
}
/**
* data_race - mark an expression as containing intentional data races
*
* This data_race() macro is useful for situations in which data races
* should be forgiven. One example is diagnostic code that accesses
* shared variables but is not a part of the core synchronization design.
*
* This macro *does not* affect normal code generation, but is a hint
* to tooling that data races here are to be ignored.
*/
#define data_race(expr) \
({ \
typeof(({ expr; })) __val; \
kcsan_disable_current(); \
__val = ({ expr; }); \
kcsan_enable_current(); \
__val; \
})
On Fri, 24 Apr 2020 at 15:42, Will Deacon <[email protected]> wrote:
>
> Hi Peter,
>
> [+KCSAN folks]
>
> On Wed, Apr 22, 2020 at 01:26:27PM +0100, Will Deacon wrote:
> > On Wed, Apr 22, 2020 at 01:37:21PM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 22, 2020 at 09:18:39AM +0100, Will Deacon wrote:
> > > > On Tue, Apr 21, 2020 at 11:42:56AM -0700, Linus Torvalds wrote:
> > > > > On Tue, Apr 21, 2020 at 8:15 AM Will Deacon <[email protected]> wrote:
> > > > > >
> > > > > > It's me again. This is version four of the READ_ONCE() codegen improvement
> > > > > > patches [...]
> > > > >
> > > > > Let's just plan on biting the bullet and do this for 5.8. I'm assuming
> > > > > that I'll juet get a pull request from you?
> > > >
> > > > Sure thing, thanks. I'll get it into -next along with the arm64 bits for
> > > > 5.8, but I'll send it as a separate pull when the time comes. I'll also
> > > > include the sparc32 changes because otherwise the build falls apart and
> > > > we'll get an army of angry robots yelling at us (they seem to form the
> > > > majority of the active sparc32 user base afaict).
> > >
> > > So I'm obviously all for these patches; do note however that it collides
> > > most mighty with the KCSAN stuff, which I believe is still pending.
> >
> > That stuff has been pending for the last two releases afaict :/
> >
> > Anyway, I'm happy to either provide a branch with this series on, or do
> > the merge myself, or send this again based on something else. What works
> > best for you? The only thing I'd obviously like to avoid is tightly
> > coupling this to KCSAN if there's a chance of it missing the merge window
> > again.
>
> FWIW, I had a go at rebasing onto linux-next, just to get an idea for how
> bad it is. It's fairly bad, and I don't think it's fair to inflict it on
> sfr. I've included the interesting part of the resulting compiler.h below
> for you and the KCSAN crowd to take a look at (yes, there's room for
> subsequent cleanup, but I was focussing on the conflict resolution for now).
Thanks for the heads up. From what I can tell, your proposed change
may work fine for KCSAN. However, I've had trouble compiling this:
1. kcsan_disable_current() / kcsan_enable_current() do not work as-is,
because READ_ONCE/WRITE_ONCE seems to be used from compilation units
where the KCSAN runtime is not available (e.g.
arch/x86/entry/vdso/Makefile which had to set KCSAN_SANITIZE := n for
that reason).
2. Some new uaccess whitelist entries were needed.
I think this is what's needed:
https://lkml.kernel.org/r/[email protected]
With that you can change the calls to __kcsan_disable_current() /
__kcsan_enable_current() for READ_ONCE() and WRITE_ONCE(). After that,
I was able to compile, and my test suite passed.
Thanks,
-- Marco
> So, I think the best bet is either for my changes to go into -tip on top
> of the KCSAN stuff, or for the KCSAN stuff to be dropped from -next (it's
> been there since at least January). Do you know if they are definitely
> supposed to be going in for 5.8?
>
> Any other ideas?
>
> Cheers,
>
> Will
>
> --->8
>
> /*
> * Prevent the compiler from merging or refetching reads or writes. The
> * compiler is also forbidden from reordering successive instances of
> * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
> * particular ordering. One way to make the compiler aware of ordering is to
> * put the two invocations of READ_ONCE or WRITE_ONCE in different C
> * statements.
> *
> * These two macros will also work on aggregate data types like structs or
> * unions.
> *
> * Their two major use cases are: (1) Mediating communication between
> * process-level code and irq/NMI handlers, all running on the same CPU,
> * and (2) Ensuring that the compiler does not fold, spindle, or otherwise
> * mutilate accesses that either do not require ordering or that interact
> * with an explicit memory barrier or atomic instruction that provides the
> * required ordering.
> */
> #include <asm/barrier.h>
> #include <linux/kasan-checks.h>
> #include <linux/kcsan-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) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
>
> #define __READ_ONCE_SCALAR(x) \
> ({ \
> typeof(x) *__xp = &(x); \
> kcsan_check_atomic_read(__xp, sizeof(*__xp)); \
> kcsan_disable_current(); \
> ({ \
> __unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp); \
> kcsan_enable_current(); \
> smp_read_barrier_depends(); \
> (typeof(x))__x; \
> }); \
> })
>
> #define READ_ONCE(x) \
> ({ \
> compiletime_assert_rwonce_type(x); \
> __READ_ONCE_SCALAR(x); \
> })
>
> #define __WRITE_ONCE(x, val) \
> do { \
> *(volatile typeof(x) *)&(x) = (val); \
> } while (0)
>
> #define __WRITE_ONCE_SCALAR(x, val) \
> do { \
> typeof(x) *__xp = &(x); \
> kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
> kcsan_disable_current(); \
> __WRITE_ONCE(*__xp, val); \
> kcsan_enable_current(); \
> } while (0)
>
> #define WRITE_ONCE(x, val) \
> do { \
> compiletime_assert_rwonce_type(x); \
> __WRITE_ONCE_SCALAR(x, val); \
> } while (0)
>
> #ifdef CONFIG_KASAN
> /*
> * We can't declare function 'inline' because __no_sanitize_address conflicts
> * with inlining. Attempt to inline it may cause a build failure.
> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
> */
> # define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
> # define __no_sanitize_or_inline __no_kasan_or_inline
> #else
> # define __no_kasan_or_inline __always_inline
> #endif
>
> #define __no_kcsan __no_sanitize_thread
> #ifdef __SANITIZE_THREAD__
> /*
> * Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
> * compilation units where instrumentation is disabled. The attribute 'noinline'
> * is required for older compilers, where implicit inlining of very small
> * functions renders __no_sanitize_thread ineffective.
> */
> # define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
> # define __no_sanitize_or_inline __no_kcsan_or_inline
> #else
> # define __no_kcsan_or_inline __always_inline
> #endif
>
> #ifndef __no_sanitize_or_inline
> #define __no_sanitize_or_inline __always_inline
> #endif
>
> static __no_sanitize_or_inline
> unsigned long __read_once_word_nocheck(const void *addr)
> {
> return __READ_ONCE(*(unsigned long *)addr);
> }
>
> /*
> * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need to load a
> * word from memory atomically but without telling KASAN/KCSAN. This is
> * usually used by unwinding code when walking the stack of a running process.
> */
> #define READ_ONCE_NOCHECK(x) \
> ({ \
> unsigned long __x = __read_once_word_nocheck(&(x)); \
> smp_read_barrier_depends(); \
> __x; \
> })
Unconditionally loading an unsigned long doesn't seem right, and might
also result in OOB reads.
> static __no_kasan_or_inline
> unsigned long read_word_at_a_time(const void *addr)
> {
> kasan_check_read(addr, 1);
> return *(unsigned long *)addr;
> }
>
> /**
> * data_race - mark an expression as containing intentional data races
> *
> * This data_race() macro is useful for situations in which data races
> * should be forgiven. One example is diagnostic code that accesses
> * shared variables but is not a part of the core synchronization design.
> *
> * This macro *does not* affect normal code generation, but is a hint
> * to tooling that data races here are to be ignored.
> */
> #define data_race(expr) \
> ({ \
> typeof(({ expr; })) __val; \
> kcsan_disable_current(); \
> __val = ({ expr; }); \
> kcsan_enable_current(); \
> __val; \
> })
On Fri, Apr 24, 2020 at 05:54:10PM +0200, Marco Elver wrote:
> On Fri, 24 Apr 2020 at 15:42, Will Deacon <[email protected]> wrote:
> > On Wed, Apr 22, 2020 at 01:26:27PM +0100, Will Deacon wrote:
> > > On Wed, Apr 22, 2020 at 01:37:21PM +0200, Peter Zijlstra wrote:
> > > > So I'm obviously all for these patches; do note however that it collides
> > > > most mighty with the KCSAN stuff, which I believe is still pending.
> > >
> > > That stuff has been pending for the last two releases afaict :/
> > >
> > > Anyway, I'm happy to either provide a branch with this series on, or do
> > > the merge myself, or send this again based on something else. What works
> > > best for you? The only thing I'd obviously like to avoid is tightly
> > > coupling this to KCSAN if there's a chance of it missing the merge window
> > > again.
> >
> > FWIW, I had a go at rebasing onto linux-next, just to get an idea for how
> > bad it is. It's fairly bad, and I don't think it's fair to inflict it on
> > sfr. I've included the interesting part of the resulting compiler.h below
> > for you and the KCSAN crowd to take a look at (yes, there's room for
> > subsequent cleanup, but I was focussing on the conflict resolution for now).
>
> Thanks for the heads up. From what I can tell, your proposed change
> may work fine for KCSAN. However, I've had trouble compiling this:
>
> 1. kcsan_disable_current() / kcsan_enable_current() do not work as-is,
> because READ_ONCE/WRITE_ONCE seems to be used from compilation units
> where the KCSAN runtime is not available (e.g.
> arch/x86/entry/vdso/Makefile which had to set KCSAN_SANITIZE := n for
> that reason).
> 2. Some new uaccess whitelist entries were needed.
>
> I think this is what's needed:
> https://lkml.kernel.org/r/[email protected]
>
> With that you can change the calls to __kcsan_disable_current() /
> __kcsan_enable_current() for READ_ONCE() and WRITE_ONCE(). After that,
> I was able to compile, and my test suite passed.
Brill, thanks Marco! I'll take a look at your other patches, but I'm pretty
stuck until we've figured out the merging plan for all of this.
Will