[PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Vegard Nossum]

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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Jiri Slaby]

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?

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Jiri Slaby]

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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Vegard Nossum]

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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Jiri Slaby]

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.

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Vegard Nossum]

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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Andi Kleen]

> 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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Jiri Slaby]

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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Andi Kleen]

> 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

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Jiri Slaby]

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.

Re: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y

[Vegard Nossum]

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