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

[Borislav Petkov]

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()."

> When the system does not have much memory, 'high_memory' points to

What does "much memory" mean, exactly?

> 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."

> 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.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--