Re: [PATCH v4 1/7] kcsan: Add atomic builtin stubs for 32-bit systems

On 9 Feb 2023, at 10:14 am, Rohan McLure [off-list ref] wrote:

quoted

On 8 Feb 2023, at 11:23 pm, Christophe Leroy [off-list ref] wrote:



Le 08/02/2023 à 04:21, Rohan McLure a écrit :

quoted

KCSAN instruments calls to atomic builtins, and will in turn call these
builtins itself. As such, architectures supporting KCSAN must have
compiler support for these atomic primitives.

Since 32-bit systems are unlikely to have 64-bit compiler builtins,
provide a stub for each missing builtin, and use BUG() to assert
unreachability.

In commit 725aea873261 ("xtensa: enable KCSAN"), xtensa implements these
locally, but does not advertise the fact with preprocessor macros. To
avoid production of duplicate symbols, do not build the stubs on xtensa.
A future patch will remove the xtensa implementation of these stubs.

Signed-off-by: Rohan McLure <redacted>
---
v4: New patch
---
kernel/kcsan/Makefile |  3 ++
kernel/kcsan/stubs.c  | 78 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 81 insertions(+)
create mode 100644 kernel/kcsan/stubs.c

I think it would be better to merge patch 1 and patch 2, that way we 
would keep the history and see that stubs.c almost comes from xtensa.

The summary would then be:

arch/xtensa/lib/Makefile                              |  1 -
kernel/kcsan/Makefile                                 |  2 +-
arch/xtensa/lib/kcsan-stubs.c => kernel/kcsan/stubs.c | 26 
+++++++++++++++++++++++++-
3 files changed, 26 insertions(+), 3 deletions(-)

quoted

diff --git a/kernel/kcsan/Makefile b/kernel/kcsan/Makefile
index 8cf70f068d92..5dfc5825aae9 100644
--- a/kernel/kcsan/Makefile
+++ b/kernel/kcsan/Makefile

@@ -12,6 +12,9 @@ CFLAGS_core.o := $(call cc-option,-fno-conserve-stack) \
 -fno-stack-protector -DDISABLE_BRANCH_PROFILING

obj-y := core.o debugfs.o report.o
+ifndef XTENSA
+ obj-y += stubs.o

obj-$(CONFIG_32BIT) += stubs.o

That would avoid the #if CONFIG_32BIT in stubs.c

Thanks. Yes happy to do this.

quoted

quoted

+endif

KCSAN_INSTRUMENT_BARRIERS_selftest.o := y
obj-$(CONFIG_KCSAN_SELFTEST) += selftest.o

diff --git a/kernel/kcsan/stubs.c b/kernel/kcsan/stubs.c
new file mode 100644
index 000000000000..ec5cd99be422
--- /dev/null
+++ b/kernel/kcsan/stubs.c

@@ -0,0 +1,78 @@
+// SPDX-License Identifier: GPL-2.0

Missing - between License and Identifier ?

quoted

+
+#include <linux/bug.h>
+#include <linux/types.h>
+
+#ifdef CONFIG_32BIT

Should be handled in Makefile

quoted

+
+#if !__has_builtin(__atomic_store_8)

Does any 32 bit ARCH support that ? Is that #if required ?

If yes, do we really need the #if for each and every function, can't we 
just check for one and assume that if we don't have __atomic_store_8 we 
don't have any of the functions ?

Turns out that testing with gcc provides 8-byte atomic builtins on x86
and arm on 32-bit. However I believe it should just suffice to check for
__atomic_store_8 or any other such builtin i.e. if an arch implements one it
likely implements them all from what I’ve seen.

In reality, __has_builtin only specifies that GCC is aware of the existance of
the builtin, but linking against libatomic may still be required. Let’s
remove this check, and have ppc32 and xtensa opt into compiling this stubs file.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108734

quoted

quoted

+void __atomic_store_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_load_8)
+u64 __atomic_load_8(const volatile void *p, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_exchange_8)
+u64 __atomic_exchange_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_compare_exchange_8)
+bool __atomic_compare_exchange_8(volatile void *p1, void *p2, u64 v, bool b, int i1, int i2)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_add_8)
+u64 __atomic_fetch_add_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_sub_8)
+u64 __atomic_fetch_sub_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_and_8)
+u64 __atomic_fetch_and_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_or_8)
+u64 __atomic_fetch_or_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_xor_8)
+u64 __atomic_fetch_xor_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#if !__has_builtin(__atomic_fetch_nand_8)
+u64 __atomic_fetch_nand_8(volatile void *p, u64 v, int i)
+{
+ BUG();
+}
+#endif
+
+#endif /* CONFIG_32BIT */