linux-kernel - Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for conditional locking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250513085001.GC25891@noisy.programming.kicks-ass.net>
Date: Tue, 13 May 2025 10:50:01 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: dan.j.williams@...el.com, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>,
	Ingo Molnar <mingo@...nel.org>,
	"Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for
 conditional locking

On Tue, May 13, 2025 at 09:09:18AM +0200, Peter Zijlstra wrote:
> On Mon, May 12, 2025 at 01:39:19PM -0700, Linus Torvalds wrote:

> > If you *really* care about this, I think you should realize that the
> > non-error case is a valid kernel pointer.
> > 
> > And we could add some architecture-specific function to check for "is
> > this a valid non-NULL and non-error pointer" with a fallback to the
> > generic case.
> > 
> > Because then on a platform like x86, where kernel pointers are always
> > negative, but not *as* negative as the error pointers, you can check
> > for that with a single compare.
> > 
> > The logic is "add MAX_ERRNO, and if it's still negative, it wasn't
> > NULL and it wasn't ERR_PTR".
> > 
> > And while 'add' needs a destination register, 'sub' with the negated
> > value does not, and is called 'cmp'.
> > 
> > So I think you can do that with
> > 
> >         cmp $-MAX_ERRNO,...
> >         js ...
> > 
> > Sadly, I can't seem to get gcc to generate that code. But I didn't try
> > very hard.

Yeah, it seems to really like emitting add and lea.

Inline asm obviously works:

003e     c09e:  48 81 fb 01 f0 ff ff    cmp    $0xfffffffffffff001,%rbx
0045     c0a5:  79 11                   jns    c0b8 <foo+0x58>
0047     c0a7:  48 89 df                mov    %rbx,%rdi
004a     c0aa:  e8 00 00 00 00          call   c0af <foo+0x4f>  c0ab: R_X86_64_PLT32    raw_spin_rq_unlock-0x4
...
0058     c0b8:  5b                      pop    %rbx
0059     c0b9:  5d                      pop    %rbp
005a     c0ba:  e9 00 00 00 00          jmp    c0bf <foo+0x5f>  c0bb: R_X86_64_PLT32    __x86_return_thunk-0x4

Just not sure its worth it at this point.

---
diff --git a/arch/x86/include/asm/cleanup.h b/arch/x86/include/asm/cleanup.h
new file mode 100644
index 000000000000..7cef49be8570
--- /dev/null
+++ b/arch/x86/include/asm/cleanup.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_CLEANUP_H
+#define _ASM_X86_CLEANUP_H
+
+#define __GUARD_IS_ERR(_ptr) \
+	({ unsigned long _var = (__force unsigned long)(_ptr); \
+	   bool _s; \
+	   asm_inline volatile ("cmp %[val], %[var]" \
+				: "=@...s" (_s) \
+				: [val] "i" (-MAX_ERRNO), \
+				  [var] "r" (_var)); \
+	   unlikely(_s); })
+
+#endif /* _ASM_X86_CLEANUP_H */
+
diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index 8675b7b4ad23..a59a88c95277 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -12,6 +12,7 @@ mandatory-y += bug.h
 mandatory-y += cacheflush.h
 mandatory-y += cfi.h
 mandatory-y += checksum.h
+mandatory-y += cleanup.h
 mandatory-y += compat.h
 mandatory-y += current.h
 mandatory-y += delay.h
diff --git a/include/asm-generic/cleanup.h b/include/asm-generic/cleanup.h
new file mode 100644
index 000000000000..616ae558638e
--- /dev/null
+++ b/include/asm-generic/cleanup.h
@@ -0,0 +1,5 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_CLEANUP_H
+#define _ASM_GENERIC_CLEANUP_H
+
+#endif /* _ASM_GENERIC_CLEANUP_H */
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 18209e191973..00e5ef7aa314 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -6,6 +6,8 @@
 #include <linux/err.h>
 #include <linux/args.h>
 
+#include <asm/cleanup.h>
+
 /**
  * DOC: scope-based cleanup helpers
  *
@@ -312,9 +314,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
 static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
+#ifndef __GUARD_IS_ERR
 #define __GUARD_IS_ERR(_ptr) \
 	({ unsigned long _rc = (__force unsigned long)(_ptr); \
 	   unlikely((_rc-1) >= -(MAX_ERRNO+1)); })
+#endif
 
 #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \