Re: [RFC][PATCH] arm64: Add support for CONFIG_DEBUG_VIRTUAL

From: Laura Abbott
Date: Fri Oct 28 2016 - 13:43:11 EST

On 10/28/2016 07:49 AM, Mark Rutland wrote:
Hi Laura,

On Thu, Oct 27, 2016 at 05:18:12PM -0700, Laura Abbott 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.

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.

This is fantastic; thanks for putting this together!

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?

I think it's worth aligning with x86, so modulo a couple of comments
below, (1) and (4) seem fine. I think (2) just requires an additional
shuffle.

---
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
+
#define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)
#define __phys_to_kimg(x) ((unsigned long)((x) + kimage_voffset))

I think we can move all the existing conversion logic here (including
into page_to_phys/phys_to_page) into the existing #ifndef __ASSEMBLY__
block at the end of the file. Assembly clearly can't use these, and we
already have virt_to_phys and others in that #ifndef.

Assuming that works, would you mind doing that as a preparatory patch?


Sure.

@@ -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 */
+ return (__x & ~PAGE_OFFSET) + PHYS_OFFSET;
+ } else {
+ VIRTUAL_BUG_ON(x < kimage_vaddr || x > (unsigned long)_end);

IIUC, in (3) you were asking if the last check should be '>' or '>='?

To match high_memory, I suspect the latter, as _end doesn't fall within
the mapped virtual address space.


I was actually concerned about if _end would be correct with KASLR.
Ard confirmed that it gets fixed up to be correct. I'll change the
check to check for >=.

+ return (__x - kimage_voffset);
+ }
+}
+EXPORT_SYMBOL(__virt_to_phys);

It's a bit annoying that we have to duplicate the logic here to add the
VIRTUAL_BUG_ON(), but I see that x86 also do this, and I don't have a
better suggestion.

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)

I have no strong feelings about this, but perhaps it's nicer to have
something like ARCH_HAS_DEBUG_VIRTUAL?


Yes, if this ever gets added for other arches this will start to get
unwieldy. I'll look at cleaning that up.

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)

Rather than an architecture list, can we depend on DEBUG_VIRTUAL? Are
there other checks that we're trying to avoid?

... or could we avoid ifdeffery entirely with something like:

/*
* We can't use __pa(high_memory) directly, since high_memory
* isn't a valid direct map VA, and DEBUG_VIRTUAL will (validly)
* complain. Find the boundary by adding one to the last valid
* address.
*/
highmem_start = __pa(high_memory - 1) + 1;


I like getting rid of the #ifdef there. Maybe future cleanup could turn
this into a #define, HIGHMEM_PHYS since it seems to be used in quite
a few places in the kernel.

Thanks,
Mark.


Thanks,
Laura

/*
* 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