edac_align_ptr() bug fix and refactoring

Fix address alignment and return value casting, modify data types and refactor the flow to be more clear and readable.

Eliav Farber (4):
EDAC: Fix calculation of returned address and next offset in
edac_align_ptr()
EDAC: Remove unnecessary cast to char* in edac_align_ptr() function
EDAC: Refactor edac_align_ptr() to use u8/u16/u32/u64 data types
EDAC: Refactor edac_align_ptr() flow

 drivers/edac/edac_mc.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

2.32.0

Do alignment logic properly and use 'ptr' for calculating the remainder of the alignment.

This became an issue because 'struct edac_mc_layer' has a size that is not zero modulo eight, and the next offset that was prepared for the private-data was unaligned, causing an alignment exception.

---
 drivers/edac/edac_mc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fd440b35d76e..61d72bd96754 100644
@@ -265,7 +265,7 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	else
 		return (char *)ptr;
 
-	r = (unsigned long)p % align;
+	r = (unsigned long)ptr % align;
 
 	if (r == 0)
 		return (char *)ptr;

2.32.0

How exactly did this "become an issue"?

I'm asking because I have been hearing about weird bugs with that pointer alignment contraption and have never managed to reproduce them myself or hear a proper explanation from people.

And that thing is an abomination, frankly, and I'd like to get rid of it but maybe some other time...

So, please explain more verbosely, a specific example or how I could reproduce it, would be even better.

Thx.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

One of the fields in our private-data structure is 'struct notifier_block' which has a next field of type 'struct notifier_block __rcu *'. The size of our private-data structure is greater than 8, and it comes after 'struct edac_mc_layer' which has a size that is not zero modulo eight, and also ends at an address that is not zero modulo eight.
Because of the bug in edac_align_ptr(), our private-data structure which should have been aligned to 8 wasn't (it was aligned to 4), so notifier_block was also not aligned to 8, and finally next wasn't aligned to 8.

Our al_mc_edac driver calls atomic_notifier_chain_register() on probe, to add the notifier_block to panic_notifier_list.
We probe the driver more than once, and each time we use the same value for the priority field in the notifier_block (so the newer notifier_block should come later in panic_notifier_list).
When the driver is probed for the second time, we get an unable to handle kernel paging request panic at rcu_assign_pointer() which is called from notifier_chain_register().
It happens when rcu_assign_pointer() tries to set the unaligned next pointer from the first probe, to point to the new notifier_block of the second probe.

Unable to handle kernel paging request at virtual address ffff8013e8037f4c

