Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] bitops: set_bit_release() lacks release barrier and uses kcsan_wmb() - [Medium] bitops: set_bit_release() undefined on archs without instrumented-atomic.h --
commit e20f77caba7042f428258dbb5808e1ad22ab5a60 Author: Philipp Stanner <[email protected]> asm-generic/bitops: Add set_bit_release() This commit adds set_bit_release() to include/asm-generic/bitops/instrumented-atomic.h to provide a bit-setting primitive with release ordering that pairs with test_bit_acquire(). > diff --git a/include/asm-generic/bitops/instrumented-atomic.h > b/include/asm-generic/bitops/instrumented-atomic.h > index c010d54275e4a..fb2d1472fd4d5 100644 > --- a/include/asm-generic/bitops/instrumented-atomic.h > +++ b/include/asm-generic/bitops/instrumented-atomic.h > @@ -72,6 +72,20 @@ static __always_inline bool test_and_set_bit(long nr, > volatile unsigned long *ad > return arch_test_and_set_bit(nr, addr); > } > > +/** > + * set_bit_release - Set a bit with release ordering > + * @nr: Bit to set > + * @addr: Address to start counting from > + * > + * This is an atomic ordered operation (implied write memory barrier). > + */ > +static __always_inline void set_bit_release(long nr, volatile unsigned long > *addr) [Severity: Medium] Will defining set_bit_release() only in include/asm-generic/bitops/instrumented-atomic.h break the build when generic kernel code calls set_bit_release() on architectures that do not include this header? Several architectures define their own atomic bitops directly in arch/*/include/asm/bitops.h without including include/asm-generic/bitops/instrumented-atomic.h or include/asm-generic/bitops/atomic.h, including 32-bit arm, mips, alpha, m68k, parisc, sparc, hexagon, openrisc, and sh: arch/arm/include/asm/bitops.h: #define set_bit(nr,p) ATOMIC_BITOP(set_bit,nr,p) #define clear_bit(nr,p) ATOMIC_BITOP(clear_bit,nr,p) #define change_bit(nr,p) ATOMIC_BITOP(change_bit,nr,p) arch/mips/include/asm/bitops.h: static inline void set_bit(unsigned long nr, volatile unsigned long *addr) Unlike clear_bit_unlock() (provided via include/asm-generic/bitops/lock.h and architecture-specific definitions) or test_bit_acquire() (wired through include/linux/bitops.h and include/asm-generic/bitops/non-atomic.h), set_bit_release() has no definition or fallback on those architectures. > +{ > + kcsan_wmb(); > + instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_set_bit(nr, addr); > +} [Severity: Critical] Does set_bit_release() in include/asm-generic/bitops/instrumented-atomic.h actually provide release memory ordering when a caller publishes state to a concurrent reader using test_bit_acquire()? First, arch_set_bit() is a relaxed (unordered) atomic operation. In include/asm-generic/bitops/atomic.h (used by arm64, loongarch, arc, csky, s390, and others), arch_set_bit() calls raw_atomic_long_or(), which is unordered and has no release barrier: include/asm-generic/bitops/atomic.h: static __always_inline void arch_set_bit(unsigned int nr, volatile unsigned long *p) { p += BIT_WORD(nr); raw_atomic_long_or(BIT_MASK(nr), (atomic_long_t *)p); } Architecture-specific implementations of arch_set_bit() in arch/powerpc/include/asm/bitops.h and arch/riscv/include/asm/bitops.h are likewise relaxed. By contrast, release-ordered bitops in include/asm-generic/bitops/lock.h use a release-ordered atomic operation: include/asm-generic/bitops/lock.h: static __always_inline void arch_clear_bit_unlock(unsigned int nr, volatile unsigned long *addr) { addr += BIT_WORD(nr); raw_atomic_long_fetch_andnot_release(BIT_MASK(nr), (atomic_long_t *)addr); } Second, kcsan_wmb() in include/linux/kcsan-checks.h is only a sanitizer annotation that compiles to a no-op when weak memory modeling is disabled and never emits a hardware memory barrier: include/linux/kcsan-checks.h: #define kcsan_wmb() do { } while (0) #define kcsan_rmb() do { } while (0) #define kcsan_release() do { } while (0) Third, both kcsan_wmb() and the function comment ("implied write memory barrier") model a write memory barrier rather than a release operation. In kernel/kcsan/core.c, wmb only orders prior writes, whereas release orders both prior reads and prior writes: kernel/kcsan/core.c: DEFINE_MEMORY_BARRIER(mb, true); DEFINE_MEMORY_BARRIER(wmb, sa->type & (KCSAN_ACCESS_WRITE | KCSAN_ACCESS_COMPOUND)); DEFINE_MEMORY_BARRIER(rmb, !(sa->type & KCSAN_ACCESS_WRITE) || (sa->type & KCSAN_ACCESS_COMPOUND)); DEFINE_MEMORY_BARRIER(release, true); Because there is no hardware or compiler release barrier here, can the compiler and processor on weakly-ordered architectures reorder prior memory reads and writes after arch_set_bit(), causing data races when paired with test_bit_acquire()? Should this follow clear_bit_unlock() in include/asm-generic/bitops/instrumented-lock.h by using kcsan_release() and a release-ordered arch_set_bit_release() helper? include/asm-generic/bitops/instrumented-lock.h: static inline void clear_bit_unlock(long nr, volatile unsigned long *addr) { kcsan_release(); instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); arch_clear_bit_unlock(nr, addr); } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
