Re: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling

[Thomas Gleixner]

On Thu, 3 Dec 2015, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> 
> The current get_user_pages() code is a wee bit more complicated
> than it needs to be for pte bit checking.  Currently, it establishes
> a mask of required pte _PAGE_* bits and ensures that the pte it
> goes after has all those bits.
> 
> This consolidates the three identical copies of this code.
> 
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> ---
> 
>  b/arch/x86/mm/gup.c |   45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff -puN arch/x86/mm/gup.c~pkeys-16-gup-swizzle arch/x86/mm/gup.c
> --- a/arch/x86/mm/gup.c~pkeys-16-gup-swizzle	2015-12-03 16:21:25.148649631 -0800
> +++ b/arch/x86/mm/gup.c	2015-12-03 16:21:25.151649767 -0800
> @@ -63,6 +63,30 @@ retry:
>  #endif
>  }
>  
> +static inline int pte_allows_gup(pte_t pte, int write)
> +{
> +	/*
> +	 * 'pte' can reall be a pte, pmd or pud.  We only check
> +	 * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
> +	 * are the same value on all 3 types.
> +	 */
> +	if (!(pte_flags(pte) & (_PAGE_PRESENT|_PAGE_USER)))
> +		return 0;
> +	if (write && !(pte_write(pte)))
> +		return 0;
> +	return 1;
> +}
> +
> +static inline int pmd_allows_gup(pmd_t pmd, int write)
> +{
> +	return pte_allows_gup(*(pte_t *)&pmd, write);
> +}
> +
> +static inline int pud_allows_gup(pud_t pud, int write)
> +{
> +	return pte_allows_gup(*(pte_t *)&pud, write);
> +}

This still puzzles me. And the only reason it compiles is because we
have -fno-strict-aliasing set ...

All this operates on the pteval or even just on the pte_flags(). Even
the new arch_pte_access_permitted() thingy which you add later is only
interrested in pte_flags() and its just a wrapper around
__pkru_allows_pkey().

So for readability and simplicity sake, can we please do something
like this (pkey check already added):

/*
 * 'pteval' can reall be a pte, pmd or pud.  We only check
 * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
 * are the same value on all 3 types.
 */
static inline int pte_allows_gup(unsigned long pteval, int write)
{
	unsigned long mask = _PAGE_PRESENT|_PAGE_USER;

	if (write)
		mask |= _PAGE_RW;

	if ((pteval & mask) != mask)
		return 0;

	if (!__pkru_allows_pkey(pte_flags_pkey(pteval), write))
	   	return 0;
	return 1;
}

and at the callsites do:

    if (pte_allows_gup(pte_val(pte, write))

    if (pte_allows_gup(pmd_val(pmd, write))

    if (pte_allows_gup(pud_val(pud, write))

Hmm?


	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/