On Fri, May 31, 2019 at 05:08:31PM +0200, Marco Elver wrote: > This adds a new header to asm-generic to allow optionally instrumenting > architecture-specific asm implementations of bitops. > > This change includes the required change for x86 as reference and > changes the kernel API doc to point to bitops-instrumented.h instead. > Rationale: the functions in x86's bitops.h are no longer the kernel API > functions, but instead the arch_ prefixed functions, which are then > instrumented via bitops-instrumented.h. > > Other architectures can similarly add support for asm implementations of > bitops. > > The documentation text was derived from x86 and existing bitops > asm-generic versions: 1) references to x86 have been removed; 2) as a > result, some of the text had to be reworded for clarity and consistency. > > Tested: using lib/test_kasan with bitops tests (pre-requisite patch). > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439 > Signed-off-by: Marco Elver <elver@xxxxxxxxxx> > --- > Changes in v3: > * Remove references to 'x86' in API documentation; as a result, had to > reword doc text for clarify and consistency. > * Remove #ifdef, since it is assumed that if asm-generic bitops > implementations are used, bitops-instrumented.h is not needed. Thanks for sorting this out. FWIW: Acked-by: Mark Rutland <mark.rutland@xxxxxxx> Mark. > > Changes in v2: > * Instrument word-sized accesses, as specified by the interface. > --- > Documentation/core-api/kernel-api.rst | 2 +- > arch/x86/include/asm/bitops.h | 189 ++++------------ > include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++ > 3 files changed, 302 insertions(+), 152 deletions(-) > create mode 100644 include/asm-generic/bitops-instrumented.h > > diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst > index a29c99d13331..65266fa1b706 100644 > --- a/Documentation/core-api/kernel-api.rst > +++ b/Documentation/core-api/kernel-api.rst > @@ -51,7 +51,7 @@ The Linux kernel provides more basic utility functions. > Bit Operations > -------------- > > -.. kernel-doc:: arch/x86/include/asm/bitops.h > +.. kernel-doc:: include/asm-generic/bitops-instrumented.h > :internal: > > Bitmap Operations > diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h > index 8e790ec219a5..ba15d53c1ca7 100644 > --- a/arch/x86/include/asm/bitops.h > +++ b/arch/x86/include/asm/bitops.h > @@ -49,23 +49,8 @@ > #define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3)) > #define CONST_MASK(nr) (1 << ((nr) & 7)) > > -/** > - * set_bit - Atomically set a bit in memory > - * @nr: the bit to set > - * @addr: the address to start counting from > - * > - * This function is atomic and may not be reordered. See __set_bit() > - * if you do not require the atomic guarantees. > - * > - * Note: there are no guarantees that this function will not be reordered > - * on non x86 architectures, so if you are writing portable code, > - * make sure not to rely on its reordering guarantees. > - * > - * Note that @nr may be almost arbitrarily large; this function is not > - * restricted to acting on a single-word quantity. > - */ > static __always_inline void > -set_bit(long nr, volatile unsigned long *addr) > +arch_set_bit(long nr, volatile unsigned long *addr) > { > if (IS_IMMEDIATE(nr)) { > asm volatile(LOCK_PREFIX "orb %1,%0" > @@ -78,32 +63,14 @@ set_bit(long nr, volatile unsigned long *addr) > } > } > > -/** > - * __set_bit - Set a bit in memory > - * @nr: the bit to set > - * @addr: the address to start counting from > - * > - * Unlike set_bit(), this function is non-atomic and may be reordered. > - * If it's called on the same region of memory simultaneously, the effect > - * may be that only one operation succeeds. > - */ > -static __always_inline void __set_bit(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch___set_bit(long nr, volatile unsigned long *addr) > { > asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory"); > } > > -/** > - * clear_bit - Clears a bit in memory > - * @nr: Bit to clear > - * @addr: Address to start counting from > - * > - * clear_bit() is atomic and may not be reordered. However, it does > - * not contain a memory barrier, so if it is used for locking purposes, > - * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic() > - * in order to ensure changes are visible on other processors. > - */ > static __always_inline void > -clear_bit(long nr, volatile unsigned long *addr) > +arch_clear_bit(long nr, volatile unsigned long *addr) > { > if (IS_IMMEDIATE(nr)) { > asm volatile(LOCK_PREFIX "andb %1,%0" > @@ -115,26 +82,21 @@ clear_bit(long nr, volatile unsigned long *addr) > } > } > > -/* > - * clear_bit_unlock - Clears a bit in memory > - * @nr: Bit to clear > - * @addr: Address to start counting from > - * > - * clear_bit() is atomic and implies release semantics before the memory > - * operation. It can be used for an unlock. > - */ > -static __always_inline void clear_bit_unlock(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch_clear_bit_unlock(long nr, volatile unsigned long *addr) > { > barrier(); > - clear_bit(nr, addr); > + arch_clear_bit(nr, addr); > } > > -static __always_inline void __clear_bit(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch___clear_bit(long nr, volatile unsigned long *addr) > { > asm volatile(__ASM_SIZE(btr) " %1,%0" : : ADDR, "Ir" (nr) : "memory"); > } > > -static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch_clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr) > { > bool negative; > asm volatile(LOCK_PREFIX "andb %2,%1" > @@ -143,48 +105,23 @@ static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile > : "ir" ((char) ~(1 << nr)) : "memory"); > return negative; > } > +#define arch_clear_bit_unlock_is_negative_byte \ > + arch_clear_bit_unlock_is_negative_byte > > -// Let everybody know we have it > -#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte > - > -/* > - * __clear_bit_unlock - Clears a bit in memory > - * @nr: Bit to clear > - * @addr: Address to start counting from > - * > - * __clear_bit() is non-atomic and implies release semantics before the memory > - * operation. It can be used for an unlock if no other CPUs can concurrently > - * modify other bits in the word. > - */ > -static __always_inline void __clear_bit_unlock(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch___clear_bit_unlock(long nr, volatile unsigned long *addr) > { > - __clear_bit(nr, addr); > + arch___clear_bit(nr, addr); > } > > -/** > - * __change_bit - Toggle a bit in memory > - * @nr: the bit to change > - * @addr: the address to start counting from > - * > - * Unlike change_bit(), this function is non-atomic and may be reordered. > - * If it's called on the same region of memory simultaneously, the effect > - * may be that only one operation succeeds. > - */ > -static __always_inline void __change_bit(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch___change_bit(long nr, volatile unsigned long *addr) > { > asm volatile(__ASM_SIZE(btc) " %1,%0" : : ADDR, "Ir" (nr) : "memory"); > } > > -/** > - * change_bit - Toggle a bit in memory > - * @nr: Bit to change > - * @addr: Address to start counting from > - * > - * change_bit() is atomic and may not be reordered. > - * Note that @nr may be almost arbitrarily large; this function is not > - * restricted to acting on a single-word quantity. > - */ > -static __always_inline void change_bit(long nr, volatile unsigned long *addr) > +static __always_inline void > +arch_change_bit(long nr, volatile unsigned long *addr) > { > if (IS_IMMEDIATE(nr)) { > asm volatile(LOCK_PREFIX "xorb %1,%0" > @@ -196,42 +133,20 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr) > } > } > > -/** > - * test_and_set_bit - Set a bit and return its old value > - * @nr: Bit to set > - * @addr: Address to count from > - * > - * This operation is atomic and cannot be reordered. > - * It also implies a memory barrier. > - */ > -static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch_test_and_set_bit(long nr, volatile unsigned long *addr) > { > return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr); > } > > -/** > - * test_and_set_bit_lock - Set a bit and return its old value for lock > - * @nr: Bit to set > - * @addr: Address to count from > - * > - * This is the same as test_and_set_bit on x86. > - */ > static __always_inline bool > -test_and_set_bit_lock(long nr, volatile unsigned long *addr) > +arch_test_and_set_bit_lock(long nr, volatile unsigned long *addr) > { > - return test_and_set_bit(nr, addr); > + return arch_test_and_set_bit(nr, addr); > } > > -/** > - * __test_and_set_bit - Set a bit and return its old value > - * @nr: Bit to set > - * @addr: Address to count from > - * > - * This operation is non-atomic and can be reordered. > - * If two examples of this operation race, one can appear to succeed > - * but actually fail. You must protect multiple accesses with a lock. > - */ > -static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch___test_and_set_bit(long nr, volatile unsigned long *addr) > { > bool oldbit; > > @@ -242,28 +157,13 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long * > return oldbit; > } > > -/** > - * test_and_clear_bit - Clear a bit and return its old value > - * @nr: Bit to clear > - * @addr: Address to count from > - * > - * This operation is atomic and cannot be reordered. > - * It also implies a memory barrier. > - */ > -static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch_test_and_clear_bit(long nr, volatile unsigned long *addr) > { > return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr); > } > > -/** > - * __test_and_clear_bit - Clear a bit and return its old value > - * @nr: Bit to clear > - * @addr: Address to count from > - * > - * This operation is non-atomic and can be reordered. > - * If two examples of this operation race, one can appear to succeed > - * but actually fail. You must protect multiple accesses with a lock. > - * > +/* > * Note: the operation is performed atomically with respect to > * the local CPU, but not other CPUs. Portable code should not > * rely on this behaviour. > @@ -271,7 +171,8 @@ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long * > * accessed from a hypervisor on the same CPU if running in a VM: don't change > * this without also updating arch/x86/kernel/kvm.c > */ > -static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch___test_and_clear_bit(long nr, volatile unsigned long *addr) > { > bool oldbit; > > @@ -282,8 +183,8 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long > return oldbit; > } > > -/* WARNING: non atomic and it can be reordered! */ > -static __always_inline bool __test_and_change_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch___test_and_change_bit(long nr, volatile unsigned long *addr) > { > bool oldbit; > > @@ -295,15 +196,8 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon > return oldbit; > } > > -/** > - * test_and_change_bit - Change a bit and return its old value > - * @nr: Bit to change > - * @addr: Address to count from > - * > - * This operation is atomic and cannot be reordered. > - * It also implies a memory barrier. > - */ > -static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr) > +static __always_inline bool > +arch_test_and_change_bit(long nr, volatile unsigned long *addr) > { > return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr); > } > @@ -326,16 +220,7 @@ static __always_inline bool variable_test_bit(long nr, volatile const unsigned l > return oldbit; > } > > -#if 0 /* Fool kernel-doc since it doesn't do macros yet */ > -/** > - * test_bit - Determine whether a bit is set > - * @nr: bit number to test > - * @addr: Address to start counting from > - */ > -static bool test_bit(int nr, const volatile unsigned long *addr); > -#endif > - > -#define test_bit(nr, addr) \ > +#define arch_test_bit(nr, addr) \ > (__builtin_constant_p((nr)) \ > ? constant_test_bit((nr), (addr)) \ > : variable_test_bit((nr), (addr))) > @@ -504,6 +389,8 @@ static __always_inline int fls64(__u64 x) > > #include <asm-generic/bitops/const_hweight.h> > > +#include <asm-generic/bitops-instrumented.h> > + > #include <asm-generic/bitops/le.h> > > #include <asm-generic/bitops/ext2-atomic-setbit.h> > diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h > new file mode 100644 > index 000000000000..ddd1c6d9d8db > --- /dev/null > +++ b/include/asm-generic/bitops-instrumented.h > @@ -0,0 +1,263 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +/* > + * This file provides wrappers with sanitizer instrumentation for bit > + * operations. > + * > + * To use this functionality, an arch's bitops.h file needs to define each of > + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(), > + * arch___set_bit(), etc.). > + */ > +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H > +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H > + > +#include <linux/kasan-checks.h> > + > +/** > + * set_bit - Atomically set a bit in memory > + * @nr: the bit to set > + * @addr: the address to start counting from > + * > + * This is a relaxed atomic operation (no implied memory barriers). > + * > + * Note that @nr may be almost arbitrarily large; this function is not > + * restricted to acting on a single-word quantity. > + */ > +static inline void set_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_set_bit(nr, addr); > +} > + > +/** > + * __set_bit - Set a bit in memory > + * @nr: the bit to set > + * @addr: the address to start counting from > + * > + * Unlike set_bit(), this function is non-atomic. If it is called on the same > + * region of memory concurrently, the effect may be that only one operation > + * succeeds. > + */ > +static inline void __set_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch___set_bit(nr, addr); > +} > + > +/** > + * clear_bit - Clears a bit in memory > + * @nr: Bit to clear > + * @addr: Address to start counting from > + * > + * This is a relaxed atomic operation (no implied memory barriers). > + */ > +static inline void clear_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_clear_bit(nr, addr); > +} > + > +/** > + * __clear_bit - Clears a bit in memory > + * @nr: the bit to clear > + * @addr: the address to start counting from > + * > + * Unlike clear_bit(), this function is non-atomic. If it is called on the same > + * region of memory concurrently, the effect may be that only one operation > + * succeeds. > + */ > +static inline void __clear_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch___clear_bit(nr, addr); > +} > + > +/** > + * clear_bit_unlock - Clear a bit in memory, for unlock > + * @nr: the bit to set > + * @addr: the address to start counting from > + * > + * This operation is atomic and provides release barrier semantics. > + */ > +static inline void clear_bit_unlock(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_clear_bit_unlock(nr, addr); > +} > + > +/** > + * __clear_bit_unlock - Clears a bit in memory > + * @nr: Bit to clear > + * @addr: Address to start counting from > + * > + * This is a non-atomic operation but implies a release barrier before the > + * memory operation. It can be used for an unlock if no other CPUs can > + * concurrently modify other bits in the word. > + */ > +static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch___clear_bit_unlock(nr, addr); > +} > + > +/** > + * change_bit - Toggle a bit in memory > + * @nr: Bit to change > + * @addr: Address to start counting from > + * > + * This is a relaxed atomic operation (no implied memory barriers). > + * > + * Note that @nr may be almost arbitrarily large; this function is not > + * restricted to acting on a single-word quantity. > + */ > +static inline void change_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_change_bit(nr, addr); > +} > + > +/** > + * __change_bit - Toggle a bit in memory > + * @nr: the bit to change > + * @addr: the address to start counting from > + * > + * Unlike change_bit(), this function is non-atomic. If it is called on the same > + * region of memory concurrently, the effect may be that only one operation > + * succeeds. > + */ > +static inline void __change_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + arch___change_bit(nr, addr); > +} > + > +/** > + * test_and_set_bit - Set a bit and return its old value > + * @nr: Bit to set > + * @addr: Address to count from > + * > + * This is an atomic fully-ordered operation (implied full memory barrier). > + */ > +static inline bool test_and_set_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch_test_and_set_bit(nr, addr); > +} > + > +/** > + * __test_and_set_bit - Set a bit and return its old value > + * @nr: Bit to set > + * @addr: Address to count from > + * > + * This operation is non-atomic. If two instances of this operation race, one > + * can appear to succeed but actually fail. > + */ > +static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch___test_and_set_bit(nr, addr); > +} > + > +/** > + * test_and_set_bit_lock - Set a bit and return its old value, for lock > + * @nr: Bit to set > + * @addr: Address to count from > + * > + * This operation is atomic and provides acquire barrier semantics if > + * the returned value is 0. > + * It can be used to implement bit locks. > + */ > +static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch_test_and_set_bit_lock(nr, addr); > +} > + > +/** > + * test_and_clear_bit - Clear a bit and return its old value > + * @nr: Bit to clear > + * @addr: Address to count from > + * > + * This is an atomic fully-ordered operation (implied full memory barrier). > + */ > +static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch_test_and_clear_bit(nr, addr); > +} > + > +/** > + * __test_and_clear_bit - Clear a bit and return its old value > + * @nr: Bit to clear > + * @addr: Address to count from > + * > + * This operation is non-atomic. If two instances of this operation race, one > + * can appear to succeed but actually fail. > + */ > +static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch___test_and_clear_bit(nr, addr); > +} > + > +/** > + * test_and_change_bit - Change a bit and return its old value > + * @nr: Bit to change > + * @addr: Address to count from > + * > + * This is an atomic fully-ordered operation (implied full memory barrier). > + */ > +static inline bool test_and_change_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch_test_and_change_bit(nr, addr); > +} > + > +/** > + * __test_and_change_bit - Change a bit and return its old value > + * @nr: Bit to change > + * @addr: Address to count from > + * > + * This operation is non-atomic. If two instances of this operation race, one > + * can appear to succeed but actually fail. > + */ > +static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch___test_and_change_bit(nr, addr); > +} > + > +/** > + * test_bit - Determine whether a bit is set > + * @nr: bit number to test > + * @addr: Address to start counting from > + */ > +static inline bool test_bit(long nr, const volatile unsigned long *addr) > +{ > + kasan_check_read(addr + BIT_WORD(nr), sizeof(long)); > + return arch_test_bit(nr, addr); > +} > + > +#if defined(arch_clear_bit_unlock_is_negative_byte) > +/** > + * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom > + * byte is negative, for unlock. > + * @nr: the bit to clear > + * @addr: the address to start counting from > + * > + * This operation is atomic and provides release barrier semantics. > + * > + * This is a bit of a one-trick-pony for the filemap code, which clears > + * PG_locked and tests PG_waiters, > + */ > +static inline bool > +clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr) > +{ > + kasan_check_write(addr + BIT_WORD(nr), sizeof(long)); > + return arch_clear_bit_unlock_is_negative_byte(nr, addr); > +} > +/* Let everybody know we have it. */ > +#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte > +#endif > + > +#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */ > -- > 2.22.0.rc1.257.g3120a18244-goog >
