Align atomic storage

This series adds the __aligned attribute to atomic_t and atomic64_t definitions in include/linux and include/asm-generic (respectively) to get natural alignment of both types on csky, m68k, microblaze, nios2, openrisc and sh.

This series also adds Kconfig options to enable a new run-time warning to help reveal misaligned atomic accesses on platforms which don't trap that.

The performance impact is expected to vary across platforms and workloads. The measurements I made on m68k show that some workloads run faster and others slower.

---

Changed since v5:
- Added acked-by and revewed-by tags.
- Added a new macro to inhibit emission of __bug_table section, for the benefit of pre-boot code like the EFI stub loader.

Changed since v4:
- Dropped parisc header file patch as it's been merged already.
- Submitted as PATCH instead of RFC.

Changed since v3:
- Rebased on v6.17.
- New patch to resolve header dependency issue on parisc.
- Dropped documentation patch.

Changed since v2:
- Specify natural alignment for atomic64_t.
- CONFIG_DEBUG_ATOMIC checks for natural alignment again.
- New patch to add weakened alignment check.
- New patch for explicit alignment in BPF header.

---

Finn Thain (3):
bpf: Explicitly align bpf_res_spin_lock
atomic: Specify alignment for atomic_t and atomic64_t
atomic: Add option for weaker alignment check

Peter Zijlstra (1):
atomic: Add alignment check to instrumented atomic operations

 arch/x86/boot/compressed/Makefile     |  1 +
 drivers/firmware/efi/libstub/Makefile |  1 +
 include/asm-generic/atomic64.h        |  2 +-
 include/asm-generic/rqspinlock.h      |  2 +-
 include/linux/instrumented.h          | 17 +++++++++++++++++
 include/linux/types.h                 |  2 +-
 kernel/bpf/rqspinlock.c               |  1 -
 lib/Kconfig.debug                     | 18 ++++++++++++++++++
 8 files changed, 40 insertions(+), 4 deletions(-)

2.49.1

Align bpf_res_spin_lock to avoid a BUILD_BUG_ON() when the alignment changes, as it will do on m68k when, in a subsequent patch, the minimum alignment of the atomic_t member of struct rqspinlock gets increased from 2 to 4. Drop the BUILD_BUG_ON() as it becomes redundant.

---

Changed since v5:
- Added tag from Arnd Bergmann.

---
 include/asm-generic/rqspinlock.h | 2 +-
 kernel/bpf/rqspinlock.c          | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

index 0f2dcbbfee2f..dd36ac96bf66 100644
@@ -28,7 +28,7 @@ struct rqspinlock {
  */
 struct bpf_res_spin_lock {
 	u32 val;
-};
+} __aligned(__alignof__(struct rqspinlock));
 
 struct qspinlock;
 #ifdef CONFIG_QUEUED_SPINLOCKS

index f7d0c8d4644e..8d892fb099ac 100644
@@ -694,7 +694,6 @@ __bpf_kfunc int bpf_res_spin_lock(struct bpf_res_spin_lock *lock)
 	int ret;
 
 	BUILD_BUG_ON(sizeof(rqspinlock_t) != sizeof(struct bpf_res_spin_lock));
-	BUILD_BUG_ON(__alignof__(rqspinlock_t) != __alignof__(struct bpf_res_spin_lock));
 
 	preempt_disable();
 	ret = res_spin_lock((rqspinlock_t *)lock);

2.49.1

Some recent commits incorrectly assumed 4-byte alignment of locks. That assumption fails on Linux/m68k (and, interestingly, would have failed on Linux/cris also). The jump label implementation makes a similar alignment assumption.

The expectation that atomic_t and atomic64_t variables will be naturally aligned seems reasonable, as indeed they are on 64-bit architectures. But atomic64_t isn't naturally aligned on csky, m68k, microblaze, nios2, openrisc and sh. Neither atomic_t nor atomic64_t are naturally aligned on m68k.

This patch brings a little uniformity by specifying natural alignment for atomic types. One benefit is that atomic64_t variables do not get split across a page boundary. The cost is that some structs grow which leads to cache misses and wasted memory.

