x86/mce: Various fixes and cleanups for MCE handling

Since Linux 4.5 commit 8b38937b7ab5 ("x86/mce: Do not enter deferred

errors into the generic pool twice") the mce=nobootlog option has become mostly ineffective (after being only slightly ineffective before), as the code is taking actions on MCEs left over from boot when they have a usable address.

Move the check for MCP_DONTLOG a bit outward to make it effective again.

Also, since Linux 4.12 commit 011d82611172 ("RAS: Add a Corrected Errors Collector") the two branches of the remaining "if" the bottom of machine_check_poll() do the same. Unify them.

Signed-off-by Jan H. Schönherr <[email protected]>

---
 arch/x86/kernel/cpu/mce/core.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

arch/x86/kernel/cpu/mce/core.c +0 -2

index d5a8b99f7ba3..81ab25d5357a 100644
@@ -760,24 +760,17 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 log_it:
 		error_seen = true;
 
-		mce_read_aux(&m, i);
-

The function mce_severity_amd_smca() requires m->bank to be initialized for correct operation. Fix the one case, where mce_severity() is called without doing so.

Fixes 6bda529ec42e ("x86/mce: Grade uncorrected errors for SMCA-enabled systems")

Fixes d28af26faa0b ("x86/MCE: Initialize mce.bank in the case of a fatal error in mce_no_way_out()")

Signed-off-by Jan H. Schönherr <[email protected]>

---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kernel/cpu/mce/core.c +0 -0

index 81ab25d5357a..6afb9de251f2 100644

-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Monday, December 16, 2019 12:26 PM
To: Jan H. Schönherr <[email protected]>; Ghannam, Yazen <[email protected]>
Cc: Tony Luck <[email protected]>; [email protected]; Thomas Gleixner <[email protected]>; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; [email protected]
Subject: Re: [PATCH 3/6] x86/mce: Fix possibly incorrect severity calculation on AMD

Quoted 16 lines · Jan H. Schönherr

The function mce_severity_amd_smca() requires m->bank to be initialized for correct operation. Fix the one case, where mce_severity() is called without doing so.

Fixes 6bda529ec42e ("x86/mce: Grade uncorrected errors for SMCA-enabled systems")

Fixes d28af26faa0b ("x86/MCE: Initialize mce.bank in the case of a fatal error in mce_no_way_out()")

Signed-off-by Jan H. Schönherr <[email protected]>

---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kernel/cpu/mce/core.c +0 -0

index 81ab25d5357a..6afb9de251f2 100644

arch/x86/kernel/cpu/mce/core.c static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,

Quoted 10 lines

arch/x86/kernel/cpu/mce/core.c +1 -2

@@ -809,8 +809,8 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
 		if (quirk_no_way_out)
 			quirk_no_way_out(i, m, regs);
 
+		m->bank = i;
 		if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
-			m->bank = i;
 			mce_read_aux(m, i);
 			*msg = tmp;
 			return 1;
--

Good catch. This should go to Linus now.

Yazen, any objections?

The function mce_severity() is not required to update its msg argument. In fact, mce_severity_amd() doesn't. Fix some code paths, that assume that it is always updated.

In particular, this avoids returning uninitialized data in mce_no_way_out(), which may be used later for printing.

Signed-off-by Jan H. Schönherr <[email protected]>

---
 arch/x86/kernel/cpu/mce/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mce/core.c +3 -1

index 6afb9de251f2..b11a74e3fea9 100644
@@ -809,10 +809,12 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
 		if (quirk_no_way_out)
 			quirk_no_way_out(i, m, regs);
 
+		tmp = NULL;
 		m->bank = i;
 		if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
 			mce_read_aux(m, i);
-			*msg = tmp;
+			if (tmp)
+				*msg = tmp;
 			return 1;
 		}
 	}

Quoted 20 lines · Jan H. Schönherr

arch/x86/kernel/cpu/mce/core.c +3 -1

index 6afb9de251f2..b11a74e3fea9 100644
@@ -809,10 +809,12 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
 		if (quirk_no_way_out)
 			quirk_no_way_out(i, m, regs);
 
+		tmp = NULL;
 		m->bank = i;
 		if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
 			mce_read_aux(m, i);
-			*msg = tmp;
+			if (tmp)
+				*msg = tmp;
 			return 1;
 		}
 	}

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1321

