Re: [PATCH v2 2/2] x86/pti: check the return value of pti_user_pagetable_walk_pmd

[Dave Hansen <dave.hansen@linux.intel.com>]

On 07/17/2018 12:23 AM, Jiang Biao wrote:
> Check the return value of pti_user_pagetable_walk_pmd() to avoid
> NULL pointer dereference. And add warning for fail allocation.

For both of these:

Acked-by: Dave Hansen <dave.hansen@intel.com>

It's minor, but if you redo these, I'd appreciate a slightly different
form.  Instead of:

> @@ -239,8 +239,10 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
>  static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
>  {
>  	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
> -	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
>  	pte_t *pte;
> +	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
> +	if (!pmd)
> +		return NULL;

I'd much rather see separation of code -- especially _important_ code
like an allocation -- from local variable definitions.  Like this:

	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
	pmd_t *pmd;
  	pte_t *pte;

	pmd = pti_user_pagetable_walk_pmd(address);
	if (!pmd)
		return NULL;

That clearly separtes the variables from the _code_ and also nicely
pairs the action with the check for that action being successful.