Messages in this thread | | | From | "Yang, Bin" <> | Subject | Re: [PATCH v3 3/5] x86/mm: add help function to check specific protection flags in range | Date | Tue, 4 Sep 2018 06:22:09 +0000 |
| |
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 >
| |