Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[sashiko-bot@kernel.org]

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 <phasta@kernel.org>

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/20261021095512.1345191-2-phasta@kernel.org?part=1