[patch 0/5] Various perfcounter fixes

[patch 0/5] Various perfcounter fixes

[Anton Blanchard]

Here are a few more perfcounter fixes, and one patch that changes the
call graph option to be consistent between perf record and perf report.
Perhaps I'm too late on that one, but I thought I'd give it a try :)

[patch 1/5] perf_counter: Make sure we dont leak kernel memory to userspace

[Anton Blanchard]

There are a few places we are leaking tiny amounts of kernel memory
to userspace. This happens when writing out strings because we always align
the end to 64 bits.

To avoid this we should always use an appropriately sized temporary buffer
and ensure it is zeroed.

Since d_path assembles the string from the end of the buffer backwards, we
need to add 64 bits after the buffer to allow for alignment.

We also need to copy arch_vma_name to the temporary buffer, because if we use
it directly we may end up copying to userspace a number of bytes after the end
of the string constant.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux.trees.git/kernel/perf_counter.c
===================================================================
--- linux.trees.git.orig/kernel/perf_counter.c	2009-07-16 19:44:23.000000000 +1000
+++ linux.trees.git/kernel/perf_counter.c	2009-07-16 20:25:37.000000000 +1000
@@ -2969,8 +2969,10 @@
 	struct perf_cpu_context *cpuctx;
 	struct perf_counter_context *ctx;
 	unsigned int size;
-	char *comm = comm_event->task->comm;
+	char comm[TASK_COMM_LEN];
 
+	memset(comm, 0, sizeof(comm));
+	strncpy(comm, comm_event->task->comm, sizeof(comm));
 	size = ALIGN(strlen(comm)+1, sizeof(u64));
 
 	comm_event->comm = comm;
@@ -3089,8 +3091,15 @@
 	char *buf = NULL;
 	const char *name;
 
