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

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

[Toshi Kani]

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.  This
led the __pa(high_memory) check in phys_mem_access_prot_allowed()
to resurrect on x86/32.

When the system does not have much memory, 'high_memory' points to
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.  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

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

Thanks.

-- 
Regards/Gruss,
    Boris.

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

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

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

[Toshi Kani]

On Tue, 2016-04-05 at 09:24 -0600, Toshi Kani wrote:
> +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:
> > > 
 :
> > > 
> > > 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...

I have not heard a confirmation from Xen folks, but I believe ZONE_HIGHMEM
is supported on 32-bit Xen guests.  So, unless someone has an objection, I
am going to remove this code block in the next version of this patch.

Thanks,
-Toshi

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

[David Vrabel]

On 08/04/16 17:34, Toshi Kani wrote:
> On Tue, 2016-04-05 at 09:24 -0600, Toshi Kani wrote:
>> +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:
>>>>
>  :
>>>>
>>>> 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...
> 
> I have not heard a confirmation from Xen folks, but I believe ZONE_HIGHMEM
> is supported on 32-bit Xen guests.  So, unless someone has an objection, I
> am going to remove this code block in the next version of this patch.

32-bit Xen guests have highmem, yes.

David

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

[Toshi Kani]

On Fri, 2016-04-08 at 18:00 +0100, David Vrabel wrote:
> On 08/04/16 17:34, Toshi Kani wrote:
> > 
> > On Tue, 2016-04-05 at 09:24 -0600, Toshi Kani wrote:
> > > 
> > > +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:
> > > > > 
> >  :
> > > > > 
> > > > > 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...
> >
> > I have not heard a confirmation from Xen folks, but I believe
> > ZONE_HIGHMEM is supported on 32-bit Xen guests.  So, unless someone has
> > an objection, I am going to remove this code block in the next version
> > of this patch.
>
> 32-bit Xen guests have highmem, yes.

Thanks David!
-Toshi