* [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
@ 2018-06-21 12:40 Borislav Petkov
2018-06-21 18:18 ` Luck, Tony
0 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2018-06-21 12:40 UTC (permalink / raw)
To: Tony Luck; +Cc: LKML
From: Borislav Petkov <bp@suse.de>
mce_no_way_out() does a quick check during #MC to see whether some of
the MCEs logged would require the kernel to panic immediately. And it
passes a struct mce where MCi_STATUS gets written.
However, after having saved a valid status value, the next iteration
of the loop which goes over the MCA banks on the CPU, overwrites the
valid status value because we're using struct mce as storage instead of
a temporary variable.
Which leads to MCE records with an empty status value:
mce: [Hardware Error]: CPU 0: Machine Check Exception: 6 Bank 0: 0000000000000000
mce: [Hardware Error]: RIP 10:<ffffffffbd42fbd7> {trigger_mce+0x7/0x10}
Change it to use a temporary variable and save to mce.status only when
valid. It will still overwrite the old status value if more than one
MCEs are logged, which warrants the second change in this patch:
Return immediately when severity is a panic one so that we can panic
immediately with the first fatal MCE logged.
Cc: <stable@vger.kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/cpu/mcheck/mce.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5c38d1f861f2..5c0a97b2bfce 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -772,12 +772,15 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
struct pt_regs *regs)
{
+ u64 mci_status;
int i, ret = 0;
char *tmp;
for (i = 0; i < mca_cfg.banks; i++) {
- m->status = mce_rdmsrl(msr_ops.status(i));
- if (m->status & MCI_STATUS_VAL) {
+ mci_status = mce_rdmsrl(msr_ops.status(i));
+ if (mci_status & MCI_STATUS_VAL) {
+ m->status = mci_status;
+
__set_bit(i, validp);
if (quirk_no_way_out)
quirk_no_way_out(i, m, regs);
@@ -785,7 +788,7 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
*msg = tmp;
- ret = 1;
+ return 1;
}
}
return ret;
--
2.17.0.582.gccdcbd54c
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
2018-06-21 12:40 [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out() Borislav Petkov
@ 2018-06-21 18:18 ` Luck, Tony
2018-06-21 19:17 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Luck, Tony @ 2018-06-21 18:18 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML
Counter proposal. We don't need the temp mci_status because we exit the
loop early. Nor the "ret" variable.
How does this look?
-Tony
---
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index e4cf6ff1c2e1..0e30abccfe54 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -772,23 +772,23 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
struct pt_regs *regs)
{
- int i, ret = 0;
+ int i;
char *tmp;
for (i = 0; i < mca_cfg.banks; i++) {
m->status = mce_rdmsrl(msr_ops.status(i));
- if (m->status & MCI_STATUS_VAL) {
- __set_bit(i, validp);
- if (quirk_no_way_out)
- quirk_no_way_out(i, m, regs);
- }
+ if (!(m->status & MCI_STATUS_VAL))
+ continue;
+ __set_bit(i, validp);
+ if (quirk_no_way_out)
+ quirk_no_way_out(i, m, regs);
if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
*msg = tmp;
- ret = 1;
+ return 1;
}
}
- return ret;
+ return 0;
}
/*
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
2018-06-21 18:18 ` Luck, Tony
@ 2018-06-21 19:17 ` Borislav Petkov
2018-06-21 20:00 ` Luck, Tony
0 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2018-06-21 19:17 UTC (permalink / raw)
To: Luck, Tony; +Cc: LKML
On Thu, Jun 21, 2018 at 11:18:09AM -0700, Luck, Tony wrote:
> Counter proposal. We don't need the temp mci_status because we exit the
> loop early. Nor the "ret" variable.
>
>
> How does this look?
Yap, better. I'll test it later or tomorrow:
---
From: Borislav Petkov <bp@suse.de>
Date: Thu, 21 Jun 2018 14:18:47 +0200
Subject: [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
mce_no_way_out() does a quick check during #MC to see whether some of
the MCEs logged would require the kernel to panic immediately. And it
passes a struct mce where MCi_STATUS gets written.
However, after having saved a valid status value, the next iteration
of the loop which goes over the MCA banks on the CPU, overwrites the
valid status value because we're using struct mce as storage instead of
a temporary variable.
Which leads to MCE records with an empty status value:
mce: [Hardware Error]: CPU 0: Machine Check Exception: 6 Bank 0: 0000000000000000
mce: [Hardware Error]: RIP 10:<ffffffffbd42fbd7> {trigger_mce+0x7/0x10}
In order to prevent the loss of the status register value, return
immediately when severity is a panic one so that we can panic
immediately with the first fatal MCE logged. This is also the intention
of this function and not to noodle over the banks while a fatal MCE is
already logged.
Suggested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: <stable@vger.kernel.org>
---
arch/x86/kernel/cpu/mcheck/mce.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5c38d1f861f2..e75418096ec6 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -772,23 +772,24 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
struct pt_regs *regs)
{
- int i, ret = 0;
char *tmp;
+ int i;
for (i = 0; i < mca_cfg.banks; i++) {
m->status = mce_rdmsrl(msr_ops.status(i));
- if (m->status & MCI_STATUS_VAL) {
- __set_bit(i, validp);
- if (quirk_no_way_out)
- quirk_no_way_out(i, m, regs);
- }
+ if (!(m->status & MCI_STATUS_VAL))
+ continue;
+
+ __set_bit(i, validp);
+ if (quirk_no_way_out)
+ quirk_no_way_out(i, m, regs);
if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
*msg = tmp;
- ret = 1;
+ return 1;
}
}
- return ret;
+ return 0;
}
/*
--
2.17.0.582.gccdcbd54c
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
2018-06-21 19:17 ` Borislav Petkov
@ 2018-06-21 20:00 ` Luck, Tony
2018-06-21 20:10 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Luck, Tony @ 2018-06-21 20:00 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML
On Thu, Jun 21, 2018 at 09:17:11PM +0200, Borislav Petkov wrote:
> On Thu, Jun 21, 2018 at 11:18:09AM -0700, Luck, Tony wrote:
> > Counter proposal. We don't need the temp mci_status because we exit the
> > loop early. Nor the "ret" variable.
> >
> >
> > How does this look?
>
> Yap, better. I'll test it later or tomorrow:
>
Or even better. Just adds the "mce_read_aux()" to check ADDRV and MISCV
and populate more of the struct mce.
-Tony
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index e4cf6ff1c2e1..5e64e973c02b 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -772,23 +772,24 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
struct pt_regs *regs)
{
- int i, ret = 0;
+ int i;
char *tmp;
for (i = 0; i < mca_cfg.banks; i++) {
m->status = mce_rdmsrl(msr_ops.status(i));
- if (m->status & MCI_STATUS_VAL) {
- __set_bit(i, validp);
- if (quirk_no_way_out)
- quirk_no_way_out(i, m, regs);
- }
+ if (!(m->status & MCI_STATUS_VAL))
+ continue;
+ __set_bit(i, validp);
+ if (quirk_no_way_out)
+ quirk_no_way_out(i, m, regs);
if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
+ mce_read_aux(m, i);
*msg = tmp;
- ret = 1;
+ return 1;
}
}
- return ret;
+ return 0;
}
/*
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
2018-06-21 20:00 ` Luck, Tony
@ 2018-06-21 20:10 ` Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2018-06-21 20:10 UTC (permalink / raw)
To: Luck, Tony; +Cc: LKML
On Thu, Jun 21, 2018 at 01:00:03PM -0700, Luck, Tony wrote:
> On Thu, Jun 21, 2018 at 09:17:11PM +0200, Borislav Petkov wrote:
> > On Thu, Jun 21, 2018 at 11:18:09AM -0700, Luck, Tony wrote:
> > > Counter proposal. We don't need the temp mci_status because we exit the
> > > loop early. Nor the "ret" variable.
> > >
> > >
> > > How does this look?
> >
> > Yap, better. I'll test it later or tomorrow:
> >
>
> Or even better. Just adds the "mce_read_aux()" to check ADDRV and MISCV
> and populate more of the struct mce.
I thought about it but decided to leave it for another day. But ok, it
is a one more line and I guess it is ok to have it in the same patch,
I'll add it.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out()
@ 2018-06-21 12:40 Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2018-06-21 12:40 UTC (permalink / raw)
To: Tony Luck; +Cc: LKML
From: Borislav Petkov <bp@suse.de>
mce_no_way_out() does a quick check during #MC to see whether some of
the MCEs logged would require the kernel to panic immediately. And it
passes a struct mce where MCi_STATUS gets written.
However, after having saved a valid status value, the next iteration
of the loop which goes over the MCA banks on the CPU, overwrites the
valid status value because we're using struct mce as storage instead of
a temporary variable.
Which leads to MCE records with an empty status value:
mce: [Hardware Error]: CPU 0: Machine Check Exception: 6 Bank 0: 0000000000000000
mce: [Hardware Error]: RIP 10:<ffffffffbd42fbd7> {trigger_mce+0x7/0x10}
Change it to use a temporary variable and save to mce.status only when
valid. It will still overwrite the old status value if more than one
MCEs are logged, which warrants the second change in this patch:
Return immediately when severity is a panic one so that we can panic
immediately with the first fatal MCE logged.
Cc: <stable@vger.kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/cpu/mcheck/mce.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5c38d1f861f2..5c0a97b2bfce 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -772,12 +772,15 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
struct pt_regs *regs)
{
+ u64 mci_status;
int i, ret = 0;
char *tmp;
for (i = 0; i < mca_cfg.banks; i++) {
- m->status = mce_rdmsrl(msr_ops.status(i));
- if (m->status & MCI_STATUS_VAL) {
+ mci_status = mce_rdmsrl(msr_ops.status(i));
+ if (mci_status & MCI_STATUS_VAL) {
+ m->status = mci_status;
+
__set_bit(i, validp);
if (quirk_no_way_out)
quirk_no_way_out(i, m, regs);
@@ -785,7 +788,7 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
*msg = tmp;
- ret = 1;
+ return 1;
}
}
return ret;
--
2.17.0.582.gccdcbd54c
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-21 20:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-21 12:40 [PATCH] x86/mce: Do not overwrite MCi_STATUS in mce_no_way_out() Borislav Petkov
2018-06-21 18:18 ` Luck, Tony
2018-06-21 19:17 ` Borislav Petkov
2018-06-21 20:00 ` Luck, Tony
2018-06-21 20:10 ` Borislav Petkov
-- strict thread matches above, loose matches on Subject: below --
2018-06-21 12:40 Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®