On Tue, May 21, 2013 at 08:19:16PM +0000, Luck, Tony wrote:
> Date: Tue, 21 May 2013 20:19:16 +0000
> From: "Luck, Tony" <tony.luck@xxxxxxxxx>
> To: Chen Gong <gong.chen@xxxxxxxxxxxxxxx>, "bp@xxxxxxxxx" <bp@xxxxxxxxx>
> CC: "linux-acpi@xxxxxxxxxxxxxxx" <linux-acpi@xxxxxxxxxxxxxxx>
> Subject: RE: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error
> injection
>
> + /* ensure param1/param2 existed */
> + if (!(param_extension || acpi5))
> + goto inject;
> +
> + /* ensure injection is memory related */
> + if (type & ACPI5_VENDOR_BIT) {
> + if (vendor_flags != SETWA_FLAGS_MEM)
> + goto inject;
> + } else if (!(type & MEM_ERROR_MASK))
> + goto inject;
>
> Maybe a comment before all these three goto blocks saying why we are jumping
> around. Perhaps:
>
> /* We need extra sanity checks for memory errors. Other types leap directly to injection */
>
Thanks, this comment is great.
> +
> + /*
> + * When error injection type is memory related, param2 is the address
> + * mask of param1. This mask is used to ensure that the final address
> + * (param1 & param2) is meaningful. If param2 has a *weird* style
> + * like 0xf0f0f0f0f0f0f0f0, it means the injection address can be
> + * anywhere around param1, and that must be forbidden. In that reason,
> + * PAGE_MASK is employed to avoid injection address discontinuous.
> + * If one finds a special case not to satisfy this requirement, please
> + * fix it.
> + */
> + pfn = PFN_DOWN(param1 & param2);
> + if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
> + return -EINVAL;
>
> This has too much comment (rare!) and is still too complicated. Split the tests apart?
Your comment is great to me. Boris ever mentioned that
"(param2 & PAGE_MASK) != PAGE_MASK)" is not usual, most of situations are
like "(param2 & PAGE_MASK) != param2). So he wants here I can give a clear
explanation for it. Maybe I can move my explanation into patch description.
>
> /*
> * Disallow crazy address masks that give BIOS leeway to pick injection address
> * almost anywhere. Insist on page or better granularity
> */
> if ((param2 & PAGE_MASK) != PAGE_MASK)
> return -EINVAL;
>
> /* make sure target address is normal memory */
> pfn = PFN_DOWN(param1 & param2);
> if (!page_is_ram(pfn))
> return -EINVAL;
>
Attachment:
signature.asc
Description: Digital signature
