Message-ID: <1459869842.13914.39.camel@hpe.com>
Subject: Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386
From: Toshi Kani <toshi.kani@hpe.com>
To: Borislav Petkov <bp@suse.de>
Cc: mingo@kernel.org, hpa@zytor.com, tglx@linutronix.de,
        ying.huang@linux.intel.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Date: Tue, 05 Apr 2016 09:24:02 -0600
In-Reply-To: <20160405110947.GB10109@pd.tnic>
References: <1459549185-14911-1-git-send-email-toshi.kani@hpe.com>
	 <20160405110947.GB10109@pd.tnic>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3796
Lines: 110

+xen-devl

On Tue, 2016-04-05 at 13:09 +0200, Borislav Petkov wrote:
> On Fri, Apr 01, 2016 at 04:19:45PM -0600, Toshi Kani wrote:
> > 
> > The following BUG_ON error was reported on QEMU/i386:
> > 
> >   kernel BUG at arch/x86/mm/physaddr.c:79!
> >   Call Trace:
> >   phys_mem_access_prot_allowed
> >   mmap_mem
> >   ? mmap_region
> >   mmap_region
> >   do_mmap
> >   vm_mmap_pgoff
> >   SyS_mmap_pgoff
> >   do_int80_syscall_32
> >   entry_INT80_32
> > 
> > after commit edfe63ec97ed ("x86/mtrr: Fix Xorg crashes in Qemu
> > sessions").
> > 
> > PAT is now set to disabled state when MTRRs are disabled...
> "... thus reactivating the __pa(high_memory) check in
> phys_mem_access_prot_allowed()."

Will do.

> > 
> > When the system does not have much memory, 'high_memory' points to
> What does "much memory" mean, exactly?

I meant to say when a 32-bit system does not have ZONE_HIGHMEM,
__pa(high_memory) points to the maximum memory address + 1.

I will remove this sentence since it is irrelevant to this BUG_ON.  Even if
a 32-bit system does have ZONE_HIGHMEM, slow_virt_to_phys() still returns 0
for high_memory because it is set to the maximum direct mapped address + 1
in this case.  This address is not covered by page table, either.

But this made me realized that this high_memory check can be harmful in
such case, ie. __pa(high_memory) is not the maximum memory address when
ZONE_HIGHMEM is present.

I assume when this code block was originally added, legacy systems without
MTRRs did not have ZONE_HIGHMEM.  However, MTRRs are also disabled on Xen.
Reactivating this code may cause an issue on Xen 32-bit guests with
ZONE_HIGHMEM.

Question to Xen folks: Does Xen support 32-bit guests with ZONE_HIGHMEM?

If yes, a safer fix may be to remove this code block since it was deadcode
anyway...

> > the maximum memory address + 1, which is empty.  When
> > CONFIG_DEBUG_VIRTUAL is also set, __pa() calls __phys_addr(), which
> > in turn calls slow_virt_to_phys() for high_memory.  Because
> > high_memory does not point to a valid memory address, this address
> > is not mapped...
> "... and slow_virt_to_phys() returns 0."

Will do.

> > Hence, BUG_ON.
> > 
> > Use __pa_nodebug() as the code does not expect a valid virtual
> > mapping for high_memory.
> > 
> > Reported-by: kernel test robot <ying.huang@linux.intel.com>
> > Link: https://lkml.org/lkml/2016/4/1/608
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Thomas Gleixner <tglx@linutronix.de>
> > Ingo Molnar <mingo@kernel.org>
> > H. Peter Anvin <hpa@zytor.com>
> > Borislav Petkov <bp@suse.de>
> > ---
> > This patch is based on -tip.
> > ---
> >  arch/x86/mm/pat.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index c4c3ddc..26b7202 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> > @@ -792,7 +792,7 @@ int phys_mem_access_prot_allowed(struct file *file,
> > unsigned long pfn,
> >  	      boot_cpu_has(X86_FEATURE_K6_MTRR) ||
> >  	      boot_cpu_has(X86_FEATURE_CYRIX_ARR) ||
> >  	      boot_cpu_has(X86_FEATURE_CENTAUR_MCR)) &&
> > -	    (pfn << PAGE_SHIFT) >= __pa(high_memory)) {
> > +	    (pfn << PAGE_SHIFT) >= __pa_nodebug(high_memory)) {
> >  		pcm = _PAGE_CACHE_MODE_UC;
> >  	}
> >  #endif
> Modulo the minor formulations issues above,
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>
> 
> AFAIU, it makes sense to do the "nodebug" check here anyway - we
> basically only want to *check* the address and if outside of available
> memory, map UC. We shouldn't be exploding just because we're checking.
> 
> But this is just me, someone should doublecheck this train of thought
> for sanity.

Yes, let's check with Xen on this.

Thanks,
-Toshi