---
Changed since v5:
- Added tags from Guo Ren and Arnd Bergmann.

Changed since v2:
- Specify natural alignment for atomic64_t.

Changed since v1:
- atomic64_t now gets an __aligned attribute too.
- The 'Fixes' tag has been dropped because Lance sent a different fix for commit e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker") that's suitable for -stable.

---
 include/asm-generic/atomic64.h | 2 +-
 include/linux/types.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 100d24b02e52..f22ccfc0df98 100644
@@ -10,7 +10,7 @@
 #include <linux/types.h>
 
 typedef struct {
-	s64 counter;
+	s64 __aligned(sizeof(s64)) counter;
 } atomic64_t;
 
 #define ATOMIC64_INIT(i)	{ (i) }

index d4437e9c452c..1760e1feeab9 100644
@@ -180,7 +180,7 @@ typedef phys_addr_t resource_size_t;
 typedef unsigned long irq_hw_number_t;
 
 typedef struct {
-	int counter;
+	int __aligned(sizeof(int)) counter;
 } atomic_t;
 
 #define ATOMIC_INIT(i) { (i) }

2.49.1

From: Peter Zijlstra <[email protected]>

Add a Kconfig option for debug builds which logs a warning when an instrumented atomic operation takes place that's misaligned. Some platforms don't trap for this.

[fthain: added __DISABLE_BUG_TABLE macro.]

---
Checkpatch.pl says...
ERROR: Missing Signed-off-by: line by nominal patch author 'Peter Ziljstra <[email protected]>'
---
Changed since v5:
- Add new __DISABLE_BUG_TABLE macro to prevent a build failure on those
architectures which use atomics in pre-boot code like the EFI stub loader:

x86_64-linux-gnu-ld: error: unplaced orphan section __bug_table' from arch/x86/boot/compressed/sev-handle-vc.o'

Changed since v2:
- Always check for natural alignment.

---
 arch/x86/boot/compressed/Makefile     |  1 +
 drivers/firmware/efi/libstub/Makefile |  1 +
 include/linux/instrumented.h          | 10 ++++++++++
 lib/Kconfig.debug                     | 10 ++++++++++
 4 files changed, 22 insertions(+)

index 68f9d7a1683b..122967c80e48 100644
@@ -42,6 +42,7 @@ KBUILD_CFLAGS += -Wno-microsoft-anon-tag
 endif
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS += -D__DISABLE_BUG_TABLE
 KBUILD_CFLAGS += -D__DISABLE_EXPORTS
 # Disable relocation relaxation in case the link is not PIE.
 KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no)

index 7d15a85d579f..ac3e7c64aedb 100644
@@ -42,6 +42,7 @@ KBUILD_CFLAGS			:= $(subst $(CC_FLAGS_FTRACE),,$(cflags-y)) \
 				   -ffreestanding \
 				   -fno-stack-protector \
 				   $(call cc-option,-fno-addrsig) \
+				   -D__DISABLE_BUG_TABLE \
 				   -D__DISABLE_EXPORTS
 
 #

index 711a1f0d1a73..bcd1113b55a1 100644
@@ -7,6 +7,7 @@
 #ifndef _LINUX_INSTRUMENTED_H
 #define _LINUX_INSTRUMENTED_H
 
+#include <linux/bug.h>
 #include <linux/compiler.h>
 #include <linux/kasan-checks.h>
 #include <linux/kcsan-checks.h>

