| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 03 Jul 2013 21:10:02 +0530
From: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To: Borislav Petkov <bp@...en8.de>
CC: tony.luck@...el.com, ananth@...ibm.com, masbock@...ux.vnet.ibm.com,
lcm@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, ying.huang@...el.com
Subject: Re: [PATCH v3 3/3] mce, acpi/apei: Soft-offline a page on firmware
GHES notification
On 07/03/2013 08:14 PM, Borislav Petkov wrote:
> On Tue, Jul 02, 2013 at 05:02:48PM +0530, Naveen N. Rao wrote:
>> Here is the updated patch. I also added printk_ratelimit() in line with the
>> rest of the GHES code.
>>
>> Thanks,
>> Naveen
>>
>> --
>> If the firmware indicates in GHES error data entry that the error threshold
>> has exceeded for a corrected error event, then we try to soft-offline the
>> page. This could be called in interrupt context, so we queue this up similar
>> to how we handle memory failure scenarios.
>>
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>
>> ---
>> drivers/acpi/apei/ghes.c | 38 +++++++++++++++++++++++++++++---------
>> include/linux/mm.h | 1 +
>> mm/memory-failure.c | 5 ++++-
>> 3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index fcd7d91..74ef688 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -409,6 +409,34 @@ static void ghes_clear_estatus(struct ghes *ghes)
>> ghes->flags &= ~GHES_TO_CLEAR;
>> }
>>
>> +static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int sev)
>> +{
>> +#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
>> + int sec_sev = ghes_severity(gdata->error_severity);
>> + struct cper_sec_mem_err *mem_err;
>> + mem_err = (struct cper_sec_mem_err *)(gdata+1);
>
> A newline here please. Also, spaces around '+'.
This was borrowed from existing code in ghes_do_proc(), but yes, let me
make this change.
>
>> + if (sec_sev == GHES_SEV_CORRECTED &&
>> + (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
>> + (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
>> + unsigned long pfn;
>
> This pfn is defined twice, move it up to the beginning of the function.
Ok.
>
>> + pfn = mem_err->physical_addr >> PAGE_SHIFT;
>> + if (pfn_valid(pfn))
>> + memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
>> + else if (printk_ratelimit())
>> + pr_warning(FW_WARN GHES_PFX
>
> WARNING: Prefer printk_ratelimited or pr_<level>_ratelimited to printk_ratelimit
> #35: FILE: drivers/acpi/apei/ghes.c:425:
> + else if (printk_ratelimit())
>
> Please run your patches through checkpatch.pl first.
I did run checkpatch.pl, but chose to ignore this warning. ghes.c uses
printk_ratelimit() [hence "in line with the rest of the ghes code" in
patch description] and I felt using it is better in this scenario given
there will be other messages being printed by the rest of the APEI code.
So, rate-limiting these messages globally seems better rather than doing
locally using pr_warn_ratelimited()
>
> This requested change will even simplify the code (ontop of your patch):
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 74ef6882bca9..87e11d468f6b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -422,10 +422,10 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
> pfn = mem_err->physical_addr >> PAGE_SHIFT;
> if (pfn_valid(pfn))
> memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
> - else if (printk_ratelimit())
> - pr_warning(FW_WARN GHES_PFX
> - "Invalid address in generic error data: %#llx\n",
> - mem_err->physical_addr);
> + else
> + pr_warn_ratelimited(FW_WARN GHES_PFX
> + "Invalid address in generic error data: %#llx\n",
> + mem_err->physical_addr);
> }
> if (sev == GHES_SEV_RECOVERABLE &&
> sec_sev == GHES_SEV_RECOVERABLE &&
> ---
>
>
>
>> + "Invalid address in generic error data: %#llx\n",
>> + mem_err->physical_addr);
>> + }
>> + if (sev == GHES_SEV_RECOVERABLE &&
>> + sec_sev == GHES_SEV_RECOVERABLE &&
>> + mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
>> + unsigned long pfn;
>> + pfn = mem_err->physical_addr >> PAGE_SHIFT;
>> + memory_failure_queue(pfn, 0, 0);
>> + }
>> +#endif
>> +}
>> +
>> static void ghes_do_proc(struct ghes *ghes,
>> const struct acpi_hest_generic_status *estatus)
>> {
>> @@ -428,15 +456,7 @@ static void ghes_do_proc(struct ghes *ghes,
>> apei_mce_report_mem_error(sev == GHES_SEV_CORRECTED,
>> mem_err);
>> #endif
>> -#ifdef CONFIG_ACPI_APEI_MEMORY_FAILURE
>> - if (sev == GHES_SEV_RECOVERABLE &&
>> - sec_sev == GHES_SEV_RECOVERABLE &&
>> - mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
>> - unsigned long pfn;
>> - pfn = mem_err->physical_addr >> PAGE_SHIFT;
>> - memory_failure_queue(pfn, 0, 0);
>> - }
>> -#endif
>> + ghes_handle_memory_failure(gdata, sev);
>> }
>> #ifdef CONFIG_ACPI_APEI_PCIEAER
>> else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index e0c8528..958e9efd 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1784,6 +1784,7 @@ enum mf_flags {
>> MF_COUNT_INCREASED = 1 << 0,
>> MF_ACTION_REQUIRED = 1 << 1,
>> MF_MUST_KILL = 1 << 2,
>> + MF_SOFT_OFFLINE = 1 << 3,
>> };
>> extern int memory_failure(unsigned long pfn, int trapno, int flags);
>> extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index ceb0c7f..0d6717e 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1286,7 +1286,10 @@ static void memory_failure_work_func(struct work_struct *work)
>> spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
>> if (!gotten)
>> break;
>> - memory_failure(entry.pfn, entry.trapno, entry.flags);
>> + if (entry.flags & MF_SOFT_OFFLINE)
>> + soft_offline_page(pfn_to_page(entry.pfn), entry.flags);
>> + else
>> + memory_failure(entry.pfn, entry.trapno, entry.flags);
>
> The rest looks ok to me.
>
> I'm guessing this has been tested by injecting errors...?
Yes, at least partially to ensure this works ;)
Thanks,
Naveen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists