From: Toshi Kani <toshi.kani@hpe.com>
To: Borislav Petkov <bp@suse.de>
Cc: ying.huang@linux.intel.com, x86@kernel.org,
linux-kernel@vger.kernel.org, hpa@zytor.com,
xen-devel@lists.xenproject.org, tglx@linutronix.de,
mingo@kernel.org
Subject: Re: [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386
Date: Tue, 05 Apr 2016 09:24:02 -0600 [thread overview]
Message-ID: <1459869842.13914.39.camel@hpe.com> (raw)
In-Reply-To: <20160405110947.GB10109@pd.tnic>
+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
next parent reply other threads:[~2016-04-05 15:32 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1459549185-14911-1-git-send-email-toshi.kani@hpe.com>
[not found] ` <20160405110947.GB10109@pd.tnic>
2016-04-05 15:24 ` Toshi Kani [this message]
2016-04-08 16:34 ` [PATCH] x86/mm/pat: Fix BUG_ON in mmap_mem on QEMU/i386 Toshi Kani
[not found] ` <1460133294.20338.82.camel@hpe.com>
[not found] ` <5707E392.2090106@citrix.com>
2016-04-08 16:56 ` Toshi Kani
2016-04-08 17:00 ` David Vrabel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1459869842.13914.39.camel@hpe.com \
--to=toshi.kani@hpe.com \
--cc=bp@suse.de \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=xen-devel@lists.xenproject.org \
--cc=ying.huang@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).