Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386

[Toshi Kani]

+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@xxxxxxxxxxxxxxx>
> > Link: https://lkml.org/lkml/2016/4/1/608
> > Signed-off-by: Toshi Kani <toshi.kani@xxxxxxx>
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Ingo Molnar <mingo@xxxxxxxxxx>
> > H. Peter Anvin <hpa@xxxxxxxxx>
> > Borislav Petkov <bp@xxxxxxx>
> > ---
> > 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@xxxxxxx>
> 
> 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