x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y (try 2)

PATCH In review Vegard Nossum 3 participants 5 messages Oct 3, 2008 – Oct 4, 2008

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Vegard Nossum

author

2008-10-03 14:07 UTC #0 lore raw

PATCH 1/1 x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y (try 2)

How about this? It would apply to tip/x86/mm-debug. Untested.

Vegard

Quoted 1 line

From 27f365e4a7b3e76e5f01af1056c47ca1fcf226d8 Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Fri, 3 Oct 2008 15:58:50 +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 <[email protected]>

Cc Andi Kleen <[email protected]>

Signed-off-by Vegard Nossum <[email protected]>

---
 arch/x86/kernel/doublefault_32.c |    2 +-
 arch/x86/mm/ioremap.c            |   12 ++++++++++++
 include/asm-x86/page.h           |    3 ++-
 include/asm-x86/page_32.h        |    4 ++--
 include/asm-x86/page_64.h        |    1 +
 5 files changed, 18 insertions(+), 4 deletions(-)

arch/x86/kernel/doublefault_32.c +1 -1

index 395acb1..b4f14c6 100644
@@ -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		= __pa_nodebug(swapper_pg_dir),
 	}
 };

arch/x86/mm/ioremap.c +12 -0

index 835e062..2d5e6de 100644
@@ -29,6 +29,18 @@ static inline int phys_addr_valid(unsigned long addr)
 	return addr < (1UL << boot_cpu_data.x86_phys_bits);
 }
 
+unsigned long __phys_addr_nodebug(unsigned long x)
+{
+	if (x >= __START_KERNEL_map) {
+		x -= __START_KERNEL_map;
+		x += phys_base;
+	} else {
+		x -= PAGE_OFFSET;
+	}
+
+	return x;
+}
+
 unsigned long __phys_addr(unsigned long x)
 {
 	if (x >= __START_KERNEL_map) {

include/asm-x86/page.h +2 -1

index c915747..9b70b41 100644
@@ -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__ */

include/asm-x86/page_32.h +2 -2

index 82b0109..deec501 100644
@@ -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)

include/asm-x86/page_64.h +1 -0

index 49380b8..d2a851e 100644
@@ -68,6 +68,7 @@ void copy_page(void *to, void *from);
 extern unsigned long max_pfn;
 extern unsigned long phys_base;
 
+extern unsigned long __phys_addr_nodebug(unsigned long);
 extern unsigned long __phys_addr(unsigned long);
 #define __phys_reloc_hide(x)	(x)

Signature · 1 line

1.5.5.1

Jiri Slaby

2008-10-03 14:24 UTC #1 lore raw

Vegard Nossum napsal(a):

Quoted 1 line

How about this? It would apply to tip/x86/mm-debug. Untested.

It is wrong in my eyes -- it's workarounding of the real problem rather than fixing. virt_addr_valid() should be fixed/augmented to the full virtual address domain instead or not used for the purpose you need in kmemcheck.

As it is now, it should be used only on direct mapping addresses (more
concrete: kernel image space and physical mapping) and it returns semi-random values on the rest -- with CONFIG_DEBUG_VIRTUAL=y it screams instead of returning a bogus value.

Vegard Nossum

author

2008-10-03 15:54 UTC #2 lore raw

Quoted 12 lines · Jiri Slaby

Vegard Nossum napsal(a):
Quoted 1 line
How about this? It would apply to tip/x86/mm-debug. Untested.
It is wrong in my eyes -- it's workarounding of the real problem rather than fixing. virt_addr_valid() should be fixed/augmented to the full virtual address domain instead or not used for the purpose you need in kmemcheck.
As it is now, it should be used only on direct mapping addresses (more
concrete: kernel image space and physical mapping) and it returns semi-random values on the rest -- with CONFIG_DEBUG_VIRTUAL=y it screams instead of returning a bogus value.

Is this what you had in mind? If not, then I don't know how to do it :-)

It seems that it might be structured in a better way -- but that better way eludes my imagination. Thanks for commenting!

Vegard

Quoted 1 line

From 320c68f94e1a2071c64ecc9db6358801bd0c1f0b Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Fri, 3 Oct 2008 15:58:50 +0200
Subject: [PATCH] x86: fix virt_addr_valid()

We also incorporate the debugging checks into virt_addr_valid().

Cc Jiri Slaby <[email protected]>

Cc Andi Kleen <[email protected]>

Signed-off-by Vegard Nossum <[email protected]>

---
 arch/x86/kernel/doublefault_32.c |    2 +-
 arch/x86/mm/ioremap.c            |   37 +++++++++++++++++++++++++++++++++++--
 include/asm-x86/page.h           |    8 +++++++-
 include/asm-x86/page_32.h        |    4 ++--
 4 files changed, 45 insertions(+), 6 deletions(-)

arch/x86/kernel/doublefault_32.c +1 -1

index 395acb1..b4f14c6 100644
@@ -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		= __pa_nodebug(swapper_pg_dir),
 	}
 };

arch/x86/mm/ioremap.c +35 -2

index 835e062..b20389d 100644
@@ -45,6 +45,28 @@ unsigned long __phys_addr(unsigned long x)
 }
 EXPORT_SYMBOL(__phys_addr);
 
+bool __virt_addr_valid(unsigned long x)
+{
+	if (x >= __START_KERNEL_map) {
+		x -= __START_KERNEL_map;
+		if (x >= KERNEL_IMAGE_SIZE)
+			return false;
+		x += phys_base;
+	} else {
+		if (x < PAGE_OFFSET)
+			return false;
+		x -= PAGE_OFFSET;
+		if (system_state == SYSTEM_BOOTING
+			? x > MAXMEM : !phys_addr_valid(x))
+		{
+			return false;
+		}
+	}
+
+	return pfn_valid(x >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL(__virt_addr_valid);
+
 #else
 
 static inline int phys_addr_valid(unsigned long addr)

@@ -56,13 +78,24 @@ static inline int phys_addr_valid(unsigned long addr)
 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)));
+	VIRTUAL_BUG_ON(x < PAGE_OFFSET);
+	VIRTUAL_BUG_ON(system_state != SYSTEM_BOOTING &&
+		is_vmalloc_addr((void *) x));
 	return x - PAGE_OFFSET;
 }
 EXPORT_SYMBOL(__phys_addr);
 #endif
 
+bool __virt_addr_valid(unsigned long x)
+{
+	if (x < PAGE_OFFSET)
+		return false;
+	if (system_state != SYSTEM_BOOTING && is_vmalloc_addr((void *) x))
+		return false;
+	return pfn_valid((x - PAGE_OFFSET) >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL(__virt_addr_valid);
+
 #endif
 
 int page_is_ram(unsigned long pagenr)

include/asm-x86/page.h +7 -1

index c915747..d4f1d57 100644
@@ -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)))

@@ -188,9 +189,14 @@ static inline pteval_t native_pte_flags(pte_t pte)
 #define __boot_va(x)		__va(x)
 #define __boot_pa(x)		__pa(x)
 
+/*
+ * virt_to_page(kaddr) returns a valid pointer if and only if
+ * virt_addr_valid(kaddr) returns true.
+ */
 #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)
+extern bool __virt_addr_valid(unsigned long kaddr);
+#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
 
 #endif	/* __ASSEMBLY__ */

include/asm-x86/page_32.h +2 -2

index 82b0109..deec501 100644
@@ -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)

Signature · 1 line

1.5.5.1

Jiri Slaby

2008-10-03 18:12 UTC #3 lore raw

Vegard Nossum napsal(a):

Quoted 1 line

Is this what you had in mind? If not, then I don't know how to do it :-)

Yeah, something like that, but I am not the one you need ACK from ;).

Few notes below.

Quoted 19 lines

arch/x86/mm/ioremap.c +11 -0

index 835e062..b20389d 100644
@@ -45,6 +45,28 @@ unsigned long __phys_addr(unsigned long x)
 }
 EXPORT_SYMBOL(__phys_addr);
 
+bool __virt_addr_valid(unsigned long x)
+{
+	if (x >= __START_KERNEL_map) {
+		x -= __START_KERNEL_map;
+		if (x >= KERNEL_IMAGE_SIZE)
+			return false;
+		x += phys_base;
+	} else {
+		if (x < PAGE_OFFSET)
+			return false;
+		x -= PAGE_OFFSET;

arch/x86/mm/ioremap.c unsigned long __phys_addr(unsigned long x) · line 68

Quoted 10 lines

arch/x86/mm/ioremap.c +10 -0

+		if (system_state == SYSTEM_BOOTING
+			? x > MAXMEM : !phys_addr_valid(x))
+		{
+			return false;
+		}
+	}
+
+	return pfn_valid(x >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL(__virt_addr_valid);

Just an implementation note. Still don't expect address for which __virt_addr_valid() returns true to not cause a page fault. If it is called e.g. for address between __START_KERNEL_map and __START_KERNEL_map+KERNEL_IMAGE_SIZE and its pte entry has present bit unset, it will crash when dereferenced -- unused pages above kernel image are unmapped during boot.

Quoted 2 lines

arch/x86/mm/ioremap.c +1 -0
+
 #else

[...]

arch/x86/mm/ioremap.c static inline int phys_addr_valid(unsigned long addr) · line 96

Quoted 8 lines

arch/x86/mm/ioremap.c +8 -0

+bool __virt_addr_valid(unsigned long x)
+{
+	if (x < PAGE_OFFSET)
+		return false;
+	if (system_state != SYSTEM_BOOTING && is_vmalloc_addr((void *) x))
+		return false;
+	return pfn_valid((x - PAGE_OFFSET) >> PAGE_SHIFT);
+}

Here too, the mappings at the top are probably returned as true? Absolutely not sure about this (what returns pfn_valid on those corresponding physical addresses).

Quoted 3 lines

#endif
int page_is_ram(unsigned long pagenr)

Ingo Molnar

Acked-by Jiri Slaby

2008-10-04 08:11 UTC #4 lore raw

Quoted 4 lines · Jiri Slaby

Vegard Nossum napsal(a):
Quoted 1 line
Is this what you had in mind? If not, then I don't know how to do it :-)
Yeah, something like that, but I am not the one you need ACK from ;).

ok - i've applied the commit below to tip/x86/core to get some testing of it.

Ingo

------------>

Quoted 1 line

From fe45d9c75945698006a608a8fcccf39d161b0669 Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Fri, 3 Oct 2008 17:54:25 +0200
Subject: [PATCH] x86: fix virt_addr_valid() with CONFIG_DEBUG_VIRTUAL=y, v2

We also incorporate the debugging checks into virt_addr_valid().

Signed-off-by Vegard Nossum <[email protected]>

Acked-by Jiri Slaby <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/doublefault_32.c |    2 +-
 arch/x86/mm/ioremap.c            |   36 ++++++++++++++++++++++++++++++++++--
 include/asm-x86/page.h           |    8 +++++++-
 include/asm-x86/page_32.h        |    4 ++--
 4 files changed, 44 insertions(+), 6 deletions(-)

arch/x86/kernel/doublefault_32.c +1 -1

index 395acb1..b4f14c6 100644
@@ -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		= __pa_nodebug(swapper_pg_dir),
 	}
 };

arch/x86/mm/ioremap.c +34 -2

index 7955a5a..e6e57fe 100644
@@ -45,6 +45,27 @@ unsigned long __phys_addr(unsigned long x)
 }
 EXPORT_SYMBOL(__phys_addr);
 
+bool __virt_addr_valid(unsigned long x)
+{
+	if (x >= __START_KERNEL_map) {
+		x -= __START_KERNEL_map;
+		if (x >= KERNEL_IMAGE_SIZE)
+			return false;
+		x += phys_base;
+	} else {
+		if (x < PAGE_OFFSET)
+			return false;
+		x -= PAGE_OFFSET;
+		if (system_state == SYSTEM_BOOTING ?
+				x > MAXMEM : !phys_addr_valid(x)) {
+			return false;
+		}
+	}
+
+	return pfn_valid(x >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL(__virt_addr_valid);
+
 #else
 
 static inline int phys_addr_valid(unsigned long addr)

@@ -56,13 +77,24 @@ static inline int phys_addr_valid(unsigned long addr)
 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)));
+	VIRTUAL_BUG_ON(x < PAGE_OFFSET);
+	VIRTUAL_BUG_ON(system_state != SYSTEM_BOOTING &&
+		is_vmalloc_addr((void *) x));
 	return x - PAGE_OFFSET;
 }
 EXPORT_SYMBOL(__phys_addr);
 #endif
 
+bool __virt_addr_valid(unsigned long x)
+{
+	if (x < PAGE_OFFSET)
+		return false;
+	if (system_state != SYSTEM_BOOTING && is_vmalloc_addr((void *) x))
+		return false;
+	return pfn_valid((x - PAGE_OFFSET) >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL(__virt_addr_valid);
+
 #endif
 
 int page_is_ram(unsigned long pagenr)

include/asm-x86/page.h +7 -1

index 79544e6..0495171 100644
@@ -178,6 +178,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)))

@@ -187,9 +188,14 @@ static inline pteval_t native_pte_flags(pte_t pte)
 #define __boot_va(x)		__va(x)
 #define __boot_pa(x)		__pa(x)
 
+/*
+ * virt_to_page(kaddr) returns a valid pointer if and only if
+ * virt_addr_valid(kaddr) returns true.
+ */
 #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)
+extern bool __virt_addr_valid(unsigned long kaddr);
+#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
 
 #endif	/* __ASSEMBLY__ */

include/asm-x86/page_32.h +2 -2

index fe86ee2..3a3e4b7 100644
@@ -73,11 +73,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)