From: Aaron Miller <aaronmiller@fb.com>
To: Borislav Petkov <bp@alien8.de>,
Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] EDAC: expose per-dimm error counts in sysfs
Date: Fri, 28 Oct 2016 18:13:08 +0000 [thread overview]
Message-ID: <7EB49132-0CB0-46AE-8F2C-1AF334DE4494@fb.com> (raw)
In-Reply-To: <20161028130246.xcdzjy4mdrthgzol@pd.tnic>
The EDAC drivers themselves are reporting the channel and slot counts and dimm config and also doing the decoding of MCEs to map them to a node/channel/slot. If one of those was reporting that an MCE came from a position it had reported earlier did not exist I would say that driver was buggy, since the MCE doesn’t contain those as raw values from h/w, they have to be determined by the driver using what it knows about the current memory configuration.
Adding some bound checks might not be a bad idea in this does happen, but you can cause an oops like this by injecting out of bounds without this patch – only added reads it there shouldn’t be a way to cause an out-of-bounds read with it.
On 10/28/16, 6:02 AM, "Borislav Petkov" <bp@alien8.de> wrote:
On Thu, Oct 27, 2016 at 02:33:32PM -0700, Aaron Miller wrote:
> The old 'csrowX' sysfs directories had per-csrow error counters, but the
> new 'dimmX' directories do not currently expose error counts.
>
> EDAC already keeps these counts, add them to sysfs so per-dimm counts
> are still available when CONFIG_EDAC_LEGACY_SYSFS=n
>
> Signed-off-by: Aaron Miller <aaronmiller@fb.com>
> ---
Hmm, so something's still broken in the adding of the error count,
especially that dancing around with layers.
Mauro, I think you should take a look. Here's what I did:
EDAC_DFS=/sys/kernel/debug/edac/mc0/
echo 24 > $EDAC_DFS/fake_inject_channel
echo 0 > $EDAC_DFS/fake_inject_slot
echo 3 > $EDAC_DFS/fake_inject_count
echo 1 > $EDAC_DFS/fake_inject
24 is the max channels the system reports during start:
[ 4864.030901] EDAC MC: Removed device 0 for sbridge_edac.c Sandy Bridge Socket#0: DEV 0000:3f:0e.0
[ 4865.867873] EDAC MC: Ver: 3.0.0
[ 4866.081102] edac_mc_alloc: errcount layer 0 size 8
[ 4866.086081] edac_mc_alloc: errcount layer 1 size 24
[ 4866.091133] edac_mc_alloc: allocating 64 error counters
[ 4866.096529] edac_mc_alloc: allocating 3320 bytes for mci data (24 dimms, 24 csrows/channels)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ 4866.105146] edac_mc_alloc: ce_per_layer[0]: ffff8928b7e918f8
[ 4866.110979] edac_mc_alloc: ce_per_layer[1]: ffff8928b7e91938
[ 4866.619521] slot 0 <7>[ 4866.621530] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm0
[ 4866.637405] slot 0 <7>[ 4866.639420] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm3
[ 4866.655282] slot 0 <7>[ 4866.657288] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm6
[ 4866.673168] slot 0 <7>[ 4866.675190] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm9
[ 4866.691114] EDAC MC0: Giving out device to module sbridge_edac.c controller Sandy Bridge Socket#0: DEV 0000:3f:0e.0 (INTERRUPT)
and when I do that injection, there's boom, see below.
It looks to me that indexing in edac_inc_ce_error() goes out of bounds:
mci->ce_per_layer[i][index] += count;
if (i < mci->n_layers - 1)
index *= mci->layers[i + 1].size;
and it shouldn't.
We need to be prepared to handle crap data coming from the hardware so
those error count adding functions should check array lengths and scream
if something's overflowing...
next prev parent reply other threads:[~2016-10-28 18:14 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-25 23:25 [PATCH] EDAC: expose per-dimm error counts in sysfs Aaron Miller
2016-10-27 18:07 ` Borislav Petkov
2016-10-27 18:18 ` Mauro Carvalho Chehab
2016-10-27 21:23 ` Aaron Miller
2016-10-28 9:55 ` Aaron Miller
2016-10-27 21:33 ` [PATCH v2] " Aaron Miller
2016-10-28 13:02 ` Borislav Petkov
2016-10-28 18:13 ` Aaron Miller [this message]
2016-11-02 10:54 ` Borislav Petkov
2016-11-03 22:01 ` [PATCH v3] " Aaron Miller
2016-11-04 11:21 ` Borislav Petkov
2017-01-19 8:56 ` Aaron Miller
2017-01-19 9:29 ` Borislav Petkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7EB49132-0CB0-46AE-8F2C-1AF334DE4494@fb.com \
--to=aaronmiller@fb.com \
--cc=bp@alien8.de \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).