+	memset(tmp, 0, sizeof(tmp));
+
 	if (file) {
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		/*
+		 * d_path works from the end of the buffer backwards, so we
+		 * need to add enough zero bytes after the string to handle
+		 * the 64bit alignment we do later.
+		 */
+		buf = kzalloc(PATH_MAX + sizeof(u64), GFP_KERNEL);
 		if (!buf) {
 			name = strncpy(tmp, "//enomem", sizeof(tmp));
 			goto got_name;
@@ -3101,9 +3110,11 @@
 			goto got_name;
 		}
 	} else {
-		name = arch_vma_name(mmap_event->vma);
-		if (name)
+		if (arch_vma_name(mmap_event->vma)) {
+			name = strncpy(tmp, arch_vma_name(mmap_event->vma),
+				       sizeof(tmp));
 			goto got_name;
+		}
 
 		if (!vma->vm_mm) {
 			name = strncpy(tmp, "[vdso]", sizeof(tmp));

-- 

[tip:perfcounters/urgent] perf_counter: Make sure we dont leak kernel memory to userspace

[tip-bot for Anton Blanchard]

Commit-ID:  413ee3b48ab582ffea33e7e140c7a2c5ea657e9a
Gitweb:     http://git.kernel.org/tip/413ee3b48ab582ffea33e7e140c7a2c5ea657e9a
Author:     Anton Blanchard <anton@samba.org>
AuthorDate: Thu, 16 Jul 2009 15:15:52 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 18 Jul 2009 11:21:29 +0200

perf_counter: Make sure we dont leak kernel memory to userspace

There are a few places we are leaking tiny amounts of kernel
memory to userspace. This happens when writing out strings
because we always align the end to 64 bits.

To avoid this we should always use an appropriately sized
temporary buffer and ensure it is zeroed.

Since d_path assembles the string from the end of the buffer
backwards, we need to add 64 bits after the buffer to allow for
alignment.

We also need to copy arch_vma_name to the temporary buffer,
because if we use it directly we may end up copying to
userspace a number of bytes after the end of the string
constant.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20090716104817.273972048@samba.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/perf_counter.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index c6c38fb..f7a8ab9 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -2968,8 +2968,10 @@ static void perf_counter_comm_event(struct perf_comm_event *comm_event)
 	struct perf_cpu_context *cpuctx;
 	struct perf_counter_context *ctx;
 	unsigned int size;
-	char *comm = comm_event->task->comm;
+	char comm[TASK_COMM_LEN];
 
+	memset(comm, 0, sizeof(comm));
+	strncpy(comm, comm_event->task->comm, sizeof(comm));
 	size = ALIGN(strlen(comm)+1, sizeof(u64));
 
 	comm_event->comm = comm;
@@ -3088,8 +3090,15 @@ static void perf_counter_mmap_event(struct perf_mmap_event *mmap_event)
 	char *buf = NULL;
 	const char *name;
 
+	memset(tmp, 0, sizeof(tmp));
+
 	if (file) {
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		/*
+		 * d_path works from the end of the buffer backwards, so we
+		 * need to add enough zero bytes after the string to handle
+		 * the 64bit alignment we do later.
+		 */
+		buf = kzalloc(PATH_MAX + sizeof(u64), GFP_KERNEL);
 		if (!buf) {
 			name = strncpy(tmp, "//enomem", sizeof(tmp));
 			goto got_name;
@@ -3100,9 +3109,11 @@ static void perf_counter_mmap_event(struct perf_mmap_event *mmap_event)
 			goto got_name;
 		}
 	} else {
-		name = arch_vma_name(mmap_event->vma);
-		if (name)
+		if (arch_vma_name(mmap_event->vma)) {
+			name = strncpy(tmp, arch_vma_name(mmap_event->vma),
+				       sizeof(tmp));
 			goto got_name;
+		}
 
 		if (!vma->vm_mm) {
 			name = strncpy(tmp, "[vdso]", sizeof(tmp));

[patch 2/5] perf_counter: Synthesize VDSO mmap event

[Anton Blanchard]

perf record synthesizes mmap events for the running process. Right now
it just catches file mappings, but we can check for the vdso symbol
and add that too.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Only tested on PowerPC, but I assume x86 has the same issue.

Index: linux.trees.git/tools/perf/builtin-record.c
===================================================================
--- linux.trees.git.orig/tools/perf/builtin-record.c	2009-07-13 12:33:49.000000000 +1000
+++ linux.trees.git/tools/perf/builtin-record.c	2009-07-13 13:25:00.000000000 +1000
@@ -313,6 +313,10 @@
 		if (*pbf == 'x') { /* vm_exec */
 			char *execname = strchr(bf, '/');
 
+			/* Catch VDSO */
+			if (execname == NULL)
+				execname = strstr(bf, "[vdso]");
+
 			if (execname == NULL)
 				continue;
 

-- 

[tip:perfcounters/urgent] perf_counter: Synthesize VDSO mmap event

[tip-bot for Anton Blanchard]

Commit-ID:  11b5f81e1b0ea0bc84fe32f0a27054e052b2bf84
Gitweb:     http://git.kernel.org/tip/11b5f81e1b0ea0bc84fe32f0a27054e052b2bf84
Author:     Anton Blanchard <anton@samba.org>
AuthorDate: Thu, 16 Jul 2009 15:44:29 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 18 Jul 2009 11:21:30 +0200

perf_counter: Synthesize VDSO mmap event

perf record synthesizes mmap events for the running process.
Right now it just catches file mappings, but we can check for
the vdso symbol and add that too.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20090716104817.517264409@samba.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/builtin-record.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4ef78a5..072aaf0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -313,6 +313,10 @@ static void pid_synthesize_mmap_samples(pid_t pid)
 		if (*pbf == 'x') { /* vm_exec */
 			char *execname = strchr(bf, '/');
 
+			/* Catch VDSO */
+			if (execname == NULL)
+				execname = strstr(bf, "[vdso]");
+
 			if (execname == NULL)
 				continue;
 

[patch 3/5] perf_counter: Log vfork as a fork event

[Anton Blanchard]

Right now we don't output vfork events. Even though we should always see an
exec after a vfork, we may get perfcounter samples between the vfork and exec.
These samples can lead to some confusion when parsing perfcounter data.

To keep things consistent we should always log a fork event. It will result
in a little more log data, but is less confusing to trace parsing tools.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

It took me a while to realise this wasn't a bug in some trace parsing
code I wrote, or an issue with samples being dropped :)

Index: linux.trees.git/kernel/fork.c
===================================================================
--- linux.trees.git.orig/kernel/fork.c	2009-07-15 10:00:30.000000000 +1000
+++ linux.trees.git/kernel/fork.c	2009-07-15 10:01:28.000000000 +1000
@@ -1407,14 +1407,11 @@
 		if (clone_flags & CLONE_VFORK) {
 			p->vfork_done = &vfork;
 			init_completion(&vfork);
-		} else if (!(clone_flags & CLONE_VM)) {
-			/*
-			 * vfork will do an exec which will call
-			 * set_task_comm()
-			 */
-			perf_counter_fork(p);
 		}
 
+		if (!(clone_flags & CLONE_THREAD))
+			perf_counter_fork(p);
+
 		audit_finish_fork(p);
 		tracehook_report_clone(regs, clone_flags, nr, p);
 

-- 

[tip:perfcounters/urgent] perf_counter: Log vfork as a fork event

[tip-bot for Anton Blanchard]

Commit-ID:  ed900c054b541254f0ce5cedaf75206e29bd614e
Gitweb:     http://git.kernel.org/tip/ed900c054b541254f0ce5cedaf75206e29bd614e
Author:     Anton Blanchard <anton@samba.org>
AuthorDate: Thu, 16 Jul 2009 15:44:29 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 18 Jul 2009 11:21:31 +0200

perf_counter: Log vfork as a fork event

Right now we don't output vfork events. Even though we should
always see an exec after a vfork, we may get perfcounter
samples between the vfork and exec. These samples can lead to
some confusion when parsing perfcounter data.

To keep things consistent we should always log a fork event. It
will result in a little more log data, but is less confusing to
trace parsing tools.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20090716104817.589309391@samba.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/fork.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 467746b..4812d60 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1408,14 +1408,11 @@ long do_fork(unsigned long clone_flags,
 		if (clone_flags & CLONE_VFORK) {
 			p->vfork_done = &vfork;
 			init_completion(&vfork);
-		} else if (!(clone_flags & CLONE_VM)) {
-			/*
-			 * vfork will do an exec which will call
-			 * set_task_comm()
-			 */
-			perf_counter_fork(p);
 		}
 
+		if (!(clone_flags & CLONE_THREAD))
+			perf_counter_fork(p);
+
 		audit_finish_fork(p);
 		tracehook_report_clone(regs, clone_flags, nr, p);
 

[patch 4/5] perf_counter: Add perf record option to log addresses.

[Anton Blanchard]

Add the -d or --data option to log event addresses (eg page faults).

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux.trees.git/tools/perf/builtin-record.c
===================================================================
--- linux.trees.git.orig/tools/perf/builtin-record.c	2009-07-15 11:31:53.000000000 +1000
+++ linux.trees.git/tools/perf/builtin-record.c	2009-07-15 11:34:26.000000000 +1000
@@ -43,6 +43,7 @@
 static int			verbose				= 0;
 static int			inherit_stat			= 0;
 static int			no_samples			= 0;
+static int			sample_address			= 0;
 
 static long			samples;
 static struct timeval		last_read;
@@ -405,6 +406,9 @@
 	if (inherit_stat)
 		attr->inherit_stat = 1;
 
+	if (sample_address)
+		attr->sample_type	|= PERF_SAMPLE_ADDR;
+
 	if (call_graph)
 		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
 
@@ -649,6 +653,8 @@
 		    "be more verbose (show counter open errors, etc)"),
 	OPT_BOOLEAN('s', "stat", &inherit_stat,
 		    "per thread counts"),
+	OPT_BOOLEAN('d', "data", &sample_address,
+		    "Sample addresses"),
 	OPT_BOOLEAN('n', "no-samples", &no_samples,
 		    "don't sample"),
 	OPT_END()

-- 

[tip:perfcounters/urgent] perf_counter: Add perf record option to log addresses

[tip-bot for Anton Blanchard]

Commit-ID:  4bba828dd9bb950ad1fe340ef148a5436a10f131
Gitweb:     http://git.kernel.org/tip/4bba828dd9bb950ad1fe340ef148a5436a10f131
Author:     Anton Blanchard <anton@samba.org>
AuthorDate: Thu, 16 Jul 2009 15:44:29 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 18 Jul 2009 11:21:32 +0200

perf_counter: Add perf record option to log addresses

Add the -d or --data option to log event addresses (eg page
faults).

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20090716104817.697698033@samba.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/builtin-record.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 072aaf0..68a9be0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -43,6 +43,7 @@ static int			call_graph			= 0;
 static int			verbose				= 0;
 static int			inherit_stat			= 0;
 static int			no_samples			= 0;
+static int			sample_address			= 0;
 
 static long			samples;
 static struct timeval		last_read;
@@ -405,6 +406,9 @@ static void create_counter(int counter, int cpu, pid_t pid)
 	if (inherit_stat)
 		attr->inherit_stat = 1;
 
+	if (sample_address)
+		attr->sample_type	|= PERF_SAMPLE_ADDR;
+
 	if (call_graph)
 		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
 
@@ -649,6 +653,8 @@ static const struct option options[] = {
 		    "be more verbose (show counter open errors, etc)"),
 	OPT_BOOLEAN('s', "stat", &inherit_stat,
 		    "per thread counts"),
+	OPT_BOOLEAN('d', "data", &sample_address,
+		    "Sample addresses"),
 	OPT_BOOLEAN('n', "no-samples", &no_samples,
 		    "don't sample"),
 	OPT_END()

[patch 5/5] perf_counter: Make call graph option consistent

[Anton Blanchard]

perf record uses -g for logging call graph data but perf report uses -c
to print call graph data. Be consistent and use -g everywhere for call graph
data.

Also update the help text to reflect the current default - fractal,0.5

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Im not sure if it's too late to make this change, but I've tripped
up a number of times when I cycle between perf record and perf report.

Index: linux.trees.git/tools/perf/builtin-report.c
===================================================================
--- linux.trees.git.orig/tools/perf/builtin-report.c	2009-07-16 19:41:41.000000000 +1000
+++ linux.trees.git/tools/perf/builtin-report.c	2009-07-16 19:41:49.000000000 +1000
@@ -2005,9 +2005,9 @@
 		   "regex filter to identify parent, see: '--sort parent'"),
 	OPT_BOOLEAN('x', "exclude-other", &exclude_other,
 		    "Only display entries with parent-match"),
-	OPT_CALLBACK_DEFAULT('c', "callchain", NULL, "output_type,min_percent",
+	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL, "output_type,min_percent",
 		     "Display callchains using output_type and min percent threshold. "
-		     "Default: flat,0", &parse_callchain_opt, callchain_default_opt),
+		     "Default: fractal,0.5", &parse_callchain_opt, callchain_default_opt),
 	OPT_STRING('d', "dsos", &dso_list_str, "dso[,dso...]",
 		   "only consider symbols in these dsos"),
 	OPT_STRING('C', "comms", &comm_list_str, "comm[,comm...]",

-- 

Re: [patch 5/5] perf_counter: Make call graph option consistent

[Frederic Weisbecker]

On Thu, Jul 16, 2009 at 08:42:52PM +1000, Anton Blanchard wrote:
> perf record uses -g for logging call graph data but perf report uses -c
> to print call graph data. Be consistent and use -g everywhere for call graph
> data.
> 
> Also update the help text to reflect the current default - fractal,0.5
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>


Thanks!

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>


> ---
> 
> Im not sure if it's too late to make this change, but I've tripped
> up a number of times when I cycle between perf record and perf report.
> 
> Index: linux.trees.git/tools/perf/builtin-report.c
> ===================================================================
> --- linux.trees.git.orig/tools/perf/builtin-report.c	2009-07-16 19:41:41.000000000 +1000
> +++ linux.trees.git/tools/perf/builtin-report.c	2009-07-16 19:41:49.000000000 +1000
> @@ -2005,9 +2005,9 @@
>  		   "regex filter to identify parent, see: '--sort parent'"),
>  	OPT_BOOLEAN('x', "exclude-other", &exclude_other,
>  		    "Only display entries with parent-match"),
> -	OPT_CALLBACK_DEFAULT('c', "callchain", NULL, "output_type,min_percent",
> +	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL, "output_type,min_percent",
>  		     "Display callchains using output_type and min percent threshold. "
> -		     "Default: flat,0", &parse_callchain_opt, callchain_default_opt),
> +		     "Default: fractal,0.5", &parse_callchain_opt, callchain_default_opt),
>  	OPT_STRING('d', "dsos", &dso_list_str, "dso[,dso...]",
>  		   "only consider symbols in these dsos"),
>  	OPT_STRING('C', "comms", &comm_list_str, "comm[,comm...]",
> 
> -- 
> 

[tip:perfcounters/urgent] perf_counter: Make call graph option consistent

[tip-bot for Anton Blanchard]

Commit-ID:  1483b19f8f5e8ad0c8816de368b099322dad4db5
Gitweb:     http://git.kernel.org/tip/1483b19f8f5e8ad0c8816de368b099322dad4db5
Author:     Anton Blanchard <anton@samba.org>
AuthorDate: Thu, 16 Jul 2009 15:44:29 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 18 Jul 2009 11:21:33 +0200

perf_counter: Make call graph option consistent

perf record uses -g for logging call graph data but perf report
uses -c to print call graph data. Be consistent and use -g
everywhere for call graph data.

Also update the help text to reflect the current default -
fractal,0.5

Signed-off-by: Anton Blanchard <anton@samba.org>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20090716104817.803604373@samba.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/builtin-report.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4e5cc26..4b980cc 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1891,9 +1891,9 @@ static const struct option options[] = {
 		   "regex filter to identify parent, see: '--sort parent'"),
 	OPT_BOOLEAN('x', "exclude-other", &exclude_other,
 		    "Only display entries with parent-match"),
-	OPT_CALLBACK_DEFAULT('c', "callchain", NULL, "output_type,min_percent",
+	OPT_CALLBACK_DEFAULT('g', "call-graph", NULL, "output_type,min_percent",
 		     "Display callchains using output_type and min percent threshold. "
-		     "Default: flat,0", &parse_callchain_opt, callchain_default_opt),
+		     "Default: fractal,0.5", &parse_callchain_opt, callchain_default_opt),
 	OPT_STRING('d', "dsos", &dso_list_str, "dso[,dso...]",
 		   "only consider symbols in these dsos"),
 	OPT_STRING('C', "comms", &comm_list_str, "comm[,comm...]",

Re: [patch 0/5] Various perfcounter fixes

[Peter Zijlstra]

On Thu, 2009-07-16 at 20:42 +1000, Anton Blanchard wrote:
> Here are a few more perfcounter fixes, and one patch that changes the
> call graph option to be consistent between perf record and perf report.
> Perhaps I'm too late on that one, but I thought I'd give it a try :)

Nice catch on 1, and agreed on the others as well. Thanks!

I'll see what I can do to make this land in .31.