[PATCH] EDAC/mc: Fix memory alignment calculation formula

[PATCH] EDAC/mc: Fix memory alignment calculation formula

[wata2ki]

From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>

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.

Signed-off-by: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>
---
 drivers/edac/edac_mc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/edac/edac_mc.c

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
old mode 100644
new mode 100755
index 75ede27bdf6a..70929f136dd7
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -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;
-- 
2.17.1

Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula

[Borislav Petkov]

On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
> From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>
> 
> 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.

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

Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula

[Naoto YAMAGUCHI]

Hi

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 <bp@alien8.de>:
>
> On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
> > From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>
> >
> > 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.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula

[Borislav Petkov]

On Wed, Jun 03, 2020 at 10:00:01PM +0900, Naoto YAMAGUCHI wrote:
> 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.

> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula

[Borislav Petkov]

On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote:
> From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>
> 
> 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.
> 
> Signed-off-by: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp>
> ---
>  drivers/edac/edac_mc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  mode change 100644 => 100755 drivers/edac/edac_mc.c
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> old mode 100644
> new mode 100755
> index 75ede27bdf6a..70929f136dd7
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -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.

-- 
Regards/Gruss,
    Boris.

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