Re: [PATCH v7 06/25] ACPI / APEI: Don't store CPER records physical address in struct ghes
From: Borislav Petkov <bp@alien8.de>
Date: 2018-12-11 17:04:40
Also in:
kvmarm, linux-acpi, linux-mm
On Mon, Dec 03, 2018 at 06:05:54PM +0000, James Morse wrote:
When CPER records are found the address of the records is stashed in the struct ghes. Once the records have been processed, this address is overwritten with zero so that it won't be processed again without being re-populated by firmware. This goes wrong if a struct ghes can be processed concurrently, as can happen at probe time when an NMI occurs. If the NMI arrives on another CPU, the probing CPU may call ghes_clear_estatus() on the records before the handler had finished with them. Even on the same CPU, once the interrupted handler is resumed, it will call ghes_clear_estatus() on the NMIs records, this memory may have already been re-used by firmware. Avoid this stashing by letting the caller hold the address. A later patch will do away with the use of ghes->flags in the read/clear code too. Signed-off-by: James Morse <james.morse@arm.com> --- Changes since v6: * Moved earlier in the series * Added buf_adder = 0 on all the error paths, and test for it in ghes_estatus_clear() for extra sanity. --- drivers/acpi/apei/ghes.c | 40 +++++++++++++++++++++++----------------- include/acpi/ghes.h | 1 - 2 files changed, 23 insertions(+), 18 deletions(-)
...
quoted hunk ↗ jump to hunk
@@ -349,17 +350,20 @@ static int ghes_read_estatus(struct ghes *ghes) if (rc) pr_warn_ratelimited(FW_WARN GHES_PFX "Failed to read error status block!\n"); + return rc; } -static void ghes_clear_estatus(struct ghes *ghes) +static void ghes_clear_estatus(struct ghes *ghes, u64 buf_paddr) { ghes->estatus->block_status = 0; if (!(ghes->flags & GHES_TO_CLEAR)) return; - ghes_copy_tofrom_phys(ghes->estatus, ghes->buffer_paddr, - sizeof(ghes->estatus->block_status), 0); - ghes->flags &= ~GHES_TO_CLEAR;
<---- newline here.
+ if (buf_paddr) {Also, you can save yourself an indendation level: if (!buf_paddr) return; ghes_copy...
+ ghes_copy_tofrom_phys(ghes->estatus, buf_paddr, + sizeof(ghes->estatus->block_status), 0); + ghes->flags &= ~GHES_TO_CLEAR; + } }
With that addressed:
Reviewed-by: Borislav Petkov <redacted>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel