Re: [RFC][PATCH] arm64: Add support for CONFIG_DEBUG_VIRTUAL
From: Laura Abbott
Date: Fri Oct 28 2016 - 13:27:31 EST
On 10/28/2016 12:52 AM, Ard Biesheuvel wrote:
Hi Laura,
On 28 October 2016 at 01:18, Laura Abbott <labbott@xxxxxxxxxx> wrote:
x86 has an option CONFIG_DEBUG_VIRTUAL to do additional checks
on virt_to_phys calls. The goal is to catch users who are calling
virt_to_phys on non-linear addresses immediately. As features
such as CONFIG_VMAP_STACK get enabled for arm64, this becomes
increasingly important. Add checks to catch bad virt_to_phys
usage.
I think this is a useful thing to have. However, the Kconfig
description talks about virt to page translations, not virt to phys.
Of course, this is a shift away from being equivalent on x86, but not
so much on arm64. Any concerns there?
I think the description is vague. I'm guessing there were problems
with virt_to_page but the actual check ends up going in virt_to_phys
because that's where the actual problem occurs.
Signed-off-by: Laura Abbott <labbott@xxxxxxxxxx>
---
This has been on my TODO list for a while. It caught a few bugs with
CONFIG_VMAP_STACK on x86 so when that eventually gets added
for arm64 it will be useful to have. This caught one driver calling __pa on an
ioremapped address already. RFC for a couple of reasons:
1) This is basically a direct port of the x86 approach.
2) I needed some #ifndef __ASSEMBLY__ which I don't like to throw around.
3) I'm not quite sure about the bounds check for the VIRTUAL_BUG_ON with KASLR,
specifically the _end check.
4) Is it worth actually keeping this as DEBUG_VIRTUAL vs. folding it into
another option?
---
arch/arm64/include/asm/memory.h | 11 ++++++++++-
arch/arm64/mm/Makefile | 2 +-
arch/arm64/mm/physaddr.c | 17 +++++++++++++++++
lib/Kconfig.debug | 2 +-
mm/cma.c | 2 +-
5 files changed, 30 insertions(+), 4 deletions(-)
create mode 100644 arch/arm64/mm/physaddr.c
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index ba62df8..9805adc 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -106,11 +106,19 @@
* private definitions which should NOT be used outside memory.h
* files. Use virt_to_phys/phys_to_virt/__pa/__va instead.
*/
-#define __virt_to_phys(x) ({ \
+#define __virt_to_phys_nodebug(x) ({ \
phys_addr_t __x = (phys_addr_t)(x); \
__x & BIT(VA_BITS - 1) ? (__x & ~PAGE_OFFSET) + PHYS_OFFSET : \
(__x - kimage_voffset); })
+#ifdef CONFIG_DEBUG_VIRTUAL
+#ifndef __ASSEMBLY__
+extern unsigned long __virt_to_phys(unsigned long x);
+#endif
+#else
+#define __virt_to_phys(x) __virt_to_phys_nodebug(x)
+#endif
+
Couldn't you move this whole block into the #ifndef __ASSEMBLY__
section lower down in the file?
Yes, I think that makes more sense.
#define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)
#define __phys_to_kimg(x) ((unsigned long)((x) + kimage_voffset))
@@ -202,6 +210,7 @@ static inline void *phys_to_virt(phys_addr_t x)
* Drivers should NOT use these either.
*/
#define __pa(x) __virt_to_phys((unsigned long)(x))
+#define __pa_nodebug(x) __virt_to_phys_nodebug((unsigned long)(x))
#define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
#define virt_to_pfn(x) __phys_to_pfn(__virt_to_phys(x))
diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
index 54bb209..bcea84e 100644
--- a/arch/arm64/mm/Makefile
+++ b/arch/arm64/mm/Makefile
@@ -5,6 +5,6 @@ obj-y := dma-mapping.o extable.o fault.o init.o \
obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o
obj-$(CONFIG_ARM64_PTDUMP) += dump.o
obj-$(CONFIG_NUMA) += numa.o
-
+obj-$(CONFIG_DEBUG_VIRTUAL) += physaddr.o
obj-$(CONFIG_KASAN) += kasan_init.o
KASAN_SANITIZE_kasan_init.o := n
diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
new file mode 100644
index 0000000..6c271e2
--- /dev/null
+++ b/arch/arm64/mm/physaddr.c
@@ -0,0 +1,17 @@
+#include <linux/mm.h>
+
+#include <asm/memory.h>
+
+unsigned long __virt_to_phys(unsigned long x)
+{
+ phys_addr_t __x = (phys_addr_t)x;
+
+ if (__x & BIT(VA_BITS - 1)) {
+ /* The bit check ensures this is the right range */
Could you expand the comment to clarify that the linear region starts
right in the middle of the kernel virtual address space, and so we
only have to test the top bit?
Sure.
+ return (__x & ~PAGE_OFFSET) + PHYS_OFFSET;
+ } else {l
+ VIRTUAL_BUG_ON(x < kimage_vaddr || x > (unsigned long)_end);
This looks fine. References to _end are relocated at boot to refer to
the actual runtime offset.
Thanks for clarifying.
+ return (__x - kimage_voffset);
+ }
+}
+EXPORT_SYMBOL(__virt_to_phys);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 33bc56c..e5634bb 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -604,7 +604,7 @@ config DEBUG_VM_PGFLAGS
config DEBUG_VIRTUAL
bool "Debug VM translations"
- depends on DEBUG_KERNEL && X86
+ depends on DEBUG_KERNEL && (X86 || ARM64)
help
Enable some costly sanity checks in virtual to page code. This can
catch mistakes with virt_to_page() and friends.
diff --git a/mm/cma.c b/mm/cma.c
index 384c2cb..2345803 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -235,7 +235,7 @@ int __init cma_declare_contiguous(phys_addr_t base,
phys_addr_t highmem_start;
int ret = 0;
-#ifdef CONFIG_X86
+#if defined(CONFIG_X86) || defined(CONFIG_ARM64)
/*
* high_memory isn't direct mapped memory so retrieving its physical
* address isn't appropriate. But it would be useful to check the
--
2.7.4