On Tue, Dec 07, 2021 at 08:13:38AM -0800, Linus Torvalds wrote:
> On Tue, Dec 7, 2021 at 5:28 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > +#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)
