[PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Vishal Verma]

The mce handler for 'nfit' devices is called for memory errors on a
Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
This list is used by the various NVDIMM drivers to avoid consuming known
poison locations during IO.

The mce handler gets called for both corrected and uncorrectable errors.
Until now, both kinds of errors have been added to the badblocks list.
However, corrected memory errors indicate that the problem has already
been fixed by hardware, and the resulting interrupt is merely a
notification to Linux. As far as future accesses to that location are
concerned, it is perfectly fine to use, and thus doesn't need to be
included in the above badblocks list.

Add a check in the nfit mce handler to filter out corrected mce events,
and only process uncorrectable errors.

Reported-by: Omar Avelar <omar.avelar@intel.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

v3: Unchanged from v2

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3a17107594c8..3111b3cee2ee 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
+bool mce_is_correctable(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 953b3ce92dcc..27015948bc41 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_is_memory_error);
 
-static bool mce_is_correctable(struct mce *m)
+bool mce_is_correctable(struct mce *m)
 {
 	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
 		return false;
@@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(mce_is_correctable);
 
 static bool cec_add_mce(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index e9626bf6ca29..7a51707f87e9 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_spa *nfit_spa;
 
-	/* We only care about memory errors */
-	if (!mce_is_memory_error(mce))
+	/* We only care about uncorrectable memory errors */
+	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
 	/*
-- 
2.17.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Vishal Verma]

The mce handler for 'nfit' devices is called for memory errors on a
Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
This list is used by the various NVDIMM drivers to avoid consuming known
poison locations during IO.

The mce handler gets called for both corrected and uncorrectable errors.
Until now, both kinds of errors have been added to the badblocks list.
However, corrected memory errors indicate that the problem has already
been fixed by hardware, and the resulting interrupt is merely a
notification to Linux. As far as future accesses to that location are
concerned, it is perfectly fine to use, and thus doesn't need to be
included in the above badblocks list.

Add a check in the nfit mce handler to filter out corrected mce events,
and only process uncorrectable errors.

Reported-by: Omar Avelar <omar.avelar@intel.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

v3: Unchanged from v2

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3a17107594c8..3111b3cee2ee 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
+bool mce_is_correctable(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 953b3ce92dcc..27015948bc41 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_is_memory_error);
 
-static bool mce_is_correctable(struct mce *m)
+bool mce_is_correctable(struct mce *m)
 {
 	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
 		return false;
@@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(mce_is_correctable);
 
 static bool cec_add_mce(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index e9626bf6ca29..7a51707f87e9 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_spa *nfit_spa;
 
-	/* We only care about memory errors */
-	if (!mce_is_memory_error(mce))
+	/* We only care about uncorrectable memory errors */
+	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
 	/*

[PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Vishal Verma]

The mce handler for 'nfit' devices is called for memory errors on a
Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
This list is used by the various NVDIMM drivers to avoid consuming known
poison locations during IO.

The mce handler gets called for both corrected and uncorrectable errors.
Until now, both kinds of errors have been added to the badblocks list.
However, corrected memory errors indicate that the problem has already
been fixed by hardware, and the resulting interrupt is merely a
notification to Linux. As far as future accesses to that location are
concerned, it is perfectly fine to use, and thus doesn't need to be
included in the above badblocks list.

Add a check in the nfit mce handler to filter out corrected mce events,
and only process uncorrectable errors.

Reported-by: Omar Avelar <omar.avelar@intel.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

v3: Unchanged from v2

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3a17107594c8..3111b3cee2ee 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
+bool mce_is_correctable(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 953b3ce92dcc..27015948bc41 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_is_memory_error);
 
-static bool mce_is_correctable(struct mce *m)
+bool mce_is_correctable(struct mce *m)
 {
 	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
 		return false;
@@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(mce_is_correctable);
 
 static bool cec_add_mce(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index e9626bf6ca29..7a51707f87e9 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_spa *nfit_spa;
 
-	/* We only care about memory errors */
-	if (!mce_is_memory_error(mce))
+	/* We only care about uncorrectable memory errors */
+	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
 	/*
-- 
2.17.1

[PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Vishal Verma]

The NFIT machine check handler uses the physical address from the 'mce'
structure, and compares it against information in the ACPI NFIT table to
determine whether that location lies on an NVDIMM. The mce->addr field
however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Reported-by: Robert Elliott <elliott@hpe.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

v3: Add this patch to address the address validation (Robert)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3111b3cee2ee..eb786f90f2d3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -217,6 +217,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 27015948bc41..cdbedeb3f3db 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d943ed50f0b4 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the mce is valid */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see
-- 
2.17.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Vishal Verma]

The NFIT machine check handler uses the physical address from the 'mce'
structure, and compares it against information in the ACPI NFIT table to
determine whether that location lies on an NVDIMM. The mce->addr field
however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Reported-by: Robert Elliott <elliott@hpe.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

v3: Add this patch to address the address validation (Robert)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3111b3cee2ee..eb786f90f2d3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -217,6 +217,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 27015948bc41..cdbedeb3f3db 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d943ed50f0b4 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the mce is valid */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see

[PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Vishal Verma]

The NFIT machine check handler uses the physical address from the 'mce'
structure, and compares it against information in the ACPI NFIT table to
determine whether that location lies on an NVDIMM. The mce->addr field
however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Reported-by: Robert Elliott <elliott@hpe.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

v3: Add this patch to address the address validation (Robert)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3111b3cee2ee..eb786f90f2d3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -217,6 +217,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 27015948bc41..cdbedeb3f3db 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d943ed50f0b4 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the mce is valid */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see
-- 
2.17.1

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> The NFIT machine check handler uses the physical address from the 'mce'
> structure, and compares it against information in the ACPI NFIT table to
> determine whether that location lies on an NVDIMM. The mce->addr field
> however may not always be valid, and this is indicated by the
> MCI_STATUS_ADDRV bit in the status field.
> 
> Export mce_usable_address() which already performs validation for the
> address, and use it in the NFIT handler.
> 
> Reported-by: Robert Elliott <elliott@hpe.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++++
>  3 files changed, 7 insertions(+), 1 deletion(-)

Is there any particular reason why is this a separate patch and not part
of the first one?

Also, do s/mce/MCE/g.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> The NFIT machine check handler uses the physical address from the 'mce'
> structure, and compares it against information in the ACPI NFIT table to
> determine whether that location lies on an NVDIMM. The mce->addr field
> however may not always be valid, and this is indicated by the
> MCI_STATUS_ADDRV bit in the status field.
> 
> Export mce_usable_address() which already performs validation for the
> address, and use it in the NFIT handler.
> 
> Reported-by: Robert Elliott <elliott@hpe.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++++
>  3 files changed, 7 insertions(+), 1 deletion(-)

Is there any particular reason why is this a separate patch and not part
of the first one?

Also, do s/mce/MCE/g.

Thx.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> The NFIT machine check handler uses the physical address from the 'mce'
> structure, and compares it against information in the ACPI NFIT table to
> determine whether that location lies on an NVDIMM. The mce->addr field
> however may not always be valid, and this is indicated by the
> MCI_STATUS_ADDRV bit in the status field.
> 
> Export mce_usable_address() which already performs validation for the
> address, and use it in the NFIT handler.
> 
> Reported-by: Robert Elliott <elliott@hpe.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++++
>  3 files changed, 7 insertions(+), 1 deletion(-)

Is there any particular reason why is this a separate patch and not part
of the first one?

Also, do s/mce/MCE/g.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 6:51 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> > The NFIT machine check handler uses the physical address from the 'mce'
> > structure, and compares it against information in the ACPI NFIT table to
> > determine whether that location lies on an NVDIMM. The mce->addr field
> > however may not always be valid, and this is indicated by the
> > MCI_STATUS_ADDRV bit in the status field.
> >
> > Export mce_usable_address() which already performs validation for the
> > address, and use it in the NFIT handler.
> >
> > Reported-by: Robert Elliott <elliott@hpe.com>
> > Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> > Cc: stable@vger.kernel.org
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  arch/x86/include/asm/mce.h       | 1 +
> >  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
> >  drivers/acpi/nfit/mce.c          | 4 ++++
> >  3 files changed, 7 insertions(+), 1 deletion(-)
>
> Is there any particular reason why is this a separate patch and not part
> of the first one?

I recommended the split so the fixes can be tracked and / or reverted
independently if they cause problems.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 6:51 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> > The NFIT machine check handler uses the physical address from the 'mce'
> > structure, and compares it against information in the ACPI NFIT table to
> > determine whether that location lies on an NVDIMM. The mce->addr field
> > however may not always be valid, and this is indicated by the
> > MCI_STATUS_ADDRV bit in the status field.
> >
> > Export mce_usable_address() which already performs validation for the
> > address, and use it in the NFIT handler.
> >
> > Reported-by: Robert Elliott <elliott@hpe.com>
> > Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> > Cc: stable@vger.kernel.org
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  arch/x86/include/asm/mce.h       | 1 +
> >  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
> >  drivers/acpi/nfit/mce.c          | 4 ++++
> >  3 files changed, 7 insertions(+), 1 deletion(-)
>
> Is there any particular reason why is this a separate patch and not part
> of the first one?

I recommended the split so the fixes can be tracked and / or reverted
independently if they cause problems.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 6:51 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> > The NFIT machine check handler uses the physical address from the 'mce'
> > structure, and compares it against information in the ACPI NFIT table to
> > determine whether that location lies on an NVDIMM. The mce->addr field
> > however may not always be valid, and this is indicated by the
> > MCI_STATUS_ADDRV bit in the status field.
> >
> > Export mce_usable_address() which already performs validation for the
> > address, and use it in the NFIT handler.
> >
> > Reported-by: Robert Elliott <elliott@hpe.com>
> > Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> > Cc: stable@vger.kernel.org
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  arch/x86/include/asm/mce.h       | 1 +
> >  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
> >  drivers/acpi/nfit/mce.c          | 4 ++++
> >  3 files changed, 7 insertions(+), 1 deletion(-)
>
> Is there any particular reason why is this a separate patch and not part
> of the first one?

I recommended the split so the fixes can be tracked and / or reverted
independently if they cause problems.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> I recommended the split so the fixes can be tracked and / or reverted
> independently if they cause problems.

Do you have anything concrete in mind that might go wrong or just a
general cautiousness?

Or do you think the hw might not spit what mce_usable_address() is
checking for, for NVDIMM MCEs, so that you'd like to be able to revert
that second patch?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> I recommended the split so the fixes can be tracked and / or reverted
> independently if they cause problems.

Do you have anything concrete in mind that might go wrong or just a
general cautiousness?

Or do you think the hw might not spit what mce_usable_address() is
checking for, for NVDIMM MCEs, so that you'd like to be able to revert
that second patch?

Thx.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> I recommended the split so the fixes can be tracked and / or reverted
> independently if they cause problems.

Do you have anything concrete in mind that might go wrong or just a
general cautiousness?

Or do you think the hw might not spit what mce_usable_address() is
checking for, for NVDIMM MCEs, so that you'd like to be able to revert
that second patch?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 9:53 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> > I recommended the split so the fixes can be tracked and / or reverted
> > independently if they cause problems.
>
> Do you have anything concrete in mind that might go wrong or just a
> general cautiousness?

Just general cautiousness, I'm not opposed to squashing them.

> Or do you think the hw might not spit what mce_usable_address() is
> checking for, for NVDIMM MCEs, so that you'd like to be able to revert
> that second patch?

mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM MCEs.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 9:53 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> > I recommended the split so the fixes can be tracked and / or reverted
> > independently if they cause problems.
>
> Do you have anything concrete in mind that might go wrong or just a
> general cautiousness?

Just general cautiousness, I'm not opposed to squashing them.

> Or do you think the hw might not spit what mce_usable_address() is
> checking for, for NVDIMM MCEs, so that you'd like to be able to revert
> that second patch?

mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM MCEs.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 9:53 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> > I recommended the split so the fixes can be tracked and / or reverted
> > independently if they cause problems.
>
> Do you have anything concrete in mind that might go wrong or just a
> general cautiousness?

Just general cautiousness, I'm not opposed to squashing them.

> Or do you think the hw might not spit what mce_usable_address() is
> checking for, for NVDIMM MCEs, so that you'd like to be able to revert
> that second patch?

mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM MCEs.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 10:02:46AM -0800, Dan Williams wrote:
> Just general cautiousness, I'm not opposed to squashing them.

Nah, I can keep them separate - I was just wondering why.

> mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM
> MCEs.

Ok.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 10:02:46AM -0800, Dan Williams wrote:
> Just general cautiousness, I'm not opposed to squashing them.

Nah, I can keep them separate - I was just wondering why.

> mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM
> MCEs.

Ok.

Thx.

Re: [PATCH v3 2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 10:02:46AM -0800, Dan Williams wrote:
> Just general cautiousness, I'm not opposed to squashing them.

Nah, I can keep them separate - I was just wondering why.

> mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM
> MCEs.

Ok.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip:x86/urgent] acpi/nfit, x86/mce: Handle only uncorrectable machine checks

[tip-bot for Vishal Verma]

Commit-ID:  5d96c9342c23ee1d084802dcf064caa67ecaa45b
Gitweb:     https://git.kernel.org/tip/5d96c9342c23ee1d084802dcf064caa67ecaa45b
Author:     Vishal Verma <vishal.l.verma@intel.com>
AuthorDate: Thu, 25 Oct 2018 18:37:28 -0600
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 6 Nov 2018 19:13:10 +0100

acpi/nfit, x86/mce: Handle only uncorrectable machine checks

The MCE handler for nfit devices is called for memory errors on a
Non-Volatile DIMM and adds the error location to a 'badblocks' list.
This list is used by the various NVDIMM drivers to avoid consuming known
poison locations during IO.

The MCE handler gets called for both corrected and uncorrectable errors.
Until now, both kinds of errors have been added to the badblocks list.
However, corrected memory errors indicate that the problem has already
been fixed by hardware, and the resulting interrupt is merely a
notification to Linux.

As far as future accesses to that location are concerned, it is
perfectly fine to use, and thus doesn't need to be included in the above
badblocks list.

Add a check in the nfit MCE handler to filter out corrected mce events,
and only process uncorrectable errors.

Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Reported-by: Omar Avelar <omar.avelar@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>
CC: elliott@hpe.com
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
CC: linux-edac <linux-edac@vger.kernel.org>
CC: linux-nvdimm@lists.01.org
CC: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
CC: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Ross Zwisler <zwisler@kernel.org>
CC: stable <stable@vger.kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: x86-ml <x86@kernel.org>
CC: Yazen Ghannam <yazen.ghannam@amd.com>
Link: http://lkml.kernel.org/r/20181026003729.8420-1-vishal.l.verma@intel.com
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4da9b1c58d28..dbd9fe2f6163 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -221,6 +221,7 @@ static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_am
 
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
+bool mce_is_correctable(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 8c66d2fc8f81..77527b8ea982 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_is_memory_error);
 
-static bool mce_is_correctable(struct mce *m)
+bool mce_is_correctable(struct mce *m)
 {
 	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
 		return false;
@@ -547,6 +547,7 @@ static bool mce_is_correctable(struct mce *m)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(mce_is_correctable);
 
 static bool cec_add_mce(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index e9626bf6ca29..7a51707f87e9 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_spa *nfit_spa;
 
-	/* We only care about memory errors */
-	if (!mce_is_memory_error(mce))
+	/* We only care about uncorrectable memory errors */
+	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
 	/*

[tip:x86/urgent] acpi/nfit, x86/mce: Handle only uncorrectable machine checks

[tip-bot for Borislav Petkov]

Commit-ID:  5d96c9342c23ee1d084802dcf064caa67ecaa45b
Gitweb:     https://git.kernel.org/tip/5d96c9342c23ee1d084802dcf064caa67ecaa45b
Author:     Vishal Verma <vishal.l.verma@intel.com>
AuthorDate: Thu, 25 Oct 2018 18:37:28 -0600
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 6 Nov 2018 19:13:10 +0100

acpi/nfit, x86/mce: Handle only uncorrectable machine checks

The MCE handler for nfit devices is called for memory errors on a
Non-Volatile DIMM and adds the error location to a 'badblocks' list.
This list is used by the various NVDIMM drivers to avoid consuming known
poison locations during IO.

The MCE handler gets called for both corrected and uncorrectable errors.
Until now, both kinds of errors have been added to the badblocks list.
However, corrected memory errors indicate that the problem has already
been fixed by hardware, and the resulting interrupt is merely a
notification to Linux.

As far as future accesses to that location are concerned, it is
perfectly fine to use, and thus doesn't need to be included in the above
badblocks list.

Add a check in the nfit MCE handler to filter out corrected mce events,
and only process uncorrectable errors.

Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Reported-by: Omar Avelar <omar.avelar@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>
CC: elliott@hpe.com
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
CC: linux-edac <linux-edac@vger.kernel.org>
CC: linux-nvdimm@lists.01.org
CC: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
CC: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Ross Zwisler <zwisler@kernel.org>
CC: stable <stable@vger.kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: x86-ml <x86@kernel.org>
CC: Yazen Ghannam <yazen.ghannam@amd.com>
Link: http://lkml.kernel.org/r/20181026003729.8420-1-vishal.l.verma@intel.com
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4da9b1c58d28..dbd9fe2f6163 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -221,6 +221,7 @@ static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_am
 
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
+bool mce_is_correctable(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 8c66d2fc8f81..77527b8ea982 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_is_memory_error);
 
-static bool mce_is_correctable(struct mce *m)
+bool mce_is_correctable(struct mce *m)
 {
 	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
 		return false;
@@ -547,6 +547,7 @@ static bool mce_is_correctable(struct mce *m)
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(mce_is_correctable);
 
 static bool cec_add_mce(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index e9626bf6ca29..7a51707f87e9 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	struct acpi_nfit_desc *acpi_desc;
 	struct nfit_spa *nfit_spa;
 
-	/* We only care about memory errors */
-	if (!mce_is_memory_error(mce))
+	/* We only care about uncorrectable memory errors */
+	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
 	/*

[tip:x86/urgent] acpi/nfit, x86/mce: Validate a MCE's address before using it

[tip-bot for Vishal Verma]

Commit-ID:  e8a308e5f47e545e0d41d0686c00f5f5217c5f61
Gitweb:     https://git.kernel.org/tip/e8a308e5f47e545e0d41d0686c00f5f5217c5f61
Author:     Vishal Verma <vishal.l.verma@intel.com>
AuthorDate: Thu, 25 Oct 2018 18:37:29 -0600
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 6 Nov 2018 19:13:26 +0100

acpi/nfit, x86/mce: Validate a MCE's address before using it

The NFIT machine check handler uses the physical address from the mce
structure, and compares it against information in the ACPI NFIT table
to determine whether that location lies on an NVDIMM. The mce->addr
field however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Reported-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Williams <dan.j.williams@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>
CC: elliott@hpe.com
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
CC: linux-edac <linux-edac@vger.kernel.org>
CC: linux-nvdimm@lists.01.org
CC: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
CC: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Ross Zwisler <zwisler@kernel.org>
CC: stable <stable@vger.kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: x86-ml <x86@kernel.org>
CC: Yazen Ghannam <yazen.ghannam@amd.com>
Link: http://lkml.kernel.org/r/20181026003729.8420-2-vishal.l.verma@intel.com
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index dbd9fe2f6163..c1a812bd5a27 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -222,6 +222,7 @@ static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_am
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 77527b8ea982..36d2696c9563 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d6c1b10f6c25 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the MCE is valid. */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see

[tip:x86/urgent] acpi/nfit, x86/mce: Validate a MCE's address before using it

[tip-bot for Borislav Petkov]

Commit-ID:  e8a308e5f47e545e0d41d0686c00f5f5217c5f61
Gitweb:     https://git.kernel.org/tip/e8a308e5f47e545e0d41d0686c00f5f5217c5f61
Author:     Vishal Verma <vishal.l.verma@intel.com>
AuthorDate: Thu, 25 Oct 2018 18:37:29 -0600
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 6 Nov 2018 19:13:26 +0100

acpi/nfit, x86/mce: Validate a MCE's address before using it

The NFIT machine check handler uses the physical address from the mce
structure, and compares it against information in the ACPI NFIT table
to determine whether that location lies on an NVDIMM. The mce->addr
field however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Reported-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Williams <dan.j.williams@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>
CC: elliott@hpe.com
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Len Brown <lenb@kernel.org>
CC: linux-acpi@vger.kernel.org
CC: linux-edac <linux-edac@vger.kernel.org>
CC: linux-nvdimm@lists.01.org
CC: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
CC: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Ross Zwisler <zwisler@kernel.org>
CC: stable <stable@vger.kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: x86-ml <x86@kernel.org>
CC: Yazen Ghannam <yazen.ghannam@amd.com>
Link: http://lkml.kernel.org/r/20181026003729.8420-2-vishal.l.verma@intel.com
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index dbd9fe2f6163..c1a812bd5a27 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -222,6 +222,7 @@ static inline void mce_hygon_feature_init(struct cpuinfo_x86 *c) { return mce_am
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 77527b8ea982..36d2696c9563 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d6c1b10f6c25 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the MCE is valid. */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Hi, Vishal,

Vishal Verma <vishal.l.verma@intel.com> writes:

> The mce handler for 'nfit' devices is called for memory errors on a
> Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
> This list is used by the various NVDIMM drivers to avoid consuming known
> poison locations during IO.
>
> The mce handler gets called for both corrected and uncorrectable errors.

Sorry for necroposting.  I thought the point of the CEC was to make sure
that the other registered decoders only ever saw uncorrected errors.
How do we end up getting called with a correctable error?

Thanks,
Jeff

> Until now, both kinds of errors have been added to the badblocks list.
> However, corrected memory errors indicate that the problem has already
> been fixed by hardware, and the resulting interrupt is merely a
> notification to Linux. As far as future accesses to that location are
> concerned, it is perfectly fine to use, and thus doesn't need to be
> included in the above badblocks list.
>
> Add a check in the nfit mce handler to filter out corrected mce events,
> and only process uncorrectable errors.
>
> Reported-by: Omar Avelar <omar.avelar@intel.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++--
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> v3: Unchanged from v2
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 3a17107594c8..3111b3cee2ee 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
>  
>  int mce_available(struct cpuinfo_x86 *c);
>  bool mce_is_memory_error(struct mce *m);
> +bool mce_is_correctable(struct mce *m);
>  
>  DECLARE_PER_CPU(unsigned, mce_exception_count);
>  DECLARE_PER_CPU(unsigned, mce_poll_count);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 953b3ce92dcc..27015948bc41 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
>  }
>  EXPORT_SYMBOL_GPL(mce_is_memory_error);
>  
> -static bool mce_is_correctable(struct mce *m)
> +bool mce_is_correctable(struct mce *m)
>  {
>  	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
>  		return false;
> @@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
>  
>  	return true;
>  }
> +EXPORT_SYMBOL_GPL(mce_is_correctable);
>  
>  static bool cec_add_mce(struct mce *m)
>  {
> diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
> index e9626bf6ca29..7a51707f87e9 100644
> --- a/drivers/acpi/nfit/mce.c
> +++ b/drivers/acpi/nfit/mce.c
> @@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
>  	struct acpi_nfit_desc *acpi_desc;
>  	struct nfit_spa *nfit_spa;
>  
> -	/* We only care about memory errors */
> -	if (!mce_is_memory_error(mce))
> +	/* We only care about uncorrectable memory errors */
> +	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
>  		return NOTIFY_DONE;
>  
>  	/*
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Hi, Vishal,

Vishal Verma <vishal.l.verma@intel.com> writes:

> The mce handler for 'nfit' devices is called for memory errors on a
> Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
> This list is used by the various NVDIMM drivers to avoid consuming known
> poison locations during IO.
>
> The mce handler gets called for both corrected and uncorrectable errors.

Sorry for necroposting.  I thought the point of the CEC was to make sure
that the other registered decoders only ever saw uncorrected errors.
How do we end up getting called with a correctable error?

Thanks,
Jeff

> Until now, both kinds of errors have been added to the badblocks list.
> However, corrected memory errors indicate that the problem has already
> been fixed by hardware, and the resulting interrupt is merely a
> notification to Linux. As far as future accesses to that location are
> concerned, it is perfectly fine to use, and thus doesn't need to be
> included in the above badblocks list.
>
> Add a check in the nfit mce handler to filter out corrected mce events,
> and only process uncorrectable errors.
>
> Reported-by: Omar Avelar <omar.avelar@intel.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++--
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> v3: Unchanged from v2
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 3a17107594c8..3111b3cee2ee 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
>  
>  int mce_available(struct cpuinfo_x86 *c);
>  bool mce_is_memory_error(struct mce *m);
> +bool mce_is_correctable(struct mce *m);
>  
>  DECLARE_PER_CPU(unsigned, mce_exception_count);
>  DECLARE_PER_CPU(unsigned, mce_poll_count);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 953b3ce92dcc..27015948bc41 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
>  }
>  EXPORT_SYMBOL_GPL(mce_is_memory_error);
>  
> -static bool mce_is_correctable(struct mce *m)
> +bool mce_is_correctable(struct mce *m)
>  {
>  	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
>  		return false;
> @@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
>  
>  	return true;
>  }
> +EXPORT_SYMBOL_GPL(mce_is_correctable);
>  
>  static bool cec_add_mce(struct mce *m)
>  {
> diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
> index e9626bf6ca29..7a51707f87e9 100644
> --- a/drivers/acpi/nfit/mce.c
> +++ b/drivers/acpi/nfit/mce.c
> @@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
>  	struct acpi_nfit_desc *acpi_desc;
>  	struct nfit_spa *nfit_spa;
>  
> -	/* We only care about memory errors */
> -	if (!mce_is_memory_error(mce))
> +	/* We only care about uncorrectable memory errors */
> +	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
>  		return NOTIFY_DONE;
>  
>  	/*

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Hi, Vishal,

Vishal Verma <vishal.l.verma@intel.com> writes:

> The mce handler for 'nfit' devices is called for memory errors on a
> Non-Volatile DIMM, and adds the error location to a 'badblocks' list.
> This list is used by the various NVDIMM drivers to avoid consuming known
> poison locations during IO.
>
> The mce handler gets called for both corrected and uncorrectable errors.

Sorry for necroposting.  I thought the point of the CEC was to make sure
that the other registered decoders only ever saw uncorrected errors.
How do we end up getting called with a correctable error?

Thanks,
Jeff

> Until now, both kinds of errors have been added to the badblocks list.
> However, corrected memory errors indicate that the problem has already
> been fixed by hardware, and the resulting interrupt is merely a
> notification to Linux. As far as future accesses to that location are
> concerned, it is perfectly fine to use, and thus doesn't need to be
> included in the above badblocks list.
>
> Add a check in the nfit mce handler to filter out corrected mce events,
> and only process uncorrectable errors.
>
> Reported-by: Omar Avelar <omar.avelar@intel.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++--
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> v3: Unchanged from v2
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 3a17107594c8..3111b3cee2ee 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -216,6 +216,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
>  
>  int mce_available(struct cpuinfo_x86 *c);
>  bool mce_is_memory_error(struct mce *m);
> +bool mce_is_correctable(struct mce *m);
>  
>  DECLARE_PER_CPU(unsigned, mce_exception_count);
>  DECLARE_PER_CPU(unsigned, mce_poll_count);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 953b3ce92dcc..27015948bc41 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -534,7 +534,7 @@ bool mce_is_memory_error(struct mce *m)
>  }
>  EXPORT_SYMBOL_GPL(mce_is_memory_error);
>  
> -static bool mce_is_correctable(struct mce *m)
> +bool mce_is_correctable(struct mce *m)
>  {
>  	if (m->cpuvendor == X86_VENDOR_AMD && m->status & MCI_STATUS_DEFERRED)
>  		return false;
> @@ -544,6 +544,7 @@ static bool mce_is_correctable(struct mce *m)
>  
>  	return true;
>  }
> +EXPORT_SYMBOL_GPL(mce_is_correctable);
>  
>  static bool cec_add_mce(struct mce *m)
>  {
> diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
> index e9626bf6ca29..7a51707f87e9 100644
> --- a/drivers/acpi/nfit/mce.c
> +++ b/drivers/acpi/nfit/mce.c
> @@ -25,8 +25,8 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
>  	struct acpi_nfit_desc *acpi_desc;
>  	struct nfit_spa *nfit_spa;
>  
> -	/* We only care about memory errors */
> -	if (!mce_is_memory_error(mce))
> +	/* We only care about uncorrectable memory errors */
> +	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
>  		return NOTIFY_DONE;
>  
>  	/*

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

Drop stable@

On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
> Sorry for necroposting.  I thought the point of the CEC was to make sure
> that the other registered decoders only ever saw uncorrected errors.

Ha, good point! You mean drivers/ras/cec.c, right?

If so, then I don't think we've ever talked about connecting CEC with
NVDIMM and whether that would make sense. Lemme add Dan.

> How do we end up getting called with a correctable error?

Good question. We shouldn't.

So we need to figure out here how exactly should those things interact
and whether NFIT should get all errors reported or it should put all the
correctable errors through the decay thing, see the comment at the top
of drivers/ras/cec.c

Thx for pointing that out Jeff.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

Drop stable@

On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
> Sorry for necroposting.  I thought the point of the CEC was to make sure
> that the other registered decoders only ever saw uncorrected errors.

Ha, good point! You mean drivers/ras/cec.c, right?

If so, then I don't think we've ever talked about connecting CEC with
NVDIMM and whether that would make sense. Lemme add Dan.

> How do we end up getting called with a correctable error?

Good question. We shouldn't.

So we need to figure out here how exactly should those things interact
and whether NFIT should get all errors reported or it should put all the
correctable errors through the decay thing, see the comment at the top
of drivers/ras/cec.c

Thx for pointing that out Jeff.

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Borislav Petkov <bp@alien8.de> writes:

> Drop stable@
>
> On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
>> Sorry for necroposting.  I thought the point of the CEC was to make sure
>> that the other registered decoders only ever saw uncorrected errors.
>
> Ha, good point! You mean drivers/ras/cec.c, right?

Yes.

> If so, then I don't think we've ever talked about connecting CEC with
> NVDIMM and whether that would make sense. Lemme add Dan.

I don't think there's a difference between MCEs for NVDIMMs and normal
DRAM.  I'll let Dan confirm or deny that.

>> How do we end up getting called with a correctable error?
>
> Good question. We shouldn't.
>
> So we need to figure out here how exactly should those things interact
> and whether NFIT should get all errors reported or it should put all the
> correctable errors through the decay thing, see the comment at the top
> of drivers/ras/cec.c
>
> Thx for pointing that out Jeff.

Sure, thanks for the quick reply, Boris!  Also, thanks for your detailed
commit messages.  They really help with understanding the code changes.

Cheers,
Jeff
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Borislav Petkov <bp@alien8.de> writes:

> Drop stable@
>
> On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
>> Sorry for necroposting.  I thought the point of the CEC was to make sure
>> that the other registered decoders only ever saw uncorrected errors.
>
> Ha, good point! You mean drivers/ras/cec.c, right?

Yes.

> If so, then I don't think we've ever talked about connecting CEC with
> NVDIMM and whether that would make sense. Lemme add Dan.

I don't think there's a difference between MCEs for NVDIMMs and normal
DRAM.  I'll let Dan confirm or deny that.

>> How do we end up getting called with a correctable error?
>
> Good question. We shouldn't.
>
> So we need to figure out here how exactly should those things interact
> and whether NFIT should get all errors reported or it should put all the
> correctable errors through the decay thing, see the comment at the top
> of drivers/ras/cec.c
>
> Thx for pointing that out Jeff.

Sure, thanks for the quick reply, Boris!  Also, thanks for your detailed
commit messages.  They really help with understanding the code changes.

Cheers,
Jeff

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Wed, Feb 20, 2019 at 02:26:45PM -0500, Jeff Moyer wrote:
> Sure, thanks for the quick reply, Boris!  Also, thanks for your detailed
> commit messages.  They really help with understanding the code changes.

Ha!

>From now on I'll be showing that to *anyone* who complains when I ask of
them to make their commit messages understandable!

:-)

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Wed, Feb 20, 2019 at 02:26:45PM -0500, Jeff Moyer wrote:
> Sure, thanks for the quick reply, Boris!  Also, thanks for your detailed
> commit messages.  They really help with understanding the code changes.

Ha!

From now on I'll be showing that to *anyone* who complains when I ask of
them to make their commit messages understandable!

:-)

Thanks!

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Dan Williams]

On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Borislav Petkov <bp@alien8.de> writes:
>
> > Drop stable@
> >
> > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
> >> Sorry for necroposting.  I thought the point of the CEC was to make sure
> >> that the other registered decoders only ever saw uncorrected errors.
> >
> > Ha, good point! You mean drivers/ras/cec.c, right?
>
> Yes.
>
> > If so, then I don't think we've ever talked about connecting CEC with
> > NVDIMM and whether that would make sense. Lemme add Dan.
>
> I don't think there's a difference between MCEs for NVDIMMs and normal
> DRAM.  I'll let Dan confirm or deny that.

There is a difference. NVDIMMs have local tracking of discovered
poison, methods to scan for latent poison, and methods to clear. A CEC
connection, iiuc, would seem an awkward fit. Awkward because what CEC
enables is meant to be implemented natively in the hardware, and CEC
seems to have no concept of the fact that errors can be repaired.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Dan Williams]

On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Borislav Petkov <bp@alien8.de> writes:
>
> > Drop stable@
> >
> > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
> >> Sorry for necroposting.  I thought the point of the CEC was to make sure
> >> that the other registered decoders only ever saw uncorrected errors.
> >
> > Ha, good point! You mean drivers/ras/cec.c, right?
>
> Yes.
>
> > If so, then I don't think we've ever talked about connecting CEC with
> > NVDIMM and whether that would make sense. Lemme add Dan.
>
> I don't think there's a difference between MCEs for NVDIMMs and normal
> DRAM.  I'll let Dan confirm or deny that.

There is a difference. NVDIMMs have local tracking of discovered
poison, methods to scan for latent poison, and methods to clear. A CEC
connection, iiuc, would seem an awkward fit. Awkward because what CEC
enables is meant to be implemented natively in the hardware, and CEC
seems to have no concept of the fact that errors can be repaired.

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Wed, Feb 20, 2019 at 11:40:10AM -0800, Dan Williams wrote:
> There is a difference. NVDIMMs have local tracking of discovered
> poison, methods to scan for latent poison, and methods to clear. A CEC
> connection, iiuc, would seem an awkward fit. Awkward because what CEC
> enables is meant to be implemented natively in the hardware, and CEC
> seems to have no concept of the fact that errors can be repaired.

CEC is a leaky bucket of sorts which does call memory_failure_queue() in
the end. So we poison only those errors which report the same address
over and over again.

Correctable errors are by definition already repaired, i.e., corrected
so there's no need to do anything.

The way stuff is plumbed now is, all correctable errors go to the CEC so
NFIT doesn't see them, if CEC is enabled.

But the patch Jeff quoted already changed NFIT to ignore correctable
errors so I guess we don't have to do anything. And this is still needed
for the case where CEC is not enabled.

I'd say.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Wed, Feb 20, 2019 at 11:40:10AM -0800, Dan Williams wrote:
> There is a difference. NVDIMMs have local tracking of discovered
> poison, methods to scan for latent poison, and methods to clear. A CEC
> connection, iiuc, would seem an awkward fit. Awkward because what CEC
> enables is meant to be implemented natively in the hardware, and CEC
> seems to have no concept of the fact that errors can be repaired.

CEC is a leaky bucket of sorts which does call memory_failure_queue() in
the end. So we poison only those errors which report the same address
over and over again.

Correctable errors are by definition already repaired, i.e., corrected
so there's no need to do anything.

The way stuff is plumbed now is, all correctable errors go to the CEC so
NFIT doesn't see them, if CEC is enabled.

But the patch Jeff quoted already changed NFIT to ignore correctable
errors so I guess we don't have to do anything. And this is still needed
for the case where CEC is not enabled.

I'd say.

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Borislav Petkov <bp@alien8.de> writes:
>>
>> > Drop stable@
>> >
>> > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
>> >> Sorry for necroposting.  I thought the point of the CEC was to make sure
>> >> that the other registered decoders only ever saw uncorrected errors.
>> >
>> > Ha, good point! You mean drivers/ras/cec.c, right?
>>
>> Yes.
>>
>> > If so, then I don't think we've ever talked about connecting CEC with
>> > NVDIMM and whether that would make sense. Lemme add Dan.
>>
>> I don't think there's a difference between MCEs for NVDIMMs and normal
>> DRAM.  I'll let Dan confirm or deny that.
>
> There is a difference. NVDIMMs have local tracking of discovered
> poison, methods to scan for latent poison, and methods to clear.

What I meant was that you couldn't tell the difference between an MCE
generated by accessing DRAM vs one generated by accessing an NVDIMM
(aside from checking the address).

> A CEC connection, iiuc, would seem an awkward fit. Awkward because
> what CEC enables is meant to be implemented natively in the hardware,
> and CEC seems to have no concept of the fact that errors can be
> repaired.

As far as I can tell, the Correctable Errors Collector just eats
correctable errors so that the rest of the registered decoders don't
have to worry about receiving them.  It sounds like you're suggesting
that NVDIMMs won't spew correctable errors.  If that's the case (I don't
think it is), then there's no need at all for these patches.

Anyway, given that the correctable errors collector can be turned off in
the kernel config, and assuming that we still can get correctable errors
from NVDIMMs (I think we can, since I believe the caching hierarchy can
generate them as well), we definitely need to continue to check for
correctable errors in the nfit mce decoder.  That's something I had
overlooked.

Cheers,
Jeff
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> On Wed, Feb 20, 2019 at 11:26 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Borislav Petkov <bp@alien8.de> writes:
>>
>> > Drop stable@
>> >
>> > On Wed, Feb 20, 2019 at 01:59:15PM -0500, Jeff Moyer wrote:
>> >> Sorry for necroposting.  I thought the point of the CEC was to make sure
>> >> that the other registered decoders only ever saw uncorrected errors.
>> >
>> > Ha, good point! You mean drivers/ras/cec.c, right?
>>
>> Yes.
>>
>> > If so, then I don't think we've ever talked about connecting CEC with
>> > NVDIMM and whether that would make sense. Lemme add Dan.
>>
>> I don't think there's a difference between MCEs for NVDIMMs and normal
>> DRAM.  I'll let Dan confirm or deny that.
>
> There is a difference. NVDIMMs have local tracking of discovered
> poison, methods to scan for latent poison, and methods to clear.

What I meant was that you couldn't tell the difference between an MCE
generated by accessing DRAM vs one generated by accessing an NVDIMM
(aside from checking the address).

> A CEC connection, iiuc, would seem an awkward fit. Awkward because
> what CEC enables is meant to be implemented natively in the hardware,
> and CEC seems to have no concept of the fact that errors can be
> repaired.

As far as I can tell, the Correctable Errors Collector just eats
correctable errors so that the rest of the registered decoders don't
have to worry about receiving them.  It sounds like you're suggesting
that NVDIMMs won't spew correctable errors.  If that's the case (I don't
think it is), then there's no need at all for these patches.

Anyway, given that the correctable errors collector can be turned off in
the kernel config, and assuming that we still can get correctable errors
from NVDIMMs (I think we can, since I believe the caching hierarchy can
generate them as well), we definitely need to continue to check for
correctable errors in the nfit mce decoder.  That's something I had
overlooked.

Cheers,
Jeff

Re: [PATCH v3 1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Thu, Feb 21, 2019 at 11:11:27AM -0500, Jeff Moyer wrote:
> Anyway, given that the correctable errors collector can be turned off in
> the kernel config,

Also, on the cmdline, for whatever reason:

ras=cec_disable

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[v3,1/2] nfit, mce: only handle uncorrectable machine checks

[Borislav Petkov]

On Thu, Feb 21, 2019 at 11:11:27AM -0500, Jeff Moyer wrote:
> Anyway, given that the correctable errors collector can be turned off in
> the kernel config,

Also, on the cmdline, for whatever reason:

ras=cec_disable