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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel