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

[Dave Hansen]

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.han...@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.