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
>
