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 <[email protected]>
Link: https://lkml.org/lkml/2016/4/1/608
Signed-off-by: Toshi Kani <[email protected]>
Thomas Gleixner <[email protected]>
Ingo Molnar <[email protected]>
H. Peter Anvin <[email protected]>
Borislav Petkov <[email protected]>
---
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
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 <[email protected]>
> Link: https://lkml.org/lkml/2016/4/1/608
> Signed-off-by: Toshi Kani <[email protected]>
> Thomas Gleixner <[email protected]>
> Ingo Molnar <[email protected]>
> H. Peter Anvin <[email protected]>
> Borislav Petkov <[email protected]>
> ---
> 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 <[email protected]>
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)
--
+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 <[email protected]>
> > Link: https://lkml.org/lkml/2016/4/1/608
> > Signed-off-by: Toshi Kani <[email protected]>
> > Thomas Gleixner <[email protected]>
> > Ingo Molnar <[email protected]>
> > H. Peter Anvin <[email protected]>
> > Borislav Petkov <[email protected]>
> > ---
> > 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 <[email protected]>
>
> 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
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
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
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