EDAC/mc: Fix memory alignment calculation formula
PATCH 1/1 EDAC/mc: Fix memory alignment calculation formula
From: Naoto Yamaguchi <[email protected]>
During the development of the off-tree driver, we found a bug that causes alignment fault exception in mutex_lock.
Line of the code:ffffffc010536ce4: c85ffe62 ldaxr x2, [x19]
Register value:
x19: ffffff800e90f6c4
This problem was caused by the alignment error of pvt_info in struct mem_ctl_info. It is caused by a calculation formula error in edac_align_ptr.
Existing calculation formula is using variable p, but this variable is address of the pointer variable not memory offset. In this calculation formula should use *p.
--- drivers/edac/edac_mc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 drivers/edac/edac_mc.c
drivers/edac/edac_mc.c +1 -1
@@ -271,7 +271,7 @@ void *edac_align_ptr(void **p, unsigned int size, int n_elems)else return (char *)ptr; - r = (unsigned long )p % align; + r = (unsigned long )(*p) % align;if (r ==0 )return (char *)ptr;
Signature · 1 line
2.17.1
Borislav Petkov
maintainerQuoted 3 lines · On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
From: Naoto Yamaguchi <[email protected]>
During the development of the off-tree driver,
Wait, what?
Am I reading this correctly that you have an out-of-tree EDAC driver?
If so, why? Why not submit it upstream?
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteHi
Out of tree driver (edac_injection) is under developing now by Gabriele Paoloni. This driver will upstream future.
When I was porting this driver to aarch64 environment, I found this bug.
This bug is also common bug for other edac me drivers. My opinion, this bug should be fixed instead of waiting for the driver to be developed.
Because alignment miss may only cause performance degradation in case of Intel, but it cause CPU exceptions (Oops/kernel panic) in case of aarch64 and other risc like architecture.
This bug was supposed to be fixed in commit(a).
But this fix conflict with the commit(b) fix and consequently the bug was not fixed.
commit(a): https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/edac/edac_mc.c?h=linux-3.7.y&id=8447c4d15e357a458c9051ddc84aa6c8b9c27000
commit(b): https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/edac/edac_mc.c?h=linux-3.7.y&id=93e4fe64ece4eccf0ff4ac69bceb389290b8ab7c
Thanks
2020年6月3日(水) 20:28 Borislav Petkov <[email protected]>:
Quoted 19 lines · On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
Quoted 3 lines
From: Naoto Yamaguchi <[email protected]>
During the development of the off-tree driver,
Wait, what?
Am I reading this correctly that you have an out-of-tree EDAC driver?
If so, why? Why not submit it upstream?
Thx.
--
Regards/Gruss,
Boris.
A message that is not in the archive (sent off-list or not yet received): CABBJnRYZTsnOjNdd9x5ZS_Vb56yvEJWsLxEERYPj-m3HfAqx1Q@mail.gmail.com
Borislav Petkov
maintainerQuoted 2 lines · Naoto YAMAGUCHI
Out of tree driver (
edac_injection) is under developing now by Gabriele Paoloni. This driver will upstream future.
Ah ok. I thought someone is running an out-of-tree EDAC driver for no good reason.
Quoted 1 line
When I was porting this driver to aarch64 environment, I found this bug.
I'll have a look after the merge window, thanks.
Btw, please do not top-post.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerQuoted 32 lines · On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
From: Naoto Yamaguchi <[email protected]>
During the development of the off-tree driver, we found a bug that causes alignment fault exception in
mutex_lock.Line of the code:
ffffffc010536ce4:c85ffe62ldaxr x2, [x19]Register value:
x19:ffffff800e90f6c4This problem was caused by the alignment error of
pvt_infoin structmem_ctl_info. It is caused by a calculation formula error inedac_align_ptr.Existing calculation formula is using variable p, but this variable is address of the pointer variable not memory offset. In this calculation formula should use *p.
Signed-off-by Naoto Yamaguchi <[email protected]>--- drivers/edac/edac_mc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 drivers/edac/edac_mc.cdrivers/edac/edac_mc.c +0
-0
drivers/edac/edac_mc.c void *edac_align_ptr(void **p, unsigned int size, int n_elems) · line 274
Quoted 5 lines
drivers/edac/edac_mc.c +1
-1@@ -271,7 +271,7 @@ void *edac_align_ptr(void **p, unsigned int size, int n_elems)else return (char *)ptr; - r = (unsigned long )p % align; + r = (unsigned long )(*p) % align;
Looks about right to me.
Btw, you don't need the () around *p - that's evaluated right-to-left so the dereference happens first and *then* the typecast, i.e., what you want here.
In any case, this line comes from
8447c4d15e35 ("edac: Do alignment logic properly in edac_align_ptr()")
and I believe it was wrong to use 'p' as that function works with the memory offsets - not with the pointer to the pointer. It's a whole different story whether I think this whole thing makes sense and it is ugly...
Anyway, adding the gentlemen from that commit to Cc.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette