Re: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection

[Chen Gong <gong.chen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

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@intel.com>
> To: Chen Gong <gong.chen@linux.intel.com>, "bp@alien8.de" <bp@alien8.de>
> CC: "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
> 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]