* [PATCH 0/2] dma-fence: Order is-signaled-bit @ 2026-10-21 9:55 Philipp Stanner 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner 0 siblings, 2 replies; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 9:55 UTC (permalink / raw) To: Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch, Philipp Stanner Changes in v2: - Add a set_bit_release() function for proper ordering and use it. Based on the DRM branch drm-misc-next Philipp Stanner (2): asm-generic/bitops: Add set_bit_release() dma-buf/dma-fence: Order setting of the signaled-bit properly drivers/dma-buf/dma-fence.c | 11 +++++++++-- include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++ include/linux/dma-fence.h | 6 +++++- 3 files changed, 28 insertions(+), 3 deletions(-) base-commit: 1ecab8b0bad393750c60cc3819ce9c256595463c -- 2.56.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 9:55 [PATCH 0/2] dma-fence: Order is-signaled-bit Philipp Stanner @ 2026-10-21 9:55 ` Philipp Stanner 2026-10-21 10:06 ` sashiko-bot ` (2 more replies) 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner 1 sibling, 3 replies; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 9:55 UTC (permalink / raw) To: Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch, Philipp Stanner The kernel already has test_bit_acquire(). A set_bit_release(), however, was missing so far. It can be expected to be useful for parties that need release ordering, but not the stricter full ordering of the other bit-setting functions. Add set_bit_release(). Signed-off-by: Philipp Stanner <phasta@kernel.org> --- include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h index c010d54275e4..fb2d1472fd4d 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) +{ + kcsan_wmb(); + instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); + arch_set_bit(nr, addr); +} + /** * test_and_clear_bit - Clear a bit and return its old value * @nr: Bit to clear -- 2.56.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner @ 2026-10-21 10:06 ` sashiko-bot 2026-10-21 11:33 ` Arnd Bergmann 2026-10-21 12:06 ` Gary Guo 2 siblings, 0 replies; 17+ messages in thread From: sashiko-bot @ 2026-10-21 10:06 UTC (permalink / raw) To: Philipp Stanner; +Cc: dri-devel, media-ci 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner 2026-10-21 10:06 ` sashiko-bot @ 2026-10-21 11:33 ` Arnd Bergmann 2026-10-21 11:54 ` Gary Guo 2026-10-21 12:06 ` Gary Guo 2 siblings, 1 reply; 17+ messages in thread From: Arnd Bergmann @ 2026-10-21 11:33 UTC (permalink / raw) To: Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E. McKenney Cc: linux-media, dri-devel, linux-kernel, Linux-Arch On Wed, Oct 21, 2026, at 11:55, Philipp Stanner wrote: > The kernel already has test_bit_acquire(). A set_bit_release(), however, > was missing so far. It can be expected to be useful for parties that > need release ordering, but not the stricter full ordering of the other > bit-setting functions. > > Add set_bit_release(). > > Signed-off-by: Philipp Stanner <phasta@kernel.org> > +/** > + * 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). > + */ Can we fix the mismatch between code comments and documentation for the set_bit()/clear_bit()/change_bit() barrier semantics while adding this? I'm always confused by this and the new function makes it harder to understand in the current form: - Your patch description mentions that set_bit() has strong rw barrier semantics and that set_bit_release() would be a weaker write-barrier version of it. This makes sense given the set_bit() implementation based on strong raw_atomic_long_or() in arch_set_bit() and the text in Documentation/memory-barriers.txt - Contrary to that, the kerneldoc comment over set_bit() mentions "This is a relaxed atomic operation (no implied memory barriers)" and leaves out the kcsan_mb() that is in test_and_set_bit(). This makes the new set_bit_release() sound like a /stronger/ version of set_bit() the same way that test_bit_acquire() is a stronger version of the (relaxed) test_bit(). As I understand it, the kerneldoc comments here are incorrect and trace back to the include/asm-i386/bitops.h file before the git era. The patch below should make them consistent with your patch and with the actual implementation. Arnd diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h index c010d54275e4..df200802e929 100644 --- a/include/asm-generic/bitops/instrumented-atomic.h +++ b/include/asm-generic/bitops/instrumented-atomic.h @@ -18,13 +18,14 @@ * @nr: the bit to set * @addr: the address to start counting from * - * This is a relaxed atomic operation (no implied memory barriers). + * This is an atomic fully-ordered operation (implied full memory barrier). * * 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) { + kcsan_mb(); instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); arch_set_bit(nr, addr); } @@ -34,10 +35,11 @@ static __always_inline void set_bit(long nr, volatile unsigned long *addr) * @nr: Bit to clear * @addr: Address to start counting from * - * This is a relaxed atomic operation (no implied memory barriers). + * This is an atomic fully-ordered operation (implied full memory barrier). */ static __always_inline void clear_bit(long nr, volatile unsigned long *addr) { + kcsan_mb(); instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); arch_clear_bit(nr, addr); } @@ -47,13 +49,14 @@ static __always_inline void clear_bit(long nr, volatile unsigned long *addr) * @nr: Bit to change * @addr: Address to start counting from * - * This is a relaxed atomic operation (no implied memory barriers). + * This is an atomic fully-ordered operation (implied full memory barrier). * * 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) { + kcsan_mb(); instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); arch_change_bit(nr, addr); } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 11:33 ` Arnd Bergmann @ 2026-10-21 11:54 ` Gary Guo 2026-10-21 12:15 ` Arnd Bergmann 0 siblings, 1 reply; 17+ messages in thread From: Gary Guo @ 2026-10-21 11:54 UTC (permalink / raw) To: Arnd Bergmann, Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E. McKenney Cc: linux-media, dri-devel, linux-kernel, Linux-Arch On Wed Oct 21, 2026 at 12:33 PM BST, Arnd Bergmann wrote: > On Wed, Oct 21, 2026, at 11:55, Philipp Stanner wrote: >> The kernel already has test_bit_acquire(). A set_bit_release(), however, >> was missing so far. It can be expected to be useful for parties that >> need release ordering, but not the stricter full ordering of the other >> bit-setting functions. >> >> Add set_bit_release(). >> >> Signed-off-by: Philipp Stanner <phasta@kernel.org> > >> +/** >> + * 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). >> + */ > > Can we fix the mismatch between code comments and documentation for > the set_bit()/clear_bit()/change_bit() barrier semantics while adding > this? I'm always confused by this and the new function makes it harder > to understand in the current form: > > - Your patch description mentions that set_bit() has strong rw > barrier semantics and that set_bit_release() would be a weaker > write-barrier version of it. This makes sense given the > set_bit() implementation based on strong raw_atomic_long_or() > in arch_set_bit() and the text in Documentation/memory-barriers.txt > > - Contrary to that, the kerneldoc comment over set_bit() mentions > "This is a relaxed atomic operation (no implied memory barriers)" > and leaves out the kcsan_mb() that is in test_and_set_bit(). > This makes the new set_bit_release() sound like a /stronger/ > version of set_bit() the same way that test_bit_acquire() > is a stronger version of the (relaxed) test_bit(). set_bit has relaxed semantics while test_and_set_bit implies barrier. RISC-V for exmaple will generate the amoor instruction for set_bit, not amoor.aqrl. Best, Gary > > As I understand it, the kerneldoc comments here are incorrect > and trace back to the include/asm-i386/bitops.h file before the > git era. The patch below should make them consistent with your > patch and with the actual implementation. > > Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 11:54 ` Gary Guo @ 2026-10-21 12:15 ` Arnd Bergmann 0 siblings, 0 replies; 17+ messages in thread From: Arnd Bergmann @ 2026-10-21 12:15 UTC (permalink / raw) To: Gary Guo, Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Tvrtko Ursulin, Danilo Krummrich, Paul E. McKenney Cc: linux-media, dri-devel, linux-kernel, Linux-Arch On Wed, Oct 21, 2026, at 13:54, Gary Guo wrote: > On Wed Oct 21, 2026 at 12:33 PM BST, Arnd Bergmann wrote: >> I'm always confused by this and the new function makes it harder >> to understand in the current form: ... >> - Contrary to that, the kerneldoc comment over set_bit() mentions >> "This is a relaxed atomic operation (no implied memory barriers)" >> and leaves out the kcsan_mb() that is in test_and_set_bit(). >> This makes the new set_bit_release() sound like a /stronger/ >> version of set_bit() the same way that test_bit_acquire() >> is a stronger version of the (relaxed) test_bit(). > > set_bit has relaxed semantics while test_and_set_bit implies barrier. RISC-V for > exmaple will generate the amoor instruction for set_bit, not amoor.aqrl. Thanks for the clarification, I did misread the implementation and see how I was confused once more. Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner 2026-10-21 10:06 ` sashiko-bot 2026-10-21 11:33 ` Arnd Bergmann @ 2026-10-21 12:06 ` Gary Guo 2026-10-21 12:17 ` Philipp Stanner 2 siblings, 1 reply; 17+ messages in thread From: Gary Guo @ 2026-10-21 12:06 UTC (permalink / raw) To: Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On Wed Oct 21, 2026 at 10:55 AM BST, Philipp Stanner wrote: > The kernel already has test_bit_acquire(). A set_bit_release(), however, > was missing so far. It can be expected to be useful for parties that > need release ordering, but not the stricter full ordering of the other > bit-setting functions. > > Add set_bit_release(). > > Signed-off-by: Philipp Stanner <phasta@kernel.org> > --- > include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h > index c010d54275e4..fb2d1472fd4d 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) > +{ > + kcsan_wmb(); > + instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); > + arch_set_bit(nr, addr); This is just wrong. There's no code here that gives you the release semantics. kcsan_wmb() is just an annotation and does nothing. wmb is also not release semantics as it does not order LOAD->STORE. Given that set_bit is a relaxed RMW atomic, you can implement set_bit_release with just smp_mb__before_atomic(); set_bit(nr, addr); Which should be correct, but is too strong on some architectures (like RISC-V), becuase it will produce a full MB instead. So we can either add release barriers (hint: I really like it), or we can add arch_set_bit_release and implement it per arch; on RISC-V for example it can then be translated to amoor.rl (which is not being produced by any existing bit ops).c Best, Gary > +} > + > /** > * test_and_clear_bit - Clear a bit and return its old value > * @nr: Bit to clear ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 12:06 ` Gary Guo @ 2026-10-21 12:17 ` Philipp Stanner 2026-10-21 12:24 ` Gary Guo 0 siblings, 1 reply; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 12:17 UTC (permalink / raw) To: Gary Guo, Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On Wed, 2026-10-21 at 13:06 +0100, Gary Guo wrote: […] > > +/** > > + * 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) > > +{ > > + kcsan_wmb(); > > + instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); > > + arch_set_bit(nr, addr); > > This is just wrong. There's no code here that gives you the release semantics. > > kcsan_wmb() is just an annotation and does nothing. wmb is also not release > semantics as it does not order LOAD->STORE. What are the other functions here then even about? /** * 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 __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) { kcsan_mb(); instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long)); return arch_test_and_set_bit(nr, addr); } Where does the "full ordering" come from? I suppose implied barrier then means it comes from the architecture's implementation. But what's then the purpose of kcsan_XX ? > Given that set_bit is a relaxed RMW atomic, you can implement set_bit_release > with just > > smp_mb__before_atomic(); > set_bit(nr, addr); > > Which should be correct, but is too strong on some architectures (like RISC-V), > becuase it will produce a full MB instead. So we can either add release barriers > (hint: I really like it), or we can add arch_set_bit_release and implement it > per arch; on RISC-V for example it can then be translated to amoor.rl (which is > not being produced by any existing bit ops).c All fine by me. P. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 12:17 ` Philipp Stanner @ 2026-10-21 12:24 ` Gary Guo 2026-10-21 13:22 ` Philipp Stanner 0 siblings, 1 reply; 17+ messages in thread From: Gary Guo @ 2026-10-21 12:24 UTC (permalink / raw) To: phasta, Gary Guo, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On Wed Oct 21, 2026 at 1:17 PM BST, Philipp Stanner wrote: > On Wed, 2026-10-21 at 13:06 +0100, Gary Guo wrote: > > > […] > >> > +/** >> > + * 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) >> > +{ >> > + kcsan_wmb(); >> > + instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); >> > + arch_set_bit(nr, addr); >> >> This is just wrong. There's no code here that gives you the release semantics. >> >> kcsan_wmb() is just an annotation and does nothing. wmb is also not release >> semantics as it does not order LOAD->STORE. > > What are the other functions here then even about? > > /** > * 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 __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) > { > kcsan_mb(); > instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long)); > return arch_test_and_set_bit(nr, addr); > } > > Where does the "full ordering" come from? I suppose implied barrier > then means it comes from the architecture's implementation. But what's > then the purpose of kcsan_XX ? It's from arch_test_and_set_bit. KCSAN doesn't see arch code, so the kcsan_mb() and in fact instrumnent_atomic_read_write() both are annotations to indicate that this is conceptually a mb() + STORE for instrumentation purpose. Best, Gary ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release() 2026-10-21 12:24 ` Gary Guo @ 2026-10-21 13:22 ` Philipp Stanner 0 siblings, 0 replies; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 13:22 UTC (permalink / raw) To: Gary Guo, phasta, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On Wed, 2026-10-21 at 13:24 +0100, Gary Guo wrote: > On Wed Oct 21, 2026 at 1:17 PM BST, Philipp Stanner wrote: […] > > static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) > > { > > kcsan_mb(); > > instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long)); > > return arch_test_and_set_bit(nr, addr); > > } > > > > Where does the "full ordering" come from? I suppose implied barrier > > then means it comes from the architecture's implementation. But what's > > then the purpose of kcsan_XX ? > > It's from arch_test_and_set_bit. > > KCSAN doesn't see arch code, so the kcsan_mb() and in fact > instrumnent_atomic_read_write() both are annotations to indicate that this is > conceptually a mb() + STORE for instrumentation purpose. We (i.e., someone who's got the overview) could probably document where and why our atomic instructions are a bit scattered. Why is test_bit_acquire() defined in include/linux/bitops.h, and should set_bit_release() be defined there – or here? P. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 9:55 [PATCH 0/2] dma-fence: Order is-signaled-bit Philipp Stanner 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner @ 2026-10-21 9:55 ` Philipp Stanner 2026-10-21 10:16 ` sashiko-bot ` (3 more replies) 1 sibling, 4 replies; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 9:55 UTC (permalink / raw) To: Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch, Philipp Stanner dma_fence_is_signaled() can return true *while* the fence is still signaling. This is fragile and has been involved in at least two bugs: commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race") commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM") The signaled-bit is set under lock-protection but read without it. Adding the lock has been rejected on the grounds of the lock-acquisition shooting down cache lines of parties that merely want to check whether the fence is already signaled. Make sure the bit can only become visible as 1 once the callbacks have ran. To achieve this, read the bit with test_bit_acquire() and set it with set_bit_release(). Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/dma-buf/dma-fence.c | 11 +++++++++-- include/linux/dma-fence.h | 6 +++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index bd58688b81a7..07d499fa35ac 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -359,8 +359,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, dma_fence_assert_held(fence); - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, - &fence->flags))) + if (unlikely(dma_fence_test_signaled_flag(fence))) return; trace_dma_fence_signaled(fence); @@ -384,6 +383,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, INIT_LIST_HEAD(&cur->node); cur->func(fence, cur); } + + /* + * This orders the signaled-state with the test_bit_acquire() in + * dma_fence_test_signaled_flag(). This ensures that functions like + * dma_fence_is_signaled() can only ever return true once signaling is + * completed (i.e., after the callbacks above ran). + */ + set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); } EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index dd07d128adc3..61522027af41 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -496,7 +496,11 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); static inline bool dma_fence_test_signaled_flag(struct dma_fence *fence) { - return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); + /* + * The counterpart of this ordered operation can be found in + * dma_fence_signal_timestamp_locked(). + */ + return test_bit_acquire(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); } /** -- 2.56.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner @ 2026-10-21 10:16 ` sashiko-bot 2026-10-21 13:47 ` Christian König ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: sashiko-bot @ 2026-10-21 10:16 UTC (permalink / raw) To: Philipp Stanner; +Cc: dri-devel, media-ci Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] dma-fence: deadlock and UAF in dma_fence_release() on put in callback - [Critical] dma-fence: broken state and re-entrancy in dma_fence_signal_timestamp_locked() - [High] dma-fence: lost wakeups in waiters when running callbacks before setting bit - [High] dma-fence: build failure and missing barrier from set_bit_release() -- commit 1792aa61ea7d70257b20df7aab8e20f6da404551 Author: Philipp Stanner <phasta@kernel.org> dma-buf/dma-fence: Order setting of the signaled-bit properly This commit defers setting DMA_FENCE_FLAG_SIGNALED_BIT in dma_fence_signal_timestamp_locked() until after all fence callbacks have completed, using set_bit_release() and test_bit_acquire() to order the signaled state. > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index bd58688b81a72..07d499fa35ac9 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -359,8 +359,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, > > dma_fence_assert_held(fence); > > - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > - &fence->flags))) > + if (unlikely(dma_fence_test_signaled_flag(fence))) > return; [Severity: Critical] Previously, test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) marked the fence as signaled at entry before clearing fence->ops, replacing fence->cb_list with fence->timestamp, and invoking callbacks. With DMA_FENCE_FLAG_SIGNALED_BIT remaining zero until the end of dma_fence_signal_timestamp_locked(), what happens if a callback queries or signals the fence? In include/linux/dma-fence.h: static inline bool dma_fence_is_signaled_locked(struct dma_fence *fence) { const struct dma_fence_ops *ops; if (dma_fence_test_signaled_flag(fence)) return true; rcu_read_lock(); ops = rcu_dereference(fence->ops); if (ops && ops->signaled && ops->signaled(fence)) { rcu_read_unlock(); dma_fence_signal_locked(fence); return true; } rcu_read_unlock(); return false; } If a callback calls dma_fence_signal_locked(fence), or calls dma_fence_is_signaled_locked(fence) or dma_fence_get_status_locked(fence) when ops->signaled(fence) returns true, dma_fence_signal_timestamp_locked() is re-entered while DMA_FENCE_FLAG_SIGNALED_BIT is still zero. If fence->ops was cleared to NULL (when !ops->release && !ops->wait), the recursive call dereferences a NULL ops pointer; otherwise, list_replace(&fence->cb_list, &cb_list) runs on the union already overwritten by fence->timestamp, corrupting memory by treating the timestamp as list pointers. Conversely, if ops->signaled is NULL (or fence->ops was cleared), calling dma_fence_is_signaled_locked(fence) or dma_fence_get_status_locked(fence) from a callback now returns false or 0 for a fence that is being signaled. If a callback instead calls unlocked helpers such as dma_fence_is_signaled(fence) (when ops->signaled returns true) or dma_fence_add_callback(fence, ...), dma_fence_test_signaled_flag(fence) returns false and those helpers attempt to re-acquire fence->lock on the same CPU, causing a spinlock self-deadlock. Similarly, in include/linux/dma-fence.h: static inline ktime_t dma_fence_timestamp(struct dma_fence *fence) { if (WARN_ON(!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) return ktime_get(); while (!test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags)) cpu_relax(); return fence->timestamp; } Won't calling dma_fence_timestamp(fence) from a fence callback (or from a worker woken by a callback before set_bit_release() runs) now trigger the WARN_ON and return ktime_get() even though fence->timestamp and DMA_FENCE_FLAG_TIMESTAMP_BIT have already been set? > > trace_dma_fence_signaled(fence); > @@ -384,6 +383,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, > INIT_LIST_HEAD(&cur->node); > cur->func(fence, cur); > } [Severity: Critical] In struct dma_fence, cb_list, timestamp, and rcu share a union whose comment notes that cb_list is only valid until the signal bit is set: union { struct list_head cb_list; /* @cb_list replaced by @timestamp on dma_fence_signal() */ ktime_t timestamp; /* @timestamp replaced by @rcu on dma_fence_release() */ struct rcu_head rcu; }; In dma_fence_signal_timestamp_locked(), list_replace(&fence->cb_list, &cb_list) and fence->timestamp = timestamp overwrite that union before cur->func(fence, cur) runs, while DMA_FENCE_FLAG_SIGNALED_BIT is still 0. Many in-tree callbacks drop a reference to the fence via dma_fence_put() inside cur->func(), such as dma_fence_chain_cb(), dma_buf_poll_cb(), drm_sched_entity_wakeup(), drm_sched_entity_kill_jobs_cb(), vmw_event_fence_action_seq_passed(), amdgpu_pasid_free_cb(), and amdgpu_display_flip_callback(). When the signaler does not hold an extra reference across dma_fence_signal_locked() (for example in sw_sync_debugfs_release() or __vmw_fences_update()), or when a waiter woken by cur->func() drops the last reference before set_bit_release() executes, dma_fence_put() invokes dma_fence_release(): if (!list_empty(&fence->cb_list) && !dma_fence_test_signaled_flag(fence)) { ... WARN(1, "Fence %s:%s:%llx:%llx released with pending signals!\n", ...); dma_fence_lock_irqsave(fence, flags); fence->error = -EDEADLK; dma_fence_signal_locked(fence); dma_fence_unlock_irqrestore(fence, flags); } Because fence->cb_list now holds fence->timestamp (making !list_empty() evaluate to true) and DMA_FENCE_FLAG_SIGNALED_BIT is not yet set, won't dma_fence_release() emit a false WARN, self-deadlock on fence->lock when called from cur->func() (or re-enter dma_fence_signal_locked() on a corrupted cb_list from another CPU), and then release or free the fence before dma_fence_signal_timestamp_locked() writes to fence->flags and unlocks fence->inline_lock? > + > + /* > + * This orders the signaled-state with the test_bit_acquire() in > + * dma_fence_test_signaled_flag(). This ensures that functions like > + * dma_fence_is_signaled() can only ever return true once signaling is > + * completed (i.e., after the callbacks above ran). > + */ [Severity: High] Could deferring DMA_FENCE_FLAG_SIGNALED_BIT until after the callbacks have run cause lost wakeups for waiters, pollers, and work items that are woken by cur->func()? Waiters register a one-shot callback with dma_fence_add_callback() that wakes the task or waitqueue (for example dma_fence_default_wait_cb(), syncobj_wait_fence_func(), fence_check_cb_func(), and radeon_fence_wait_cb()), and then check whether the fence is signaled without acquiring fence->lock. For instance, in drivers/dma-buf/dma-fence.c:dma_fence_wait_any_timeout(): while (ret > 0) { if (intr) set_current_state(TASK_INTERRUPTIBLE); else set_current_state(TASK_UNINTERRUPTIBLE); if (dma_fence_test_signaled_any(fences, count, idx)) break; ret = schedule_timeout(ret); If CPU0 runs dma_fence_default_wait_cb() and wakes the waiting task on CPU1 before CPU0 finishes the callback loop and calls set_bit_release(), CPU1 can set its task state and call dma_fence_test_signaled_any() -> dma_fence_test_signaled_flag(fence), observe DMA_FENCE_FLAG_SIGNALED_BIT == 0, and go to sleep in schedule_timeout(ret) with its callback already removed and executed. Doesn't the same race also affect: 1. drm_syncobj_array_wait_timeout() in drivers/gpu/drm/drm_syncobj.c, where woken task checks dma_fence_is_signaled(fence) while entries[i].fence_cb.func is already non-NULL (and after fence->ops was cleared to NULL), sees false, and sleeps again in schedule_timeout()? 2. sync_file_poll() in drivers/dma-buf/sync_file.c, where fence_check_cb_func() wakes sync_file->wq while DMA_FENCE_FLAG_SIGNALED_BIT is still 0, causing the woken poller to see POLL_ENABLED already set and dma_fence_is_signaled(sync_file->fence) returning false, missing EPOLLIN? 3. radeon_fence_default_wait() in drivers/gpu/drm/radeon/radeon_fence.c and __i915_request_wait() in drivers/gpu/drm/i915/i915_request.c, which wake the waiter from a fence callback and test DMA_FENCE_FLAG_SIGNALED_BIT before calling schedule_timeout()? 4. nouveau_cli_work() in drivers/gpu/drm/nouveau/nouveau_drm.c, where nouveau_cli_work_fence() schedules cli->work from the callback, and if nouveau_cli_work() runs before set_bit_release() and sees dma_fence_is_signaled(fence) == false, it skips the work item with no further callback to reschedule it? > + set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); [Severity: High] Does set_bit_release() build on all architectures and provide a hardware release barrier on weakly-ordered CPUs? set_bit_release() is only defined in include/asm-generic/bitops/instrumented-atomic.h: static __always_inline void set_bit_release(long nr, volatile unsigned long *addr) { kcsan_wmb(); instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long)); arch_set_bit(nr, addr); } Architectures that define their own atomic bitops without including asm-generic/bitops/instrumented-atomic.h (such as 32-bit arm, mips, sparc, parisc, m68k, alpha, sh, openrisc, and hexagon) do not declare set_bit_release(), which breaks compiling drivers/dma-buf/dma-fence.c. Furthermore, on architectures that do include asm-generic/bitops/instrumented-atomic.h (such as arm64, riscv, powerpc, and loongarch), kcsan_wmb() is only a KCSAN annotation (a no-op when CONFIG_KCSAN=n) and arch_set_bit() is a relaxed atomic operation without release semantics: 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); } Unlike test_and_set_bit(), which implies a full memory barrier, doesn't set_bit_release() omit the CPU release barrier needed to pair with test_bit_acquire() on weakly-ordered architectures? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20261021095512.1345191-2-phasta@kernel.org?part=2 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner 2026-10-21 10:16 ` sashiko-bot @ 2026-10-21 13:47 ` Christian König 2026-10-21 14:52 ` Philipp Stanner 2026-10-26 1:34 ` kernel test robot 2026-10-26 7:48 ` kernel test robot 3 siblings, 1 reply; 17+ messages in thread From: Christian König @ 2026-10-21 13:47 UTC (permalink / raw) To: Philipp Stanner, Sumit Semwal, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On 10/21/26 11:55, Philipp Stanner wrote: > dma_fence_is_signaled() can return true *while* the fence is still > signaling. This is fragile and has been involved in at least two bugs: > > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race") > commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM") > > The signaled-bit is set under lock-protection but read without it. > Adding the lock has been rejected on the grounds of the lock-acquisition > shooting down cache lines of parties that merely want to check whether > the fence is already signaled. > > Make sure the bit can only become visible as 1 once the callbacks have > ran. To achieve this, read the bit with test_bit_acquire() and set it > with set_bit_release(). That has been suggested and rejected before. Background is that the signaling bit must be visible before the callbacks are run or things like dma_fence_default_wait() (as an example) just break. Regards, Christian. > > Signed-off-by: Philipp Stanner <phasta@kernel.org> > --- > drivers/dma-buf/dma-fence.c | 11 +++++++++-- > include/linux/dma-fence.h | 6 +++++- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index bd58688b81a7..07d499fa35ac 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -359,8 +359,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, > > dma_fence_assert_held(fence); > > - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > - &fence->flags))) > + if (unlikely(dma_fence_test_signaled_flag(fence))) > return; > > trace_dma_fence_signaled(fence); > @@ -384,6 +383,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, > INIT_LIST_HEAD(&cur->node); > cur->func(fence, cur); > } > + > + /* > + * This orders the signaled-state with the test_bit_acquire() in > + * dma_fence_test_signaled_flag(). This ensures that functions like > + * dma_fence_is_signaled() can only ever return true once signaling is > + * completed (i.e., after the callbacks above ran). > + */ > + set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); > } > EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index dd07d128adc3..61522027af41 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -496,7 +496,11 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence); > static inline bool > dma_fence_test_signaled_flag(struct dma_fence *fence) > { > - return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); > + /* > + * The counterpart of this ordered operation can be found in > + * dma_fence_signal_timestamp_locked(). > + */ > + return test_bit_acquire(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); > } > > /** ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 13:47 ` Christian König @ 2026-10-21 14:52 ` Philipp Stanner 2026-10-21 15:04 ` Christian König 0 siblings, 1 reply; 17+ messages in thread From: Philipp Stanner @ 2026-10-21 14:52 UTC (permalink / raw) To: Christian König, Philipp Stanner, Sumit Semwal, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On Wed, 2026-10-21 at 15:47 +0200, Christian König wrote: > On 10/21/26 11:55, Philipp Stanner wrote: > > dma_fence_is_signaled() can return true *while* the fence is still > > signaling. This is fragile and has been involved in at least two bugs: > > > > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race") > > commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM") > > > > The signaled-bit is set under lock-protection but read without it. > > Adding the lock has been rejected on the grounds of the lock-acquisition > > shooting down cache lines of parties that merely want to check whether > > the fence is already signaled. > > > > Make sure the bit can only become visible as 1 once the callbacks have > > ran. To achieve this, read the bit with test_bit_acquire() and set it > > with set_bit_release(). > > That has been suggested and rejected before. > > Background is that the signaling bit must be visible before the > callbacks are run or things like dma_fence_default_wait() (as an > example) just break. dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) { struct default_wait_cb cb; unsigned long flags; signed long ret = timeout ? timeout : 1; dma_fence_lock_irqsave(fence, flags); if (dma_fence_test_signaled_flag(fence)) goto out; if (intr && signal_pending(current)) { ret = -ERESTARTSYS; goto out; } if (!timeout) { ret = 0; goto out; } cb.base.func = dma_fence_default_wait_cb; cb.task = current; list_add(&cb.base.node, &fence->cb_list); while (!dma_fence_test_signaled_flag(fence) && ret > 0) { if (intr) __set_current_state(TASK_INTERRUPTIBLE); else __set_current_state(TASK_UNINTERRUPTIBLE); dma_fence_unlock_irqrestore(fence, flags); That function evaluates the flag under lock-protection, with full ordering. The bit is only ever set under lock-protection, with full ordering. So currently, the bit is 1 before the callbacks ran. It becomes 1 before the callbacks ran, but dma_fence_default_wait() can only see that after it took the lock, and it can only do that after the lock is released after the callback ran. So behavior for this function is unchanged, unless I miss sth. Actually, this barrier makes things more robust, since things like if (dma_fence_is_signaled(f)) ts = dma_fence_timestamp(f) could misorder the accesses to the signaled-bit. Do you have another example of what would break? You see, the bit sometimes being locked and sometimes being lockless causes confusion. P. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 14:52 ` Philipp Stanner @ 2026-10-21 15:04 ` Christian König 0 siblings, 0 replies; 17+ messages in thread From: Christian König @ 2026-10-21 15:04 UTC (permalink / raw) To: phasta, Sumit Semwal, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: linux-media, dri-devel, linux-kernel, linux-arch On 10/21/26 16:52, Philipp Stanner wrote: > On Wed, 2026-10-21 at 15:47 +0200, Christian König wrote: >> On 10/21/26 11:55, Philipp Stanner wrote: >>> dma_fence_is_signaled() can return true *while* the fence is still >>> signaling. This is fragile and has been involved in at least two bugs: >>> >>> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race") >>> commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM") >>> >>> The signaled-bit is set under lock-protection but read without it. >>> Adding the lock has been rejected on the grounds of the lock-acquisition >>> shooting down cache lines of parties that merely want to check whether >>> the fence is already signaled. >>> >>> Make sure the bit can only become visible as 1 once the callbacks have >>> ran. To achieve this, read the bit with test_bit_acquire() and set it >>> with set_bit_release(). >> >> That has been suggested and rejected before. >> >> Background is that the signaling bit must be visible before the >> callbacks are run or things like dma_fence_default_wait() (as an >> example) just break. > > dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) > { > struct default_wait_cb cb; > unsigned long flags; > signed long ret = timeout ? timeout : 1; > > dma_fence_lock_irqsave(fence, flags); > > if (dma_fence_test_signaled_flag(fence)) > goto out; > > if (intr && signal_pending(current)) { > ret = -ERESTARTSYS; > goto out; > } > > if (!timeout) { > ret = 0; > goto out; > } > > cb.base.func = dma_fence_default_wait_cb; > cb.task = current; > list_add(&cb.base.node, &fence->cb_list); > > while (!dma_fence_test_signaled_flag(fence) && ret > 0) { > if (intr) > __set_current_state(TASK_INTERRUPTIBLE); > else > __set_current_state(TASK_UNINTERRUPTIBLE); > dma_fence_unlock_irqrestore(fence, flags); > > > That function evaluates the flag under lock-protection, with full > ordering. The bit is only ever set under lock-protection, with full > ordering. > > So currently, the bit is 1 before the callbacks ran. It becomes 1 > before the callbacks ran, but dma_fence_default_wait() can only see > that after it took the lock, and it can only do that after the lock is > released after the callback ran. > > So behavior for this function is unchanged, unless I miss sth. Yeah that argument came up before and no it is not correct. > Actually, this barrier makes things more robust, since things like > > if (dma_fence_is_signaled(f)) > ts = dma_fence_timestamp(f) > > could misorder the accesses to the signaled-bit. > > > Do you have another example of what would break? Please search the mailing list. We had the same patch like three or four times already. It was even applied one IIRC, but yes it immediately breaks things. Regards, Christian. > > > You see, the bit sometimes being locked and sometimes being lockless > causes confusion. > > > P. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner 2026-10-21 10:16 ` sashiko-bot 2026-10-21 13:47 ` Christian König @ 2026-10-26 1:34 ` kernel test robot 2026-10-26 7:48 ` kernel test robot 3 siblings, 0 replies; 17+ messages in thread From: kernel test robot @ 2026-10-26 1:34 UTC (permalink / raw) To: Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: oe-kbuild-all, linux-media, dri-devel, linux-kernel, linux-arch, Philipp Stanner Hi Philipp, kernel test robot noticed the following build errors: [auto build test ERROR on 1ecab8b0bad393750c60cc3819ce9c256595463c] url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/asm-generic-bitops-Add-set_bit_release/20261021-115512 base: 1ecab8b0bad393750c60cc3819ce9c256595463c patch link: https://lore.kernel.org/r/20261021095512.1345191-4-phasta%40kernel.org patch subject: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20261026/202610260958.IcV50YNq-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 16.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261026/202610260958.IcV50YNq-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202610260958.IcV50YNq-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/dma-buf/dma-fence.c: In function 'dma_fence_signal_timestamp_locked': >> drivers/dma-buf/dma-fence.c:393:9: error: implicit declaration of function 'set_bit_release'; did you mean 'seq_release'? [-Wimplicit-function-declaration] 393 | set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); | ^~~~~~~~~~~~~~~ | seq_release vim +393 drivers/dma-buf/dma-fence.c 337 338 /** 339 * dma_fence_signal_timestamp_locked - signal completion of a fence 340 * @fence: the fence to signal 341 * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain 342 * 343 * Signal completion for software callbacks on a fence, this will unblock 344 * dma_fence_wait() calls and run all the callbacks added with 345 * dma_fence_add_callback(). Can be called multiple times, but since a fence 346 * can only go from the unsignaled to the signaled state and not back, it will 347 * only be effective the first time. Set the timestamp provided as the fence 348 * signal timestamp. 349 * 350 * Unlike dma_fence_signal_timestamp(), this function must be called with 351 * &dma_fence.lock held. 352 */ 353 void dma_fence_signal_timestamp_locked(struct dma_fence *fence, 354 ktime_t timestamp) 355 { 356 const struct dma_fence_ops *ops; 357 struct dma_fence_cb *cur, *tmp; 358 struct list_head cb_list; 359 360 dma_fence_assert_held(fence); 361 362 if (unlikely(dma_fence_test_signaled_flag(fence))) 363 return; 364 365 trace_dma_fence_signaled(fence); 366 367 /* 368 * When neither a release nor a wait operation is specified set the ops 369 * pointer to NULL to allow the fence structure to become independent 370 * from who originally issued it. 371 */ 372 ops = rcu_dereference_protected(fence->ops, true); 373 if (!ops->release && !ops->wait) 374 RCU_INIT_POINTER(fence->ops, NULL); 375 376 /* Stash the cb_list before replacing it with the timestamp */ 377 list_replace(&fence->cb_list, &cb_list); 378 379 fence->timestamp = timestamp; 380 set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); 381 382 list_for_each_entry_safe(cur, tmp, &cb_list, node) { 383 INIT_LIST_HEAD(&cur->node); 384 cur->func(fence, cur); 385 } 386 387 /* 388 * This orders the signaled-state with the test_bit_acquire() in 389 * dma_fence_test_signaled_flag(). This ensures that functions like 390 * dma_fence_is_signaled() can only ever return true once signaling is 391 * completed (i.e., after the callbacks above ran). 392 */ > 393 set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); 394 } 395 EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); 396 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner ` (2 preceding siblings ...) 2026-10-26 1:34 ` kernel test robot @ 2026-10-26 7:48 ` kernel test robot 3 siblings, 0 replies; 17+ messages in thread From: kernel test robot @ 2026-10-26 7:48 UTC (permalink / raw) To: Philipp Stanner, Sumit Semwal, Christian König, Yury Norov, Rasmus Villemoes, Arnd Bergmann, Tvrtko Ursulin, Danilo Krummrich, Gary Guo, Paul E . McKenney Cc: llvm, oe-kbuild-all, linux-media, dri-devel, linux-kernel, linux-arch, Philipp Stanner Hi Philipp, kernel test robot noticed the following build errors: [auto build test ERROR on 1ecab8b0bad393750c60cc3819ce9c256595463c] url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/asm-generic-bitops-Add-set_bit_release/20261021-115512 base: 1ecab8b0bad393750c60cc3819ce9c256595463c patch link: https://lore.kernel.org/r/20261021095512.1345191-4-phasta%40kernel.org patch subject: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly config: sparc64-allmodconfig (https://download.01.org/0day-ci/archive/20261026/202610261518.t3m9Kwds-lkp@intel.com/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261026/202610261518.t3m9Kwds-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202610261518.t3m9Kwds-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/dma-buf/dma-fence.c:393:2: error: call to undeclared function 'set_bit_release'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 393 | set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); | ^ 1 error generated. vim +/set_bit_release +393 drivers/dma-buf/dma-fence.c 337 338 /** 339 * dma_fence_signal_timestamp_locked - signal completion of a fence 340 * @fence: the fence to signal 341 * @timestamp: fence signal timestamp in kernel's CLOCK_MONOTONIC time domain 342 * 343 * Signal completion for software callbacks on a fence, this will unblock 344 * dma_fence_wait() calls and run all the callbacks added with 345 * dma_fence_add_callback(). Can be called multiple times, but since a fence 346 * can only go from the unsignaled to the signaled state and not back, it will 347 * only be effective the first time. Set the timestamp provided as the fence 348 * signal timestamp. 349 * 350 * Unlike dma_fence_signal_timestamp(), this function must be called with 351 * &dma_fence.lock held. 352 */ 353 void dma_fence_signal_timestamp_locked(struct dma_fence *fence, 354 ktime_t timestamp) 355 { 356 const struct dma_fence_ops *ops; 357 struct dma_fence_cb *cur, *tmp; 358 struct list_head cb_list; 359 360 dma_fence_assert_held(fence); 361 362 if (unlikely(dma_fence_test_signaled_flag(fence))) 363 return; 364 365 trace_dma_fence_signaled(fence); 366 367 /* 368 * When neither a release nor a wait operation is specified set the ops 369 * pointer to NULL to allow the fence structure to become independent 370 * from who originally issued it. 371 */ 372 ops = rcu_dereference_protected(fence->ops, true); 373 if (!ops->release && !ops->wait) 374 RCU_INIT_POINTER(fence->ops, NULL); 375 376 /* Stash the cb_list before replacing it with the timestamp */ 377 list_replace(&fence->cb_list, &cb_list); 378 379 fence->timestamp = timestamp; 380 set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags); 381 382 list_for_each_entry_safe(cur, tmp, &cb_list, node) { 383 INIT_LIST_HEAD(&cur->node); 384 cur->func(fence, cur); 385 } 386 387 /* 388 * This orders the signaled-state with the test_bit_acquire() in 389 * dma_fence_test_signaled_flag(). This ensures that functions like 390 * dma_fence_is_signaled() can only ever return true once signaling is 391 * completed (i.e., after the callbacks above ran). 392 */ > 393 set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); 394 } 395 EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); 396 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2026-10-26 7:49 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-21 9:55 [PATCH 0/2] dma-fence: Order is-signaled-bit Philipp Stanner 2026-10-21 9:55 ` [PATCH 1/2] asm-generic/bitops: Add set_bit_release() Philipp Stanner 2026-10-21 10:06 ` sashiko-bot 2026-10-21 11:33 ` Arnd Bergmann 2026-10-21 11:54 ` Gary Guo 2026-10-21 12:15 ` Arnd Bergmann 2026-10-21 12:06 ` Gary Guo 2026-10-21 12:17 ` Philipp Stanner 2026-10-21 12:24 ` Gary Guo 2026-10-21 13:22 ` Philipp Stanner 2026-10-21 9:55 ` [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly Philipp Stanner 2026-10-21 10:16 ` sashiko-bot 2026-10-21 13:47 ` Christian König 2026-10-21 14:52 ` Philipp Stanner 2026-10-21 15:04 ` Christian König 2026-10-26 1:34 ` kernel test robot 2026-10-26 7:48 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.