Quoted 7 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1313,6 +1315,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		 * make sure we have the right "msg".
 		 */
 		if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) {
+			msg = "Unknown";
 			mce_severity(&m, cfg->tolerant, &msg, true);
 			mce_panic("Local fatal machine check!", &m, msg);
 		}

Can we get rid of all that silliness of dealing with a possibly uninitialized pointer in the callers and simply do at the beginning of mce_panic():

	if (!msg)
		msg = "Unknown";

?

Not quite. mce_panic() already handles NULL as a value for "exp" (not "msg").

We still need to pass NULL or a proper pointer. Not some uninitialized, potentially random data.

So, at the very least we need to initialize "tmp" in mce_no_way_out(), if you're looking for a minimal patch.

Hi.

Here is a series of smallish fixes/cleanups for the handling of MCEs.

Note, that except for patches 2 and 4, the patches are just compile tested.

Quoted 5 lines · Jan H. Schönherr

Hi.

Here is a series of smallish fixes/cleanups for the handling of MCEs.

Note, that except for patches 2 and 4, the patches are just compile tested.

I tried some UC injections with these patches applied. Stuff still works. But thanks to the UCN/Deferred patch I see:

	Memory failure: 0x5fe3284: already hardware poisoned

which is expected because on Intel we see an SRAR error in bank 1 and the machine check takes the page offline. Then we see a UCNA from another bank for the same address and try to take the same page offline again.

This is a return to the old behavior, but might surprise some folks.

One nit-pick - I think we should rename the srao_decode_notifier() function since it now handles both SRAO and UCNA. Not sure what a good name is ... something like "uc_decode_notifier()" since it takes action for multiple classes of uncorrected errors?

All the patches look OK to me (modulo the above rename suggestion). So consider this a "Reviewed-by" for all six.

This and names like "uncorrected_memory_error_notifier()" seem to imply a wider scope than the function actually has. That brings me to another
question: should the scope be wider?

Instead of filtering for usable addresses and specific severities, we could for example filter for (similar to cec_add_mce()):

mce_is_memory_error(m) &&
!mce_is_correctable(m) &&
mce_usable_address(m)

Would that make sense? Or does that violate anything, that I'm not aware of?

Quoted 10 lines · Jan H. Schönherr

This and names like "uncorrected_memory_error_notifier()" seem to imply a wider scope than the function actually has. That brings me to another
question: should the scope be wider?

Instead of filtering for usable addresses and specific severities, we could for example filter for (similar to cec_add_mce()):

mce_is_memory_error(m) &&
!mce_is_correctable(m) &&
mce_usable_address(m)

There's a comment above that code which says what that function wants:

	/* We eat only correctable DRAM errors with usable addresses. */

Quoted 1 line

Would that make sense? Or does that violate anything, that I'm not aware of?

So this should be a decision of the two CPU vendors basically answering the question: for which error severities you want the kernel to poison pages?

Basically a question for Tony and Yazen. CCed.

-----Original Message-----
From: Luck, Tony <[email protected]>
Sent: Monday, December 16, 2019 4:59 PM
To: Borislav Petkov <[email protected]>
Cc: Jan H. Schönherr <[email protected]>; Ghannam, Yazen <[email protected]>; [email protected]; Thomas Gleixner <[email protected]>; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; [email protected]
Subject: Re: [PATCH 0/6] x86/mce: Various fixes and cleanups for MCE handling

Quoted 18 lines · Borislav Petkov

Quoted 10 lines · Jan H. Schönherr

This and names like "uncorrected_memory_error_notifier()" seem to imply a wider scope than the function actually has. That brings me to another
question: should the scope be wider?

Instead of filtering for usable addresses and specific severities, we could for example filter for (similar to cec_add_mce()):

mce_is_memory_error(m) &&
!mce_is_correctable(m) &&
mce_usable_address(m)

There's a comment above that code which says what that function wants:

	/* We eat only correctable DRAM errors with usable addresses. */

Quoted 1 line

Would that make sense? Or does that violate anything, that I'm not aware of?

Quoted 5 lines

So this should be a decision of the two CPU vendors basically answering the question: for which error severities you want the kernel to poison pages?

Basically a question for Tony and Yazen. CCed.

Using Intel naming, I'd like the SRAO and UCNA severity uncorrected errors to take the soft offline path to stop using pages.

Quoted 2 lines

Using Intel naming, I'd like the SRAO and UCNA severity uncorrected errors to take the soft offline path to stop using pages.

For AMD, I'd like that no errors are handled in the SRAO notifier for now.

The DEFERRED severity could be used, but I'd like to do more testing beforehand.