* [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
@ 2008-10-01 10:47 Vegard Nossum
2008-10-01 11:06 ` Jiri Slaby
0 siblings, 1 reply; 11+ messages in thread
From: Vegard Nossum @ 2008-10-01 10:47 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Jiri Slaby, Andi Kleen, linux-kernel
Hi,
Fix for tip/x86/mm-debug (commit 59ea746337c69f6a5f1bc4d5e8544b3cbf12f801).
I'm not sure if choice of names/structure is entirely correct, comments are
appreciated.
Vegard
>From 01613a1949de51c7ab9d0acaaa9a5444722a5cfa Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegardno@ben.ifi.uio.no>
Date: Wed, 1 Oct 2008 12:36:34 +0200
Subject: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
virt_addr_valid() calls __pa(), which calls __phys_addr(). With
CONFIG_DEBUG_VIRTUAL=y, __phys_addr() will kill the kernel if the
address *isn't* valid. That's clearly wrong for virt_addr_valid().
Cc: Jiri Slaby <jirislaby@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Vegard Nossum <vegardno@ben.ifi.uio.no>
---
arch/x86/kernel/doublefault_32.c | 2 +-
arch/x86/mm/ioremap.c | 2 +-
include/asm-x86/page.h | 3 ++-
include/asm-x86/page_32.h | 4 ++--
include/asm-x86/page_64.h | 1 +
5 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/doublefault_32.c b/arch/x86/kernel/doublefault_32.c
index 395acb1..1580e0b 100644
--- a/arch/x86/kernel/doublefault_32.c
+++ b/arch/x86/kernel/doublefault_32.c
@@ -66,6 +66,6 @@ struct tss_struct doublefault_tss __cacheline_aligned = {
.ds = __USER_DS,
.fs = __KERNEL_PERCPU,
- .__cr3 = __phys_addr_const((unsigned long)swapper_pg_dir)
+ .__cr3 = __phys_addr_nodebug((unsigned long)swapper_pg_dir)
}
};
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 835e062..4d49e88 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -58,7 +58,7 @@ unsigned long __phys_addr(unsigned long x)
/* VMALLOC_* aren't constants; not available at the boot time */
VIRTUAL_BUG_ON(x < PAGE_OFFSET || (system_state != SYSTEM_BOOTING &&
is_vmalloc_addr((void *)x)));
- return x - PAGE_OFFSET;
+ return __phys_addr_nodebug(x);
}
EXPORT_SYMBOL(__phys_addr);
#endif
diff --git a/include/asm-x86/page.h b/include/asm-x86/page.h
index c915747..9b70b41 100644
--- a/include/asm-x86/page.h
+++ b/include/asm-x86/page.h
@@ -179,6 +179,7 @@ static inline pteval_t native_pte_flags(pte_t pte)
#endif /* CONFIG_PARAVIRT */
#define __pa(x) __phys_addr((unsigned long)(x))
+#define __pa_nodebug(x) __phys_addr_nodebug((unsigned long)(x))
/* __pa_symbol should be used for C visible symbols.
This seems to be the official gcc blessed way to do such arithmetic. */
#define __pa_symbol(x) __pa(__phys_reloc_hide((unsigned long)(x)))
@@ -190,7 +191,7 @@ static inline pteval_t native_pte_flags(pte_t pte)
#define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
-#define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
+#define virt_addr_valid(kaddr) pfn_valid(__pa_nodebug(kaddr) >> PAGE_SHIFT)
#endif /* __ASSEMBLY__ */
diff --git a/include/asm-x86/page_32.h b/include/asm-x86/page_32.h
index 82b0109..deec501 100644
--- a/include/asm-x86/page_32.h
+++ b/include/asm-x86/page_32.h
@@ -71,11 +71,11 @@ typedef struct page *pgtable_t;
#endif
#ifndef __ASSEMBLY__
-#define __phys_addr_const(x) ((x) - PAGE_OFFSET)
+#define __phys_addr_nodebug(x) ((x) - PAGE_OFFSET)
#ifdef CONFIG_DEBUG_VIRTUAL
extern unsigned long __phys_addr(unsigned long);
#else
-#define __phys_addr(x) ((x) - PAGE_OFFSET)
+#define __phys_addr(x) __phys_addr_nodebug(x)
#endif
#define __phys_reloc_hide(x) RELOC_HIDE((x), 0)
diff --git a/include/asm-x86/page_64.h b/include/asm-x86/page_64.h
index 49380b8..8697b6f 100644
--- a/include/asm-x86/page_64.h
+++ b/include/asm-x86/page_64.h
@@ -69,6 +69,7 @@ extern unsigned long max_pfn;
extern unsigned long phys_base;
extern unsigned long __phys_addr(unsigned long);
+#define __phys_addr_nodebug(x) __phys_addr(x)
#define __phys_reloc_hide(x) (x)
/*
--
1.5.6.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 10:47 [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y Vegard Nossum
@ 2008-10-01 11:06 ` Jiri Slaby
2008-10-01 11:07 ` Jiri Slaby
2008-10-01 11:15 ` Vegard Nossum
0 siblings, 2 replies; 11+ messages in thread
From: Jiri Slaby @ 2008-10-01 11:06 UTC (permalink / raw)
To: Vegard Nossum; +Cc: Ingo Molnar, Andi Kleen, linux-kernel
On 10/01/2008 12:47 PM, Vegard Nossum wrote:
> Hi,
>
> Fix for tip/x86/mm-debug (commit 59ea746337c69f6a5f1bc4d5e8544b3cbf12f801).
> I'm not sure if choice of names/structure is entirely correct, comments are
> appreciated.
> From 01613a1949de51c7ab9d0acaaa9a5444722a5cfa Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegardno@ben.ifi.uio.no>
> Date: Wed, 1 Oct 2008 12:36:34 +0200
> Subject: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
>
> virt_addr_valid() calls __pa(), which calls __phys_addr(). With
> CONFIG_DEBUG_VIRTUAL=y, __phys_addr() will kill the kernel if the
> address *isn't* valid. That's clearly wrong for virt_addr_valid().
>
> Cc: Jiri Slaby <jirislaby@gmail.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Signed-off-by: Vegard Nossum <vegardno@ben.ifi.uio.no>
> ---
> diff --git a/arch/x86/kernel/doublefault_32.c b/arch/x86/kernel/doublefault_32.c
> index 395acb1..1580e0b 100644
> --- a/arch/x86/kernel/doublefault_32.c
> +++ b/arch/x86/kernel/doublefault_32.c
> @@ -66,6 +66,6 @@ struct tss_struct doublefault_tss __cacheline_aligned = {
> .ds = __USER_DS,
> .fs = __KERNEL_PERCPU,
>
> - .__cr3 = __phys_addr_const((unsigned long)swapper_pg_dir)
> + .__cr3 = __phys_addr_nodebug((unsigned long)swapper_pg_dir)
Now, this may be switched back to __pa: __pa_nodebug(swapper_pg_dir);
> }
> };
[...]
> --- a/include/asm-x86/page_64.h
> +++ b/include/asm-x86/page_64.h
> @@ -69,6 +69,7 @@ extern unsigned long max_pfn;
> extern unsigned long phys_base;
>
> extern unsigned long __phys_addr(unsigned long);
> +#define __phys_addr_nodebug(x) __phys_addr(x)
> #define __phys_reloc_hide(x) (x)
x86_64 is screwed in the same way, isn't it?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 11:06 ` Jiri Slaby
@ 2008-10-01 11:07 ` Jiri Slaby
2008-10-01 11:15 ` Vegard Nossum
1 sibling, 0 replies; 11+ messages in thread
From: Jiri Slaby @ 2008-10-01 11:07 UTC (permalink / raw)
To: Vegard Nossum; +Cc: Ingo Molnar, Andi Kleen, linux-kernel
Ah, and thanks for spotting this!
On 10/01/2008 01:06 PM, Jiri Slaby wrote:
> On 10/01/2008 12:47 PM, Vegard Nossum wrote:
>> Subject: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 11:06 ` Jiri Slaby
2008-10-01 11:07 ` Jiri Slaby
@ 2008-10-01 11:15 ` Vegard Nossum
2008-10-01 11:26 ` Jiri Slaby
1 sibling, 1 reply; 11+ messages in thread
From: Vegard Nossum @ 2008-10-01 11:15 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Vegard Nossum, Ingo Molnar, Andi Kleen, linux-kernel
On Wed, Oct 1, 2008 at 1:06 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 10/01/2008 12:47 PM, Vegard Nossum wrote:
>> Hi,
>>
>> Fix for tip/x86/mm-debug (commit 59ea746337c69f6a5f1bc4d5e8544b3cbf12f801).
>> I'm not sure if choice of names/structure is entirely correct, comments are
>> appreciated.
>
>
>> From 01613a1949de51c7ab9d0acaaa9a5444722a5cfa Mon Sep 17 00:00:00 2001
>> From: Vegard Nossum <vegardno@ben.ifi.uio.no>
>> Date: Wed, 1 Oct 2008 12:36:34 +0200
>> Subject: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
>>
>> virt_addr_valid() calls __pa(), which calls __phys_addr(). With
>> CONFIG_DEBUG_VIRTUAL=y, __phys_addr() will kill the kernel if the
>> address *isn't* valid. That's clearly wrong for virt_addr_valid().
>>
...
>> --- a/include/asm-x86/page_64.h
>> +++ b/include/asm-x86/page_64.h
>> @@ -69,6 +69,7 @@ extern unsigned long max_pfn;
>> extern unsigned long phys_base;
>>
>> extern unsigned long __phys_addr(unsigned long);
>> +#define __phys_addr_nodebug(x) __phys_addr(x)
>> #define __phys_reloc_hide(x) (x)
>
> x86_64 is screwed in the same way, isn't it?
Hm. I didn't see any #ifdef CONFIG_DEBUG_VIRTUAL in the x86_64 code,
so I assumed it wasn't. But it seems that you are right (because the
checks, or at least some kind of checks, are _always_ performed on
x86_64 regardless of the CONFIG_DEBUG_VIRTUAL setting). Why doesn't
the checking in x86_64 code depend on DEBUG_VIRTUAL?
I will try to make another patch, thanks.
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 11:15 ` Vegard Nossum
@ 2008-10-01 11:26 ` Jiri Slaby
2008-10-01 16:42 ` Vegard Nossum
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2008-10-01 11:26 UTC (permalink / raw)
To: Vegard Nossum; +Cc: Vegard Nossum, Ingo Molnar, Andi Kleen, linux-kernel
On 10/01/2008 01:15 PM, Vegard Nossum wrote:
> On Wed, Oct 1, 2008 at 1:06 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> x86_64 is screwed in the same way, isn't it?
>
> Hm. I didn't see any #ifdef CONFIG_DEBUG_VIRTUAL in the x86_64 code,
> so I assumed it wasn't. But it seems that you are right (because the
> checks, or at least some kind of checks, are _always_ performed on
> x86_64 regardless of the CONFIG_DEBUG_VIRTUAL setting). Why doesn't
> the checking in x86_64 code depend on DEBUG_VIRTUAL?
Yeah, it does: VIRTUAL_BUG_ON depends on it...
x86_64 just distinguish pointer to kernel image addresses (which are mapped only
up to kernel image size from phys_base physical address) and whole physical
memory map at another virtual address.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 11:26 ` Jiri Slaby
@ 2008-10-01 16:42 ` Vegard Nossum
2008-10-01 16:52 ` Andi Kleen
0 siblings, 1 reply; 11+ messages in thread
From: Vegard Nossum @ 2008-10-01 16:42 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Vegard Nossum, Ingo Molnar, Andi Kleen, linux-kernel
On Wed, Oct 1, 2008 at 1:26 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 10/01/2008 01:15 PM, Vegard Nossum wrote:
>> On Wed, Oct 1, 2008 at 1:06 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>>> x86_64 is screwed in the same way, isn't it?
>>
>> Hm. I didn't see any #ifdef CONFIG_DEBUG_VIRTUAL in the x86_64 code,
>> so I assumed it wasn't. But it seems that you are right (because the
>> checks, or at least some kind of checks, are _always_ performed on
>> x86_64 regardless of the CONFIG_DEBUG_VIRTUAL setting). Why doesn't
>> the checking in x86_64 code depend on DEBUG_VIRTUAL?
>
> Yeah, it does: VIRTUAL_BUG_ON depends on it...
>
> x86_64 just distinguish pointer to kernel image addresses (which are mapped only
> up to kernel image size from phys_base physical address) and whole physical
> memory map at another virtual address.
You are right.
But it seems that the current virt_addr_valid() doesn't take this into
account. Should virt_addr_valid() be modified (on both x86_32 and
x86_64) to take into account the same checks as __phys_addr() does
when DEBUG_VIRTUAL=y? Or is it enough to use pfn_valid()?
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 16:42 ` Vegard Nossum
@ 2008-10-01 16:52 ` Andi Kleen
2008-10-01 19:32 ` Jiri Slaby
0 siblings, 1 reply; 11+ messages in thread
From: Andi Kleen @ 2008-10-01 16:52 UTC (permalink / raw)
To: Vegard Nossum
Cc: Jiri Slaby, Vegard Nossum, Ingo Molnar, Andi Kleen, linux-kernel
> But it seems that the current virt_addr_valid() doesn't take this into
> account. Should virt_addr_valid() be modified (on both x86_32 and
> x86_64) to take into account the same checks as __phys_addr() does
> when DEBUG_VIRTUAL=y? Or is it enough to use pfn_valid()?
At least in Linus' tree virt_addr_valid() is just a wrapper
around pfn_valid()
-Andi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 16:52 ` Andi Kleen
@ 2008-10-01 19:32 ` Jiri Slaby
2008-10-01 19:46 ` Andi Kleen
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2008-10-01 19:32 UTC (permalink / raw)
To: Andi Kleen; +Cc: Vegard Nossum, Vegard Nossum, Ingo Molnar, linux-kernel
On 10/01/2008 06:52 PM, Andi Kleen wrote:
>> But it seems that the current virt_addr_valid() doesn't take this into
>> account. Should virt_addr_valid() be modified (on both x86_32 and
>> x86_64) to take into account the same checks as __phys_addr() does
>> when DEBUG_VIRTUAL=y? Or is it enough to use pfn_valid()?
>
> At least in Linus' tree virt_addr_valid() is just a wrapper
> around pfn_valid()
Yes, but __pa() used for converting to a physical address used as a parameter
for __pfn_valid() will panic on invalid addresses passed to it when DEBUG_VIRTUAL=y.
Anyway virt_addr_valid() is IMHO wrong. E.g. first modules VM address
0xffffffffa0000000 is after __pa() 200M which is valid pfn after the shift even
on the flatmem model with enough memory.
Am I missing something? What's the exact purpose of the virt_addr_valid()?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 19:32 ` Jiri Slaby
@ 2008-10-01 19:46 ` Andi Kleen
2008-10-01 20:01 ` Jiri Slaby
0 siblings, 1 reply; 11+ messages in thread
From: Andi Kleen @ 2008-10-01 19:46 UTC (permalink / raw)
To: Jiri Slaby
Cc: Andi Kleen, Vegard Nossum, Vegard Nossum, Ingo Molnar, linux-kernel
> Yes, but __pa() used for converting to a physical address used as a parameter
> for __pfn_valid() will panic on invalid addresses passed to it when DEBUG_VIRTUAL=y.
Ok that's a bug indeed.
> Anyway virt_addr_valid() is IMHO wrong. E.g. first modules VM address
> 0xffffffffa0000000 is after __pa() 200M which is valid pfn after the shift even
> on the flatmem model with enough memory.
>
> Am I missing something? What's the exact purpose of the virt_addr_valid()?
I think it's supposed to be only used on direct mapping anyways (judging
from a quick look a the users)
So not handling text mapping is ok, but don't panic on it.
-Andi
--
ak@linux.intel.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 19:46 ` Andi Kleen
@ 2008-10-01 20:01 ` Jiri Slaby
2008-10-02 6:18 ` Vegard Nossum
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2008-10-01 20:01 UTC (permalink / raw)
To: Andi Kleen; +Cc: Vegard Nossum, Vegard Nossum, Ingo Molnar, linux-kernel
On 10/01/2008 09:46 PM, Andi Kleen wrote:
>> Anyway virt_addr_valid() is IMHO wrong. E.g. first modules VM address
>> 0xffffffffa0000000 is after __pa() 200M which is valid pfn after the shift even
>> on the flatmem model with enough memory.
>>
>> Am I missing something? What's the exact purpose of the virt_addr_valid()?
>
> I think it's supposed to be only used on direct mapping anyways (judging
> from a quick look a the users)
Then kmemcheck assumes something else. Citing:
* We need to be extremely careful not to follow any invalid pointers,
* because this function can be called for *any* possible address.
and the very first check is !virt_addr_valid(address).
> So not handling text mapping is ok, but don't panic on it.
It doesn't handle properly anything but text and direct mapping. Now it
oopses/causes BUG on that wrong cases.
I think we should set it down there that it was intended to be used only on
text/direct mapping and only for checking if there is a physical memory page
behind this kind of virtual address.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y
2008-10-01 20:01 ` Jiri Slaby
@ 2008-10-02 6:18 ` Vegard Nossum
0 siblings, 0 replies; 11+ messages in thread
From: Vegard Nossum @ 2008-10-02 6:18 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Andi Kleen, Vegard Nossum, Ingo Molnar, linux-kernel
On Wed, Oct 1, 2008 at 10:01 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 10/01/2008 09:46 PM, Andi Kleen wrote:
>>> Anyway virt_addr_valid() is IMHO wrong. E.g. first modules VM address
>>> 0xffffffffa0000000 is after __pa() 200M which is valid pfn after the shift even
>>> on the flatmem model with enough memory.
>>>
>>> Am I missing something? What's the exact purpose of the virt_addr_valid()?
>>
>> I think it's supposed to be only used on direct mapping anyways (judging
>> from a quick look a the users)
>
> Then kmemcheck assumes something else. Citing:
> * We need to be extremely careful not to follow any invalid pointers,
> * because this function can be called for *any* possible address.
> and the very first check is !virt_addr_valid(address).
The purpose of this call was to make sure that the page behind the
virtual address has an associated struct page. That is the assumption:
virt_to_page() will return something meaningful if and only if
virt_addr_valid().
>
>> So not handling text mapping is ok, but don't panic on it.
>
> It doesn't handle properly anything but text and direct mapping. Now it
> oopses/causes BUG on that wrong cases.
>
> I think we should set it down there that it was intended to be used only on
> text/direct mapping and only for checking if there is a physical memory page
> behind this kind of virtual address.
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2008-10-02 6:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-01 10:47 [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y Vegard Nossum
2008-10-01 11:06 ` Jiri Slaby
2008-10-01 11:07 ` Jiri Slaby
2008-10-01 11:15 ` Vegard Nossum
2008-10-01 11:26 ` Jiri Slaby
2008-10-01 16:42 ` Vegard Nossum
2008-10-01 16:52 ` Andi Kleen
2008-10-01 19:32 ` Jiri Slaby
2008-10-01 19:46 ` Andi Kleen
2008-10-01 20:01 ` Jiri Slaby
2008-10-02 6:18 ` Vegard Nossum
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).