Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1567785 > unrolled thread

Re: perf/jit doesn't cope well with mprotect() to jit containing pages

Started byPeter Zijlstra <peterz@infradead.org>
First post2017-01-27 00:00 +0100
Last post2017-01-30 13:00 +0100
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: perf/jit doesn't cope well with mprotect() to jit containing  pages Peter Zijlstra <peterz@infradead.org> - 2017-01-27 00:00 +0100
    Re: perf/jit doesn't cope well with mprotect() to jit containing  pages Andres Freund <andres@anarazel.de> - 2017-01-27 00:10 +0100
    [tip:perf/core] perf/core: Fix PERF_RECORD_MMAP2 prot/flags for  anonymous memory tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2017-01-30 13:00 +0100

#1567785 — Re: perf/jit doesn't cope well with mprotect() to jit containing pages

FromPeter Zijlstra <peterz@infradead.org>
Date2017-01-27 00:00 +0100
SubjectRe: perf/jit doesn't cope well with mprotect() to jit containing pages
Message-ID<t41bj-4KV-7@gated-at.bofh.it>
On Mon, Dec 12, 2016 at 09:49:03AM +0100, Peter Zijlstra wrote:

> > BTW, it's also a bit weird that those MMAP2 records triggered by
> > mprotect/mmap, have prot set to 0...
> 
> Yes, mprotect() does: vma->vm_flags = newflags; before calling
> perf_event_mmap(vma); which then looks at VM_{READ,WRITE,EXEC} bits in
> that word to generate the prot value.
> 
> So that being 0 is a bit weird.

OK, that was a silly bug :/

With that fixed, the following proglet:

#include <unistd.h>
#include <signal.h>
#include <stdio.h>
#include <malloc.h>
#include <stdlib.h>
#include <errno.h>
#include <sys/mman.h>

void main(void)
{
	void *ptr[5];
	int i;

	for (i=0; i<5; i++) {
		ptr[i] = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
		*(char *)ptr[i] = '\0';
		mprotect(ptr[i], 4096, PROT_EXEC);
	}
}


Gives:

root@ivb-ep:~# perf record -d ./mprot
...
root@ivb-ep:~# perf report -D | grep MMAP2
...
43805378481 0xc60 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec0862000(0x1000) @ 0x7f6ec0862000 00:00 0 0]: --xp //anon
43805381755 0xcc0 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec0861000(0x1000) @ 0x7f6ec0861000 00:00 0 0]: rw-p //anon
43805392374 0xd20 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec0861000(0x2000) @ 0x7f6ec0861000 00:00 0 0]: --xp //anon
43805395093 0xd80 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec0860000(0x1000) @ 0x7f6ec0860000 00:00 0 0]: rw-p //anon
43805402842 0xde0 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec0860000(0x3000) @ 0x7f6ec0860000 00:00 0 0]: --xp //anon
43805405388 0xe40 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec085f000(0x1000) @ 0x7f6ec085f000 00:00 0 0]: rw-p //anon
43805412605 0xea0 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec085f000(0x4000) @ 0x7f6ec085f000 00:00 0 0]: --xp //anon
43805415134 0xf00 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec085e000(0x1000) @ 0x7f6ec085e000 00:00 0 0]: rw-p //anon
43805422225 0xf60 [0x60]: PERF_RECORD_MMAP2 2073/2073: [0x7f6ec085e000(0x5000) @ 0x7f6ec085e000 00:00 0 0]: --xp //anon

If you omit the mmap_data=1 thing, it looks like:

123087941779 0x500 [0x60]: PERF_RECORD_MMAP2 2084/2084: [0x7fca5a8bd000(0x1000) @ 0x7fca5a8bd000 00:00 0 0]: --xp //anon
123087953825 0x560 [0x60]: PERF_RECORD_MMAP2 2084/2084: [0x7fca5a8bc000(0x2000) @ 0x7fca5a8bc000 00:00 0 0]: --xp //anon
123087962944 0x5c0 [0x60]: PERF_RECORD_MMAP2 2084/2084: [0x7fca5a8bb000(0x3000) @ 0x7fca5a8bb000 00:00 0 0]: --xp //anon
123087971135 0x620 [0x60]: PERF_RECORD_MMAP2 2084/2084: [0x7fca5a8ba000(0x4000) @ 0x7fca5a8ba000 00:00 0 0]: --xp //anon
123087979360 0x680 [0x60]: PERF_RECORD_MMAP2 2084/2084: [0x7fca5a8b9000(0x5000) @ 0x7fca5a8b9000 00:00 0 0]: --xp //anon

---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 896aa53..e67c5ba 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6660,6 +6683,27 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	char *buf = NULL;
 	char *name;
 
