Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] EDAC/mc: Fix memory alignment calculation formula
@ 2020-05-16 16:21 wata2ki
  2020-06-03 11:28 ` Borislav Petkov
  2020-06-17 17:58 ` Borislav Petkov
  0 siblings, 2 replies; 5+ messages in thread
From: wata2ki @ 2020-05-16 16:21 UTC (permalink / raw)
  To: linux-edac; +Cc: Naoto Yamaguchi

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula
  2020-05-16 16:21 [PATCH] EDAC/mc: Fix memory alignment calculation formula wata2ki
@ 2020-06-03 11:28 ` Borislav Petkov
  2020-06-03 13:07   ` Naoto YAMAGUCHI
       [not found]   ` <CABBJnRYZTsnOjNdd9x5ZS_Vb56yvEJWsLxEERYPj-m3HfAqx1Q@mail.gmail.com>
  2020-06-17 17:58 ` Borislav Petkov
  1 sibling, 2 replies; 5+ messages in thread
From: Borislav Petkov @ 2020-06-03 11:28 UTC (permalink / raw)
  To: wata2ki; +Cc: linux-edac, Naoto Yamaguchi

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula
  2020-06-03 11:28 ` Borislav Petkov
@ 2020-06-03 13:07   ` Naoto YAMAGUCHI
       [not found]   ` <CABBJnRYZTsnOjNdd9x5ZS_Vb56yvEJWsLxEERYPj-m3HfAqx1Q@mail.gmail.com>
  1 sibling, 0 replies; 5+ messages in thread
From: Naoto YAMAGUCHI @ 2020-06-03 13:07 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-edac, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula
       [not found]   ` <CABBJnRYZTsnOjNdd9x5ZS_Vb56yvEJWsLxEERYPj-m3HfAqx1Q@mail.gmail.com>
@ 2020-06-03 17:36     ` Borislav Petkov
  0 siblings, 0 replies; 5+ messages in thread
From: Borislav Petkov @ 2020-06-03 17:36 UTC (permalink / raw)
  To: Naoto YAMAGUCHI; +Cc: linux-edac, Naoto Yamaguchi

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] EDAC/mc: Fix memory alignment calculation formula
  2020-05-16 16:21 [PATCH] EDAC/mc: Fix memory alignment calculation formula wata2ki
  2020-06-03 11:28 ` Borislav Petkov
@ 2020-06-17 17:58 ` Borislav Petkov
  1 sibling, 0 replies; 5+ messages in thread
From: Borislav Petkov @ 2020-06-17 17:58 UTC (permalink / raw)
  To: wata2ki, Chris Metcalf, Mauro Carvalho Chehab; +Cc: linux-edac, Naoto Yamaguchi

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-16 16:21 [PATCH] EDAC/mc: Fix memory alignment calculation formula wata2ki
2020-06-03 11:28 ` Borislav Petkov
2020-06-03 13:07   ` Naoto YAMAGUCHI
     [not found]   ` <CABBJnRYZTsnOjNdd9x5ZS_Vb56yvEJWsLxEERYPj-m3HfAqx1Q@mail.gmail.com>
2020-06-03 17:36     ` Borislav Petkov
2020-06-17 17:58 ` Borislav Petkov

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git