@@ -67,6 +68,9 @@ static __always_inline void instrument_atomic_read(const volatile void *v, size_
 {
 	kasan_check_read(v, size);
 	kcsan_check_atomic_read(v, size);
+#ifndef __DISABLE_BUG_TABLE
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+#endif
 }
 
 /**

@@ -81,6 +85,9 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_write(v, size);
+#ifndef __DISABLE_BUG_TABLE
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+#endif
 }
 
 /**

@@ -95,6 +102,9 @@ static __always_inline void instrument_atomic_read_write(const volatile void *v,
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_read_write(v, size);
+#ifndef __DISABLE_BUG_TABLE
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+#endif
 }
 
 /**

index ba36939fda79..4b4d1445ef9c 100644
@@ -1359,6 +1359,16 @@ config DEBUG_PREEMPT
 	  depending on workload as it triggers debugging routines for each
 	  this_cpu operation. It should only be used for debugging purposes.
 
+config DEBUG_ATOMIC
+	bool "Debug atomic variables"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here then the kernel will add a runtime alignment check
+	  to atomic accesses. Useful for architectures that do not have trap on
+	  mis-aligned access.
+
+	  This option has potentially significant overhead.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT

2.49.1

Urgh, so why not simply use __DISABLE_EXPORTS, that's typically (ab)used for these things?

Also, unless __DISABLE_BUG_TABLE goes live inside asm/bug.h and kills all __bug_table emissions, its a misnomer.

Furthermore, that SEV thing is broken and needs to be fixed anyway, this isn't helping it much. noinstr code should not be using instrumented things to begin with.

OK, I'll change it back to __DISABLE_EXPORTS.

Yes, __DISABLE_BUG_TABLE is certainly a misnomer, since what it actually does is to elide code that would emit bug table entries. I would argue that this distinction is splitting hairs but it's moot.

The problem is not confined to x86. I needed something that would work for loongarch, arm, riscv etc.

Anyway, thanks for your review. I will make the necessary changes.

Add a new Kconfig symbol to make CONFIG_DEBUG_ATOMIC more useful on those architectures which do not align dynamic allocations to 8-byte boundaries. Without this, CONFIG_DEBUG_ATOMIC produces excessive WARN splats.

---
Changed since v5:
- Rebased.

Changed since v3:
- Dropped #include <linux/cache.h> to avoid a build failure on arm64.
- Rewrote Kconfig help text to better describe preferred alignment.
- Refactored to avoid line-wrapping and duplication.

---
 include/linux/instrumented.h | 25 ++++++++++++++++---------
 lib/Kconfig.debug            |  8 ++++++++
 2 files changed, 24 insertions(+), 9 deletions(-)

index bcd1113b55a1..e8983eb8a888 100644
@@ -56,6 +56,19 @@ static __always_inline void instrument_read_write(const volatile void *v, size_t
 	kcsan_check_read_write(v, size);
 }
 
+static __always_inline void instrument_atomic_check_alignment(const volatile void *v, size_t size)
+{
+#ifndef __DISABLE_BUG_TABLE
+	if (IS_ENABLED(CONFIG_DEBUG_ATOMIC)) {
+		unsigned int mask = size - 1;
+
+		if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_LARGEST_ALIGN))
+			mask &= sizeof(struct { long x; } __aligned_largest) - 1;
+		WARN_ON_ONCE((unsigned long)v & mask);
+	}
+#endif
+}
+
 /**
  * instrument_atomic_read - instrument atomic read access
  * @v: address of access

@@ -68,9 +81,7 @@ static __always_inline void instrument_atomic_read(const volatile void *v, size_
 {
 	kasan_check_read(v, size);
 	kcsan_check_atomic_read(v, size);
-#ifndef __DISABLE_BUG_TABLE
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
-#endif
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -85,9 +96,7 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_write(v, size);
-#ifndef __DISABLE_BUG_TABLE
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
-#endif
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -102,9 +111,7 @@ static __always_inline void instrument_atomic_read_write(const volatile void *v,
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_read_write(v, size);
-#ifndef __DISABLE_BUG_TABLE
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
-#endif
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

index 4b4d1445ef9c..977a2c9163a2 100644
@@ -1369,6 +1369,14 @@ config DEBUG_ATOMIC
 
 	  This option has potentially significant overhead.
 
+config DEBUG_ATOMIC_LARGEST_ALIGN
+	bool "Check alignment only up to __aligned_largest"
+	depends on DEBUG_ATOMIC
+	help
+	  If you say Y here then the check for natural alignment of
+	  atomic accesses will be constrained to the compiler's largest
+	  alignment for scalar types.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT

2.49.1