Mem abort info:
  ESR = 0x96000061
  Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000061
  CM = 0, WnR = 1
  swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) [ffff8013e8037f4c] pgd=00000013ffff8003, pud=00680013c0000711

  Internal error: Oops: 96000061 [#1] SMP
  Modules linked in:
  Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
  CPU: 10 PID: 1 Comm: swapper/0 Not tainted 4.19.191 #1016
  Hardware name: Annapurna Labs Alpine V3 EVP (DT)
  pstate: 20000085 (nzCv daIf -PAN -UAO)
  pc : atomic_notifier_chain_register+0x80/0xb8
  lr : atomic_notifier_chain_register+0x38/0xb8

sp : ffff0000097d3b10
x29: ffff0000097d3b10 x28: ffff000009108068
x27: ffff0000095ca000 x26: ffff8013ed0a8744
x25: ffff0000096e3000 x24: ffff000009199000
x23: ffff8013ed016810 x22: ffff8013ed016800
x21: 0000000000000000 x20: ffff8013ed0a8744
x19: ffff0000095ca6d8 x18: ffffffffffffffff
x17: 0000000000000000 x16: 0000000000000000
x15: ffff0000091996c8 x14: 2820564544203a63
x13: 6d5f6c612072656c x12: 6c6f72746e6f6320
x11: 636164655f636d5f x10: ffff000009199918
x9 : ffff000009173018 x8 : ffff00000878ec80
x7 : 676e69766947203a x6 : 00000000000002b1
x5 : 000000000000003f x4 : 0000000000000000
x3 : 000000007fffffff x2 : ffff8013e8037f4c
x1 : 0000000000000096 x0 : ffff0000091bacf8

Call trace:
 atomic_notifier_chain_register+0x80/0xb8
 al_mc_edac_probe+0x224/0x468
 platform_drv_probe+0x58/0xa8
really_probe+0x2cc/0x3b8
driver_probe_device+0x12c/0x148
__driver_attach+0x148/0x150
bus_for_each_dev+0x84/0xd8
driver_attach+0x30/0x40
bus_add_driver+0x174/0x2a8
driver_register+0x64/0x110
__platform_driver_register+0x54/0x60
al_mc_edac_driver_init+0x20/0x28
do_one_initcall+0x54/0x208
kernel_init_freeable+0x294/0x354
kernel_init+0x18/0x118
ret_from_fork+0x10/0x18
Code: 91002002 f9400400 b5ffff60 f9000680 (c89ffc54)
---[ end trace dba8c8c6291afa5b ]---
Kernel panic - not syncing: Fatal exception

SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x0,20006008
Memory Limit: none

---[ end Kernel panic - not syncing: Fatal exception ]---

Just to make sure I understand you correctly: you're talking about some internal version of al_mc_edac - not what's upstream?

Ok, I think I see what's going on. So this:

8447c4d15e35 ("edac: Do alignment logic properly in edac_align_ptr()")

changed that remainder computation thing and it was supposed to do what your patch does. Hell, even the commit message says so:

"The logic was checking the sizeof the structure being allocated to determine whether an alignment fixup was required. This isn't right; what we actually care about is the alignment of the actual pointer that's about to be returned."

So if we really do care about the alignment of the actual pointer we're about to return, then yes, we should check ptr - not p.

Lemme add that newly discovered info to your patch and queue it.

Thx.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Yes, I'm talking about an internal version that wasn't up-streamed yet.

--
Regards, Eliav

Amend commit 7391c6dcab30 ("drivers/edac: mod edac_align_ptr function") and change all return path to use void* instead of char* according to the new signature.

---
 drivers/edac/edac_mc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 61d72bd96754..8b9b86a7720a 100644
@@ -263,12 +263,12 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	else if (size > sizeof(char))
 		align = sizeof(short);
 	else
-		return (char *)ptr;
+		return ptr;
 
 	r = (unsigned long)ptr % align;
 
 	if (r == 0)
-		return (char *)ptr;
+		return ptr;
 
 	*p += align - r;
 

2.32.0

Prefer well defined size variables, that are same in size across all systems.

---
 drivers/edac/edac_mc.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

index 8b9b86a7720a..3367bf997b73 100644
@@ -250,18 +250,13 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	 * 'size'.  Adjust 'p' so that its alignment is at least as
 	 * stringent as what the compiler would provide for X and return
 	 * the aligned result.
-	 * Here we assume that the alignment of a "long long" is the most
-	 * stringent alignment that the compiler will ever provide by default.
-	 * As far as I know, this is a reasonable assumption.
 	 */
-	if (size > sizeof(long))
-		align = sizeof(long long);
-	else if (size > sizeof(int))
-		align = sizeof(long);
-	else if (size > sizeof(short))
-		align = sizeof(int);
-	else if (size > sizeof(char))
-		align = sizeof(short);
+	if (size > sizeof(u32))
+		align = sizeof(u64);
+	else if (size > sizeof(u16))
+		align = sizeof(u32);
+	else if (size > sizeof(u8))
+		align = sizeof(u16);
 	else
 		return ptr;
 

2.32.0

Modify flow to be more clear:
- Calculate required alignment based on size.
- Check if *p is aligned and fix if not.
- Set return ptr to to be *p.
- Increase *p by new size for the next call.

---
 drivers/edac/edac_mc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

index 3367bf997b73..a3ff5a019fc7 100644
@@ -241,9 +241,7 @@ EXPORT_SYMBOL_GPL(edac_mem_types);
 void *edac_align_ptr(void **p, unsigned size, int n_elems)
 {
 	unsigned align, r;
-	void *ptr = *p;
-
-	*p += size * n_elems;
+	void *ptr;
 
 	/*
 	 * 'p' can possibly be an unaligned item X such that sizeof(X) is

@@ -258,16 +256,22 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
 	else if (size > sizeof(u8))
 		align = sizeof(u16);
 	else
-		return ptr;
-
-	r = (unsigned long)ptr % align;
+		goto out;
 
-	if (r == 0)
-		return ptr;
+	/* Calculate alignment, and fix *p if not aligned. */
+	r = (unsigned long)*p % align;
+	if (r)
+		*p += align - r;
 
-	*p += align - r;
+out:
+	/*
+	 * Set return ptr to to be *p (after alignment if it was needed),
+	 * and increase *p by new size for the next call.
+	 */
+	ptr = *p;
+	*p += size * n_elems;
 
-	return (void *)(((unsigned long)ptr) + align - r);
+	return ptr;
 }
 
 static void _edac_mc_free(struct mem_ctl_info *mci)

2.32.0

Right, as I said earlier, piling more on this crap design is not the right thing to do. Looking at the call sites, what I think you should do instead is simply compute the whole allocation size by making sure the alignment of those substruct element pointers we're assigning to, is 8 or so, for simplicity.

You can store the offsets in those helper variables, like it is done now.

Then do the allocation and add the offsets to the pointer kzalloc has returned.

And then get rid of that edac_align_ptr() crap. This thing is silly beyond repair and passing in that **p offset back'n'forth is making stuff more complicated than it is - one can simply do that computation before calling kzalloc and doesn't need a "helper" which ain't helping.

I'd say.

Thx.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette