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
