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

[Borislav Petkov <bp@alien8.de>]

On Wed, May 22, 2013 at 02:10:29AM -0400, Chen Gong wrote:
> > +	 * 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.

No, please not in the patch description as no one would read it there.
You could move it above the function definition. Also, you could
simplify it a bit.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--