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 v6
- Test for __DISABLE_EXPORTS macro instead of __DISABLE_BUG_TABLE macro.

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

 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 ++++++++++++++++++
 6 files changed, 38 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.

See also, commit bbf2a330d92c ("x86: atomic64: The atomic64_t data type should be 8 bytes aligned on 32-bit too").

---
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_EXPORTS conditional and refactored as helper function. ]

---
Checkpatch.pl says...
ERROR: Missing Signed-off-by: line by nominal patch author 'Peter Ziljstra <[email protected]>'

---
Changed since v6:
- Implemented helper function earlier in patch series, as requested by Ard.
- Dropped __DISABLE_BUG_TABLE macro in favour of __DISABLE_EXPORTS, as requested by Peter.

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.

---
 include/linux/instrumented.h | 11 +++++++++++
 lib/Kconfig.debug            | 10 ++++++++++
 2 files changed, 21 insertions(+)

index 711a1f0d1a73..e34b6a557e0a 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>

@@ -55,6 +56,13 @@ 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_EXPORTS
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+#endif
+}
+
 /**
  * instrument_atomic_read - instrument atomic read access
  * @v: address of access

@@ -67,6 +75,7 @@ static __always_inline void instrument_atomic_read(const volatile void *v, size_
 {
 	kasan_check_read(v, size);
 	kcsan_check_atomic_read(v, size);
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -81,6 +90,7 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_write(v, size);
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -95,6 +105,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);
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

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

Hi Finn,

Alternatively, you can credit Peter using

just before the Link-header pointing to his suggestion.

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

I'll leave that up to Peter as he's both author and maintainer.

If there was to be another revision, perhaps I can add --

;-)

We have his signoff.

edited and added, thanks.

Feel free to add:

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 v6:
- Rebased.

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 | 8 +++++++-
 lib/Kconfig.debug            | 8 ++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

index e34b6a557e0a..a1b4cf81adc2 100644
@@ -59,7 +59,13 @@ static __always_inline void instrument_read_write(const volatile void *v, size_t
 static __always_inline void instrument_atomic_check_alignment(const volatile void *v, size_t size)
 {
 #ifndef __DISABLE_EXPORTS
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+	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
 }
 

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