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

["Yang, Bin" <bin.yang@intel.com>]

On Tue, 2018-09-04 at 00:10 +0200, Thomas Gleixner wrote:
> 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.

Sorry for this mistake. Could I change it as below?

static inline bool
needs_static_protections(pgprot_t new_prot, pgprot_t req_prot,
                unsigned long address, unsigned long len, unsigned long pfn)
...
                pgprot_t chk_prot = static_protections(req_prot, address, pfn);

                if (pgprot_val(chk_prot) != pgprot_val(new_prot))
...

> 
> Thanks,
> 
> 	tglx
>