+	if (vma->vm_flags & VM_READ)
+		prot |= PROT_READ;
+	if (vma->vm_flags & VM_WRITE)
+		prot |= PROT_WRITE;
+	if (vma->vm_flags & VM_EXEC)
+		prot |= PROT_EXEC;
+
+	if (vma->vm_flags & VM_MAYSHARE)
+		flags = MAP_SHARED;
+	else
+		flags = MAP_PRIVATE;
+
+	if (vma->vm_flags & VM_DENYWRITE)
+		flags |= MAP_DENYWRITE;
+	if (vma->vm_flags & VM_MAYEXEC)
+		flags |= MAP_EXECUTABLE;
+	if (vma->vm_flags & VM_LOCKED)
+		flags |= MAP_LOCKED;
+	if (vma->vm_flags & VM_HUGETLB)
+		flags |= MAP_HUGETLB;
+
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
@@ -6686,27 +6730,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		maj = MAJOR(dev);
 		min = MINOR(dev);
 
-		if (vma->vm_flags & VM_READ)
-			prot |= PROT_READ;
-		if (vma->vm_flags & VM_WRITE)
-			prot |= PROT_WRITE;
-		if (vma->vm_flags & VM_EXEC)
-			prot |= PROT_EXEC;
-
-		if (vma->vm_flags & VM_MAYSHARE)
-			flags = MAP_SHARED;
-		else
-			flags = MAP_PRIVATE;
-
-		if (vma->vm_flags & VM_DENYWRITE)
-			flags |= MAP_DENYWRITE;
-		if (vma->vm_flags & VM_MAYEXEC)
-			flags |= MAP_EXECUTABLE;
-		if (vma->vm_flags & VM_LOCKED)
-			flags |= MAP_LOCKED;
-		if (vma->vm_flags & VM_HUGETLB)
-			flags |= MAP_HUGETLB;
-
 		goto got_name;
 	} else {
 		if (vma->vm_ops && vma->vm_ops->name) {

[toc] | [next] | [standalone]

#1567792

FromAndres Freund <andres@anarazel.de>
Date2017-01-27 00:10 +0100
Message-ID<t41kZ-530-9@gated-at.bofh.it>
In reply to#1567785
On 2017-01-26 23:15:08 +0100, Peter Zijlstra wrote:
> On Mon, Dec 12, 2016 at 09:49:03AM +0100, Peter Zijlstra wrote:
> 
> > > BTW, it's also a bit weird that those MMAP2 records triggered by
> > > mprotect/mmap, have prot set to 0...
> > 
> > Yes, mprotect() does: vma->vm_flags = newflags; before calling
> > perf_event_mmap(vma); which then looks at VM_{READ,WRITE,EXEC} bits in
> > that word to generate the prot value.
> > 
> > So that being 0 is a bit weird.
> 
> OK, that was a silly bug :/

Heh, that explains that part.  Thanks for looking into it!


> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 896aa53..e67c5ba 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6660,6 +6683,27 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  	char *buf = NULL;
>  	char *name;
>  
> +	if (vma->vm_flags & VM_READ)
> +		prot |= PROT_READ;
> +	if (vma->vm_flags & VM_WRITE)
> +		prot |= PROT_WRITE;
> +	if (vma->vm_flags & VM_EXEC)
> +		prot |= PROT_EXEC;
> +
> +	if (vma->vm_flags & VM_MAYSHARE)
> +		flags = MAP_SHARED;
> +	else
> +		flags = MAP_PRIVATE;
> +
> +	if (vma->vm_flags & VM_DENYWRITE)
> +		flags |= MAP_DENYWRITE;
> +	if (vma->vm_flags & VM_MAYEXEC)
> +		flags |= MAP_EXECUTABLE;
> +	if (vma->vm_flags & VM_LOCKED)
> +		flags |= MAP_LOCKED;
> +	if (vma->vm_flags & VM_HUGETLB)
> +		flags |= MAP_HUGETLB;
> +
>  	if (file) {
>  		struct inode *inode;
>  		dev_t dev;
> @@ -6686,27 +6730,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  		maj = MAJOR(dev);
>  		min = MINOR(dev);
>  
> -		if (vma->vm_flags & VM_READ)
> -			prot |= PROT_READ;
> -		if (vma->vm_flags & VM_WRITE)
> -			prot |= PROT_WRITE;
> -		if (vma->vm_flags & VM_EXEC)
> -			prot |= PROT_EXEC;
> -
> -		if (vma->vm_flags & VM_MAYSHARE)
> -			flags = MAP_SHARED;
> -		else
> -			flags = MAP_PRIVATE;
> -
> -		if (vma->vm_flags & VM_DENYWRITE)
> -			flags |= MAP_DENYWRITE;
> -		if (vma->vm_flags & VM_MAYEXEC)
> -			flags |= MAP_EXECUTABLE;
> -		if (vma->vm_flags & VM_LOCKED)
> -			flags |= MAP_LOCKED;
> -		if (vma->vm_flags & VM_HUGETLB)
> -			flags |= MAP_HUGETLB;
> -
>  		goto got_name;
>  	} else {
>  		if (vma->vm_ops && vma->vm_ops->name) {

I'm not a mm/perf (or kernel in general) expert. But this looks
obviously buggy to me. And the fix makes it look correct. So if that's
helpful (I'm not actually sure):
Signed-off-by: Andres Freund <andres@anarazel.de>

Not sure if this on its own has enough consequences to be worth being
put into stable?

Not that it's this fix's responsibility, but I did note that that piece
of code doesn't maintain MAP_GROWSDOWN, nor set the bits controlling the
size of the HUGETLB mapping.  Seems harmless enough, but given that I
already looked I thought it'd bring it up.

Greetings,

Andres Freund

[toc] | [prev] | [next] | [standalone]

#1569639 — [tip:perf/core] perf/core: Fix PERF_RECORD_MMAP2 prot/flags for anonymous memory

Fromtip-bot for Peter Zijlstra <tipbot@zytor.com>
Date2017-01-30 13:00 +0100
Subject[tip:perf/core] perf/core: Fix PERF_RECORD_MMAP2 prot/flags for anonymous memory
Message-ID<t5iMN-3x0-7@gated-at.bofh.it>
In reply to#1567785
Commit-ID:  0b3589be9b98994ce3d5aeca52445d1f5627c4ba
Gitweb:     http://git.kernel.org/tip/0b3589be9b98994ce3d5aeca52445d1f5627c4ba
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 26 Jan 2017 23:15:08 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 30 Jan 2017 11:41:26 +0100

perf/core: Fix PERF_RECORD_MMAP2 prot/flags for anonymous memory

Andres reported that MMAP2 records for anonymous memory always have
their protection field 0.

Turns out, someone daft put the prot/flags generation code in the file
branch, leaving them unset for anonymous memory.

Reported-by: Andres Freund <andres@anarazel.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Don Zickus <dzickus@redhat.com
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: acme@kernel.org
Cc: anton@ozlabs.org
Cc: namhyung@kernel.org
Cc: stable@vger.kernel.org # v3.16+
Fixes: f972eb63b100 ("perf: Pass protection and flags bits through mmap2 interface")
Link: http://lkml.kernel.org/r/20170126221508.GF6536@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4e1f4c0..e5aaa80 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6632,6 +6632,27 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	char *buf = NULL;
 	char *name;
 
+	if (vma->vm_flags & VM_READ)
+		prot |= PROT_READ;
+	if (vma->vm_flags & VM_WRITE)
+		prot |= PROT_WRITE;
+	if (vma->vm_flags & VM_EXEC)
+		prot |= PROT_EXEC;
+
+	if (vma->vm_flags & VM_MAYSHARE)
+		flags = MAP_SHARED;
+	else
+		flags = MAP_PRIVATE;
+
+	if (vma->vm_flags & VM_DENYWRITE)
+		flags |= MAP_DENYWRITE;
+	if (vma->vm_flags & VM_MAYEXEC)
+		flags |= MAP_EXECUTABLE;
+	if (vma->vm_flags & VM_LOCKED)
+		flags |= MAP_LOCKED;
+	if (vma->vm_flags & VM_HUGETLB)
+		flags |= MAP_HUGETLB;
+
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
@@ -6658,27 +6679,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		maj = MAJOR(dev);
 		min = MINOR(dev);
 
-		if (vma->vm_flags & VM_READ)
-			prot |= PROT_READ;
-		if (vma->vm_flags & VM_WRITE)
-			prot |= PROT_WRITE;
-		if (vma->vm_flags & VM_EXEC)
-			prot |= PROT_EXEC;
-
-		if (vma->vm_flags & VM_MAYSHARE)
-			flags = MAP_SHARED;
-		else
-			flags = MAP_PRIVATE;
-
-		if (vma->vm_flags & VM_DENYWRITE)
-			flags |= MAP_DENYWRITE;
-		if (vma->vm_flags & VM_MAYEXEC)
-			flags |= MAP_EXECUTABLE;
-		if (vma->vm_flags & VM_LOCKED)
-			flags |= MAP_LOCKED;
-		if (vma->vm_flags & VM_HUGETLB)
-			flags |= MAP_HUGETLB;
-
 		goto got_name;
 	} else {
 		if (vma->vm_ops && vma->vm_ops->name) {

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web