[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

Subsystems: arm64 port (aarch64 architecture), the rest

STALE3908d

7 messages, 3 authors, 2016-01-28 · open the first message on its own page

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Ard Biesheuvel <hidden>
Date: 2016-01-18 14:01:05

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 arch/arm64/mm/pageattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 3571c7309c5e..1360a02d88b7 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c

@@ -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;

-- 
2.5.0

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: mark.rutland@arm.com (Mark Rutland)
Date: 2016-01-18 15:05:56

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>

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 <mark.rutland@arm.com>

quoted hunk

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

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 3571c7309c5e..1360a02d88b7 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c

@@ -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 hunk

 
 #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;

-- 
2.5.0

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Will Deacon <hidden>
Date: 2016-01-28 15:08:16

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 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 <redacted>

Will

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Ard Biesheuvel <hidden>
Date: 2016-01-28 16:40:23

On 28 January 2016 at 16:08, Will Deacon [off-list ref] wrote:

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

quoted

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 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:

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

Acked-by: Will Deacon <redacted>

Will

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Ard Biesheuvel <hidden>
Date: 2016-01-28 16:43:41

On 28 January 2016 at 16:08, Will Deacon [off-list ref] wrote:

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

quoted

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 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

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

Acked-by: Will Deacon <redacted>

Thanks

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Will Deacon <hidden>
Date: 2016-01-28 18:10:58

On Thu, Jan 28, 2016 at 05:43:41PM +0100, Ard Biesheuvel wrote:

On 28 January 2016 at 16:08, Will Deacon [off-list ref] wrote:

quoted

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

quoted

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 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.

Urgh :(

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

Will

[PATCH] arm64: allow vmalloc regions to be set with set_memory_*

From: Ard Biesheuvel <hidden>
Date: 2016-01-28 19:07:02

On 28 January 2016 at 19:10, Will Deacon [off-list ref] wrote:

On Thu, Jan 28, 2016 at 05:43:41PM +0100, Ard Biesheuvel wrote:

quoted

On 28 January 2016 at 16:08, Will Deacon [off-list ref] wrote:

quoted

On Mon, Jan 18, 2016 at 03:01:05PM +0100, Ard Biesheuvel wrote:

quoted

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 <redacted>
Signed-off-by: Ard Biesheuvel <redacted>
---
 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.

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.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help