arm64: allow vmalloc regions to be set with set_memory_*

PATCH Superseded Ard Biesheuvel 3 participants 7 messages Jan 18, 2016 – Jan 28, 2016 arm64

Share & tools

Ard Biesheuvel

author

2016-01-18 14:01 UTC #0 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

arch/arm64/mm/pageattr.c +19 -4

index 3571c7309c5e..1360a02d88b7 100644
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/vmalloc.h>
 #include <linux/sched.h>
 
 #include <asm/pgtable.h>

@@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr, int numpages,
 	unsigned long end = start + size;
 	int ret;
 	struct page_change_data data;
+	struct vm_struct *area;
 
 	if (!PAGE_ALIGNED(addr)) {
 		start &= PAGE_MASK;

@@ -51,10 +53,23 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
-	if (start < MODULES_VADDR || start >= MODULES_END)
-		return -EINVAL;
-
-	if (end < MODULES_VADDR || end >= MODULES_END)
+	/*
+	 * Kernel VA mappings are always live, and splitting live section
+	 * mappings into page mappings may cause TLB conflicts. This means
+	 * we have to ensure that changing the permission bits of the range
+	 * we are operating on does not result in such splitting.
+	 *
+	 * Let's restrict ourselves to mappings created by vmalloc (or vmap).
+	 * Those are guaranteed to consist entirely of page mappings, and
+	 * splitting is never needed.
+	 *
+	 * So check whether the [addr, addr + size) interval is entirely
+	 * covered by precisely one VM area that has the VM_ALLOC flag set.
+	 */
+	area = find_vm_area((void *)addr);
+	if (!area ||
+	    end > (unsigned long)area->addr + area->size ||
+	    !(area->flags & VM_ALLOC))
 		return -EINVAL;
 
 	data.set_mask = set_mask;

Signature · 1 line

2.5.0

Mark Rutland

reviewer

2016-01-18 15:05 UTC #1 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

Quoted 8 lines · Ard Biesheuvel

The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Previously we allowed set_memory_* calls on any range in the modules area (even if that covered multiple VMAs). However, I believe that given the way vmap area is allocated, no caller can rely on mappings being adjacent, and thus such calls would be erroneous.

Given that, this looks good to me (with one minor nit below). FWIW:

Acked-by Mark Rutland <[email protected]>

Quoted 9 lines

---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

arch/arm64/mm/pageattr.c +0 -0

index 3571c7309c5e..1360a02d88b7 100644

arch/arm64/mm/pageattr.c line 17

Quoted 5 lines

arch/arm64/mm/pageattr.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/vmalloc.h>
 #include <linux/sched.h>

Nit: please keep alphabetical order here.

Mark.

Quoted 41 lines

arch/arm64/mm/pageattr.c +18 -4

 #include <asm/pgtable.h>

@@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr, int numpages,
 	unsigned long end = start + size;
 	int ret;
 	struct page_change_data data;
+	struct vm_struct *area;
 
 	if (!PAGE_ALIGNED(addr)) {
 		start &= PAGE_MASK;

@@ -51,10 +53,23 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
-	if (start < MODULES_VADDR || start >= MODULES_END)
-		return -EINVAL;
-
-	if (end < MODULES_VADDR || end >= MODULES_END)
+	/*
+	 * Kernel VA mappings are always live, and splitting live section
+	 * mappings into page mappings may cause TLB conflicts. This means
+	 * we have to ensure that changing the permission bits of the range
+	 * we are operating on does not result in such splitting.
+	 *
+	 * Let's restrict ourselves to mappings created by vmalloc (or vmap).
+	 * Those are guaranteed to consist entirely of page mappings, and
+	 * splitting is never needed.
+	 *
+	 * So check whether the [addr, addr + size) interval is entirely
+	 * covered by precisely one VM area that has the VM_ALLOC flag set.
+	 */
+	area = find_vm_area((void *)addr);
+	if (!area ||
+	    end > (unsigned long)area->addr + area->size ||
+	    !(area->flags & VM_ALLOC))
 		return -EINVAL;
 
 	data.set_mask = set_mask;

Signature · 1 line

2.5.0

Will Deacon

2016-01-28 15:08 UTC #2 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

In reply to Ard Biesheuvel

Quoted 11 lines · Ard Biesheuvel

The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

What's the user for this? It would be better to apply along with something that actually needs to change permission for arbitrary vmalloc mappings.

Anyway, with that (and a rebase to -rc2):

Acked-by Will Deacon <[email protected]>

Will

Ard Biesheuvel

author

2016-01-28 16:40 UTC #3 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

Quoted 7 lines · Will Deacon

Quoted 6 lines · Ard Biesheuvel
The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.

Quoted 9 lines

Quoted 5 lines
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
What's the user for this? It would be better to apply along with something that actually needs to change permission for arbitrary vmalloc mappings.

BPF already uses set_memory_ro() but doesn't bother to check the return value.

Before the patch, I get:

Quoted 5 lines

Anyway, with that (and a rebase to -rc2):
Acked-by Will Deacon <[email protected]>
Will

Ard Biesheuvel

author

2016-01-28 16:43 UTC #4 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

In reply to Will Deacon

Quoted 7 lines · Will Deacon

Quoted 6 lines · Ard Biesheuvel
The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.

Quoted 9 lines

Quoted 5 lines
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
What's the user for this? It would be better to apply along with something that actually needs to change permission for arbitrary vmalloc mappings.

BPF already uses set_memory_ro() but doesn't bother to check the return value.

Without the patch, I get:

root at ubuntu-arm64:~# grep bpf /proc/vmallocinfo
0xffff000000014000-0xffff000000016000 8192 bpf_prog_alloc+0x3c/0xb8
pages=1 vmalloc
0xffff00000017a000-0xffff00000017c000 8192 bpf_prog_alloc+0x3c/0xb8
pages=1 vmalloc
root at ubuntu-arm64:~# grep -E 0xffff000000014000\|0xffff00000017a000 /sys/kernel/debug/kernel_page_tables
0xffff000000014000-0xffff000000015000 4K RW NX SHD AF
UXN MEM/NORMAL
0xffff00000017a000-0xffff00000017b000 4K RW NX SHD AF
UXN MEM/NORMAL

and with:

root at ubuntu-arm64:~# grep bpf /proc/vmallocinfo
0xffff000000014000-0xffff000000016000 8192 bpf_prog_alloc+0x3c/0xb8
pages=1 vmalloc
0xffff00000017a000-0xffff00000017c000 8192 bpf_prog_alloc+0x3c/0xb8
pages=1 vmalloc
root at ubuntu-arm64:~# grep -E 0xffff000000014000\|0xffff00000017a000 /sys/kernel/debug/kernel_page_tables
0xffff000000014000-0xffff000000015000 4K ro NX SHD AF
UXN MEM/NORMAL
0xffff00000017a000-0xffff00000017b000 4K ro NX SHD AF
UXN MEM/NORMAL

Quoted 4 lines

Anyway, with that (and a rebase to -rc2):
Acked-by Will Deacon <[email protected]>

Thanks

Will Deacon

2016-01-28 18:10 UTC #5 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

Quoted 14 lines · Ard Biesheuvel

Quoted 13 lines · Will Deacon
Quoted 11 lines · Ard Biesheuvel
The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Quoted 5 lines

Quoted 3 lines
What's the user for this? It would be better to apply along with something that actually needs to change permission for arbitrary vmalloc mappings.
BPF already uses set_memory_ro() but doesn't bother to check the return value.

Urgh :(

Then I can take it as a fix, if you like.

Will

Ard Biesheuvel

author

2016-01-28 19:07 UTC #6 lore raw

PATCH 1/1 arm64: allow vmalloc regions to be set with set_memory_*

Quoted 19 lines · Will Deacon

Quoted 18 lines · Ard Biesheuvel
Quoted 16 lines · Will Deacon
Quoted 11 lines · Ard Biesheuvel
The range of set_memory_* is currently restricted to the module address range because of difficulties in breaking down larger block sizes. vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the function ranges and add a comment explaining why the range is restricted the way it is.
Suggested-by Laura Abbott <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)
What's the user for this? It would be better to apply along with something that actually needs to change permission for arbitrary vmalloc mappings.

Quoted 6 lines

Quoted 1 line
BPF already uses set_memory_ro() but doesn't bother to check the return value.
Urgh :(
Then I can take it as a fix, if you like.

I will let you decide whether it should be merged now or in v4.6. I don't think there's any urgency here, though.

BTW I already sent a v2 based on -rc2 here
http://article.gmane.org/gmane.linux.ports.arm.kernel/472555

Thanks,
Ard.