Re: [PATCH v3 3/5] x86/mm: add help function to check specific protection flags in range

[Thomas Gleixner]

On Tue, 21 Aug 2018, Bin Yang wrote:
>  /*
> + * static_protections() "forces" page protections for some address
> + * ranges.  Return true if any part of the address/len range is forced
> + * to change from 'prot'.
> + */
> +static inline bool
> +needs_static_protections(pgprot_t prot, unsigned long address,
> +		unsigned long len, unsigned long pfn)
> +{
> +	int i;
> +
> +	address &= PAGE_MASK;
> +	len = PFN_ALIGN(len);
> +	for (i = 0; i < (len >> PAGE_SHIFT); i++, address += PAGE_SIZE, pfn++) {
> +		pgprot_t chk_prot = static_protections(prot, address, pfn);
> +
> +		if (pgprot_val(chk_prot) != pgprot_val(prot))
> +			return true;
> +	}
> +
> +	/* Does static_protections() demand a change ? */
> +	return false;
> +}

...

>  	if (cpa->force_split)
> @@ -660,14 +684,8 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
>  	 * static_protection() requires a different pgprot for one of
>  	 * the pages in the range we try to preserve:
>  	 */
> -	pfn = old_pfn;
> -	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr += PAGE_SIZE, pfn++) {
> -		pgprot_t chk_prot = static_protections(req_prot, addr, pfn);
> -
> -		if (pgprot_val(chk_prot) != pgprot_val(new_prot))
> -			goto out_unlock;
> -	}
> -
> +	if (needs_static_protections(new_prot, addr, psize, old_pfn))
> +		goto out_unlock;

This is not the same. The existing code does:

     new_prot = static_protections(req_prot, address, pfn);

which is the protection updated pgprot for the base of the address range
which should be modified. The loop does:

    chk_prot = static_protections(req_prot, addr, pfn);

    if (chk_prot != new_prot)
    	   goto split;

Now mapping your new function back and then the loop becomes:

    chk_prot = static_protections(new_prot, addr, pfn);

    if (chk_prot != new_prot)
    	   goto split;

which is broken in case that after the initial static protections
invocation

	new_prot = static_protections(req_prot, address, pfn);

the result is:

   new_prot != req_prot

and in the loop

   new_prot is valid for _ALL_ pages in the large page because the static
   protection which got applied for the first address can be applied to the
   complete range, i.e. new_prot it is not further modified by
   static_protections() for any page.

That again can cause wrong large page preservations.

Thanks,

	tglx