Re: [PATCH v2 2/3] x86/mce/amd: Reset MCA_SYND1/2 between bank scans

[OptoCloud]

Thanks for the review, here's v2.

Same issue as the MCA_SYND leak, for the two supplemental AMD
syndrome registers. mce_read_aux() writes
err->vendor.amd.synd1/synd2 only when MCI_STATUS_SYNDV is set, and
nothing clears them between bank iterations in either
machine_check_poll() or __mc_scan_banks(). A bank without SYNDV can
inherit synd1/synd2 from an earlier bank in the same scan.

Factor the per-bank field clearing (MISC, ADDR, SYND, and now
SYND1/2) into a shared mce_clear_hw_err_fields() helper, called from
both loops, to avoid repeating the same resets at each call site.

Found by code inspection; not reproduced on hardware.

Fixes: d4fca1358ea9 ("x86/MCE/AMD: Add support for new MCA_SYND{1,2} registers")
Signed-off-by: Eirik Bøe <git@xxxxxxxxxxxx>
---
Changes since v1:
- Dropped Cc: stable (Yazen)
- Added mce_clear_hw_err_fields() helper function to avoid
  repetition in machine_check_poll() and __mc_scan_banks() (Yazen)

 arch/x86/kernel/cpu/mce/core.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 16183fa4ddc7..6fa15e19988e 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -653,6 +653,22 @@ static struct notifier_block mce_default_nb = {
 	.priority	= MCE_PRIO_LOWEST,
 };
 
+/*
+ * mce_read_aux() only fills these fields when the matching status bit
+ * (MISCV/ADDRV/SYNDV) is set, so clear them before each bank to avoid
+ * inheriting a stale value from the previous bank in the scan.
+ */
+static noinstr void mce_clear_hw_err_fields(struct mce_hw_err *err)
+{
+	struct mce *m = &err->m;
+
+	m->misc = 0;
+	m->addr = 0;
+	m->synd = 0;
+	err->vendor.amd.synd1 = 0;
+	err->vendor.amd.synd2 = 0;
+}
+
 /*
  * Read ADDR and MISC registers.
  */
@@ -806,9 +822,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!mce_banks[i].ctl || !test_bit(i, *b))
 			continue;
 
-		m->misc = 0;
-		m->addr = 0;
-		m->synd = 0;
+		mce_clear_hw_err_fields(&err);
 		m->bank = i;
 
 		barrier();
@@ -1346,9 +1360,7 @@ __mc_scan_banks(struct mce_hw_err *err, struct pt_regs *regs,
 		if (!mce_banks[i].ctl)
 			continue;
 
-		m->misc = 0;
-		m->addr = 0;
-		m->synd = 0;
+		mce_clear_hw_err_fields(err);
 		m->bank = i;
 
 		m->status = mce_rdmsrq(mca_msr_reg(i, MCA_STATUS));
-- 
Cheers,
Eirik Bøe