Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 4 Sep 2018 09:41:59 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Yang, Bin" <bin.yang@intel.com>
cc:     "mingo@kernel.org" <mingo@kernel.org>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "Gross, Mark" <mark.gross@intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "Hansen, Dave" <dave.hansen@intel.com>
Subject: Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page
 mapping
In-Reply-To: <2ad0ff43ea406005890ad4fcbb7a42da747289d9.camel@intel.com>
Message-ID: <alpine.DEB.2.21.1809040939400.1462@nanos.tec.linutronix.de>
References: <1534814186-37067-1-git-send-email-bin.yang@intel.com>  <1534814186-37067-6-git-send-email-bin.yang@intel.com>  <alpine.DEB.2.21.1809040023450.1462@nanos.tec.linutronix.de> <2ad0ff43ea406005890ad4fcbb7a42da747289d9.camel@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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