On Thu, May 16, 2013 at 01:05:03PM +0200, Borislav Petkov wrote:
> Date: Thu, 16 May 2013 13:05:03 +0200
> From: Borislav Petkov <bp@xxxxxxxxx>
> To: Chen Gong <gong.chen@xxxxxxxxxxxxxxx>
> Cc: tony.luck@xxxxxxxxx, linux-acpi@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error
> injection
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Wed, May 15, 2013 at 10:29:06PM -0400, Chen Gong wrote:
> > When param1 is enabled in EINJ but not assigned with a valid
> > value, sometimes it will cause the error like below:
> >
> > APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger
> > registers
> >
> > It is because some firmware will access target address specified in
> > param1 to trigger the error when injecting memory error. This will
> > cause resource conflict with regular memory. So It must be removed
> > from trigger table resources, but uncorrected param1/param2
> > combination will stop this action. Add extra check to avoid
> > happening this error.
> >
> > Signed-off-by: Chen Gong <gong.chen@xxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/apei/einj.c | 29 +++++++++++++++++++++++++----
> > kernel/resource.c | 1 +
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> > index 8d457b5..015546e 100644
> > --- a/drivers/acpi/apei/einj.c
> > +++ b/drivers/acpi/apei/einj.c
> > @@ -32,6 +32,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/nmi.h>
> > #include <linux/delay.h>
> > +#include <linux/mm.h>
> > #include <acpi/acpi.h>
> >
> > #include "apei-internal.h"
> > @@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
> > /* Inject the specified hardware error */
> > static int einj_error_inject(u32 type, u64 param1, u64 param2)
> > {
> > - int rc;
> > + int rc = 0;
> > + unsigned long pfn;
> > + bool checkparam = false;
> > + u64 param2_mask = 0xfffffffffffff000;
> >
> > - mutex_lock(&einj_mutex);
> > - rc = __einj_error_inject(type, param1, param2);
> > - mutex_unlock(&einj_mutex);
> > + /* ensure param1/param2 existed & injection is memory related */
> > + if (param_extension || acpi5) {
> > + if (type & 0x80000000) {
>
> if (type & BIT(31)) {
>
> better readable.
>
> even better if you define what bit 31 is
>
> #define INJ_TYPE BIT(31)
>
> > + if (vendor_flags == SETWA_FLAGS_MEM)
> > + checkparam = true;
> > + } else if (type & 0x38)
>
> what is 0x38? Also a macro define with readable name pls.
>
> > + checkparam = true;
> > + }
> > + if (checkparam) {
> > + pfn = PFN_DOWN(param1 & param2);
> > + if (!page_is_ram(pfn) ||
> > + ((param2 & param2_mask) != param2_mask))
>
> Hmm, shouldn't this be:
>
> (param2 & param2_mask) != param2
>
> ?
>
> > + rc = -EINVAL;
> > + }
>
> This checkparam variable looks unneeded to me:
>
> if (type & INJ_TYPE) {
> if (vendor_flags == SETWA_FLAGS_MEM || type & 0x38) {
> ...
>
> Ok, I went and rewrote the function to show you what I mean. I've
> also reversed the logic to flatten the indentation level (completely
> untested, of course):
>
> static int einj_error_inject(u32 type, u64 param1, u64 param2)
> {
> int rc;
> unsigned long pfn;
>
> /* ensure param1/param2 exist and we inject to real memory */
> if (!(param_extension || acpi5))
> goto inject;
>
> if (type & INJ_TYPE)
> if (vendor_flags != SETWA_FLAGS_MEM)
> goto inject;
> else if (!(type & THREE_BIT_MASK))
> goto inject;
>
> pfn = PFN_DOWN(param1 & param2);
> if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != param2))
> return -EINVAL;
>
> inject:
> mutex_lock(&einj_mutex);
> rc = __einj_error_inject(type, param1, param2);
> mutex_unlock(&einj_mutex);
>
> return rc;
> }
>
> --
> Regards/Gruss,
> Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --
Hi, Boris
Thanks for your review. I agree with you about readability enhancement.
IMHO, I don't think your attached patch has obvious improvment. I paste
my updated patch here and please continue to review. I will send updated
patch based on your further review.
In this patch, I update previous confused definition at the same time.
They are very little and no ill-efect so I don't write a new patch
for it.
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 8d457b5..dba59be 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -32,6 +32,7 @@
#include <linux/seq_file.h>
#include <linux/nmi.h>
#include <linux/delay.h>
+#include <linux/mm.h>
#include <acpi/acpi.h>
#include "apei-internal.h"
@@ -41,6 +42,11 @@
#define SPIN_UNIT 100 /* 100ns */
/* Firmware should respond within 1 milliseconds */
#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC)
+#define ACPI5_VENDOR_BIT BIT(31)
+#define PAGE_MASK 0xfffffffffffff000
+#define MEM_ERROR_MASK (ACPI_EINJ_MEMORY_CORRECTABLE | \
+ ACPI_EINJ_MEMORY_UNCORRECTABLE | \
+ ACPI_EINJ_MEMORY_FATAL)
/*
* ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
@@ -367,7 +373,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
* This will cause resource conflict with regular memory. So
* remove it from trigger table resources.
*/
- if ((param_extension || acpi5) && (type & 0x0038) && param2) {
+ if ((param_extension || acpi5) && (type & MEM_ERROR_MASK) && param2) {
struct apei_resources addr_resources;
apei_resources_init(&addr_resources);
trigger_param_region = einj_get_trigger_parameter_region(
@@ -427,7 +433,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
struct set_error_type_with_address *v5param = einj_param;
tototototoram->type = type;
- if (type & 0x80000000) {
+ if (type & ACPI5_VENDOR_BIT) {
switch (vendor_flags) {
case SETWA_FLAGS_APICID:
v5param->apicid = param1;
@@ -512,6 +518,25 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
static int einj_error_inject(u32 type, u64 param1, u64 param2)
{
int rc;
+ unsigned long pfn;
+ bool checkparam;
+
+ /* ensure param1/param2 existed & injection is memory related */
+ if (param_extension || acpi5) {
+ checkparam = false;
+ if (type & ACPI5_VENDOR_BIT) {
+ if (vendor_flags == SETWA_FLAGS_MEM)
+ checkparam = true;
+ } else if (type & MEM_ERROR_MASK)
+ checkparam = true;
+
+ if (checkparam) {
+ pfn = PFN_DOWN(param1 & param2);
+ if (!page_is_ram(pfn) ||
+ ((param2 & PAGE_MASK) != PAGE_MASK))
+ return -EINVAL;
+ }
+ }
mutex_lock(&einj_mutex);
rc = __einj_error_inject(type, param1, param2);
@@ -590,7 +615,7 @@ static int error_type_set(void *data, u64 val)
* Vendor defined types have 0x80000000 bit set, and
* are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
*/
- vendor = val & 0x80000000;
+ vendor = val & ACPI5_VENDOR_BIT;
tval = val & 0x7fffffff;
/* Only one error type can be specified */
diff --git a/kernel/resource.c b/kernel/resource.c
index d738698..77bf11a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -409,6 +409,7 @@ int __weak page_is_ram(unsigned long pfn)
{
return walk_system_ram_range(pfn, 1, NULL, __is_ram) == 1;
}
+EXPORT_SYMBOL_GPL(page_is_ram);
void __weak arch_remove_reservations(struct resource *avail)
{
Attachment:
signature.asc
Description: Digital signature
