On Tue, Dec 07, 2021 at 08:13:38AM -0800, Linus Torvalds wrote:

On Tue, Dec 7, 2021 at 5:28 AM Peter Zijlstra [off-list ref] wrote:

quoted

+#define refcount_dec_and_test refcount_dec_and_test
+static inline bool refcount_dec_and_test(refcount_t *r)
+{
+       asm_volatile_goto (LOCK_PREFIX "decl %[var]\n\t"
+                          "jz %l[cc_zero]\n\t"
+                          "jl %l[cc_error]"
+                          : : [var] "m" (r->refs.counter)
+                          : "memory"
+                          : cc_zero, cc_error);
+       return false;
+
+cc_zero:
+       return true;
+
+cc_error:
+       refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
+       return false;
+}

Please don't add broken arch-specific helpers that are useless for
anything else than the broken refcount_t use.

I take issue with the broken, but I concede the point.

Make it return three return values, call it atomic_dec_and_test_ref()
or something like that, and now people can use it without having to
use a refcount_t.

Nobody really wants two different error cases anyway. The fact that
refcount_warn_saturate() has different cases is only an annoyance.

How about we do something like the unsafe_ uaccess functions and do it
like so?

It's a bit gross, and there seems to be a problem with macro expansion
of __ofl, but it 'works'.

---

diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 5e754e895767..921ecfd5a40b 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h

@@ -263,6 +263,22 @@ static __always_inline int arch_atomic_fetch_xor(int i, atomic_t *v)
 }
 #define arch_atomic_fetch_xor arch_atomic_fetch_xor
 
+#define atomic_dec_and_test_ofl(_v, __ofl)				\
+({									\
+	__label__ __zero;						\
+	__label__ __out;						\
+	bool __ret = false;						\
+	asm_volatile_goto (LOCK_PREFIX "decl %[var]\n\t"		\
+			   "jz %l[__zero]\n\t"				\
+			   "jl %l[__ofl]"				\
+			   : : [var] "m" ((_v)->counter)		\
+			   : "memory"					\
+			   : __zero, __ofl);				\
+	goto __out;							\
+__zero:	__ret = true;							\
+__out:	__ret;								\
+})
+
 #ifdef CONFIG_X86_32
 # include <asm/atomic64_32.h>
 #else

diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index b8a6e387f8f9..f11ce70de2da 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h

@@ -330,7 +330,15 @@ static inline __must_check bool __refcount_dec_and_test(refcount_t *r, int *oldp
  */
 static inline __must_check bool refcount_dec_and_test(refcount_t *r)
 {
+#ifdef atomic_dec_and_test_ofl
+	return atomic_dec_and_test_ofl(&r->refs, __ofl);
+
+__ofl:
+	refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
+	return false;
+#else
 	return __refcount_dec_and_test(r, NULL);
+#endif
 }
 
 static inline void __refcount_dec(refcount_t *r, int *oldp)