Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371393 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386 Borislav Petkov <bp@suse.de> - 2016-04-05 13:10 +0200
    Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386 Toshi Kani <toshi.kani@hpe.com> - 2016-04-05 17:40 +0200
      Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386 Toshi Kani <toshi.kani@hpe.com> - 2016-04-08 18:50 +0200
        Re: [Xen-devel] [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on  QEMU/i386 David Vrabel <david.vrabel@citrix.com> - 2016-04-08 19:10 +0200
          Re: [Xen-devel] [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on  QEMU/i386 Toshi Kani <toshi.kani@hpe.com> - 2016-04-08 19:10 +0200

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

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

[toc] | [next] | [standalone]

#1371726

+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

[toc] | [prev] | [next] | [standalone]

#1374359

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web