Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping

[Thomas Gleixner]

On Tue, 4 Sep 2018, Yang, Bin wrote:
> On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
> > >  
> > >  	psize = page_level_size(level);
> > >  	pmask = page_level_mask(level);
> > > +	addr = address & pmask;
> > >  
> > >  	/*
> > >  	 * Calculate the number of pages, which fit into this large
> > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
> > >  		cpa->numpages = numpages;
> > >  
> > >  	/*
> > > +	 * The old pgprot should not have any protection bit. Otherwise,
> > > +	 * the existing mapping is wrong already.
> > > +	 */
> > > +	WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn));
> > 
> > The check itself is fine, but it just emits a warning and goes on as if
> > nothing happened.
> > 
> > We really want to think about a proper way to fix that up without overhead
> > for the sane case.
> 
> could we change it as below? I think it should be safe to split large
> page if current mapping is wrong already.
> 
>         if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
>                 WARN_ON_ONCE(1);
>                 goto out_unlock;
>         }

Sure, but what enforces the static protections on the pages which are not
in the modified range of the current CPA call? Nothing.

And the above is also wrong because you unconditionally check even if the
existing pgprot has the PRESENT bit cleared. Guess what happens.

Thanks,

	tglx