2009-12-07 04:06:25

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH 1/3] perf/sched: fix 'perf sched trace'

If we use 'perf sched trace', it will call symbol__init() again,
and can lead perf tool crash:

[root@localhost perf]# ./perf sched trace
*** glibc detected *** ./perf: free(): invalid next size (normal): 0x094c1898 ***
======= Backtrace: =========
/lib/libc.so.6[0xb7602404]
/lib/libc.so.6(cfree+0x96)[0xb76043b6]
./perf[0x80730fe]
./perf[0x8074c97]
./perf[0x805eb59]
./perf[0x80536fd]
./perf[0x804b618]
./perf[0x804bdc3]
/lib/libc.so.6(__libc_start_main+0xe5)[0xb75a9735]
./perf[0x804af81]
======= Memory map: ========
08048000-08158000 r-xp 00000000 fe:00 556831 /home/eric/....
08158000-08168000 rw-p 0010f000 fe:00 556831 /home/eric/...
08168000-085fe000 rw-p 00000000 00:00 0
094ab000-094cc000 rw-p 00000000 00:00 0 [heap]

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/builtin-sched.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 45c46c7..7481ebd 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1888,13 +1888,18 @@ static int __cmd_record(int argc, const char **argv)

int cmd_sched(int argc, const char **argv, const char *prefix __used)
{
- symbol__init(0);
-
argc = parse_options(argc, argv, sched_options, sched_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
if (!argc)
usage_with_options(sched_usage, sched_options);

+ /*
+ * Aliased to 'perf trace' for now:
+ */
+ if (!strcmp(argv[0], "trace"))
+ return cmd_trace(argc, argv, prefix);
+
+ symbol__init(0);
if (!strncmp(argv[0], "rec", 3)) {
return __cmd_record(argc, argv);
} else if (!strncmp(argv[0], "lat", 3)) {
@@ -1918,11 +1923,6 @@ int cmd_sched(int argc, const char **argv, const char *prefix __used)
usage_with_options(replay_usage, replay_options);
}
__cmd_replay();
- } else if (!strcmp(argv[0], "trace")) {
- /*
- * Aliased to 'perf trace' for now:
- */
- return cmd_trace(argc, argv, prefix);
} else {
usage_with_options(sched_usage, sched_options);
}
--
1.6.1.2


2009-12-07 04:08:05

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH 2/3] perf_event: fix for procing raw event

We use 'data.raw_data' parameter to call process_raw_event(), but
data.raw_data buffer not include data size. it can make perf tool
crash.

This bug is imported by:

Commit-ID: 180f95e29aa8782c019caa64ede2a28d8ab62564
Gitweb: http://git.kernel.org/tip/180f95e29aa8782c019caa64ede2a28d8ab62564
Author: OGAWA Hirofumi <[email protected]>
AuthorDate: Sun, 6 Dec 2009 20:08:24 +0900
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 6 Dec 2009 18:15:01 +0100

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/builtin-kmem.c | 11 ++++++++---
tools/perf/builtin-sched.c | 11 ++++++++---
2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index f218990..f84d7a3 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -289,13 +289,17 @@ static void process_free_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, u32 size, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
+ struct raw_event_sample *raw;
struct event *event;
int type;

+ raw = malloc_or_die(sizeof(*raw)+size);
+ raw->size = size;
+ memcpy(raw->data, data, size);
+
type = trace_parse_common_type(raw->data);
event = trace_find_event(type);

@@ -345,7 +349,8 @@ static int process_sample_event(event_t *event)

dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ data.time, thread);

return 0;
}
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 7481ebd..4655e16 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1570,13 +1570,17 @@ process_sched_migrate_task_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, u32 size, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
+ struct raw_event_sample *raw;
struct event *event;
int type;

+ raw = malloc_or_die(sizeof(*raw)+size);
+ raw->size = size;
+ memcpy(raw->data, data, size);
+
type = trace_parse_common_type(raw->data);
event = trace_find_event(type);

@@ -1629,7 +1633,8 @@ static int process_sample_event(event_t *event)
if (profile_cpu != -1 && profile_cpu != (int)data.cpu)
return 0;

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ data.time, thread);

return 0;
}
--
1.6.1.2


2009-12-07 04:08:51

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH 3/3] perf_event: fix __dsos__write_buildid_table()

The remain buff size is 'len - pos->long_name_len - 1', not
'len - pos->long_name_len + 1'

This bug is imported by:
Commit-ID: 7691b1ec2e4a8d4bd88dcf88b29792399ebe1c91
Gitweb: http://git.kernel.org/tip/7691b1ec2e4a8d4bd88dcf88b29792399ebe1c91
Author: OGAWA Hirofumi <[email protected]>
AuthorDate: Sun, 6 Dec 2009 20:10:49 +0900
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 6 Dec 2009 18:15:02 +0100

perf tools: Misc small fixes

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/util/header.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 08b6759..59a9c0b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -209,7 +209,7 @@ static int __dsos__write_buildid_table(struct list_head *head, int fd)
err = do_write(fd, pos->long_name, pos->long_name_len + 1);
if (err < 0)
return err;
- err = do_write(fd, zero_buf, len - pos->long_name_len + 1);
+ err = do_write(fd, zero_buf, len - pos->long_name_len - 1);
if (err < 0)
return err;
}
--
1.6.1.2

2009-12-07 04:55:51

by Xiao Guangrong

[permalink] [raw]
Subject: Re: [PATCH 2/3] perf_event: fix for procing raw event



Xiao Guangrong wrote:
> We use 'data.raw_data' parameter to call process_raw_event(), but
> data.raw_data buffer not include data size. it can make perf tool
> crash.
>

It seems raw->size is not use, I'll send a patch to cleanup it.

2009-12-07 05:05:41

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH] perf_event: fix for processing raw event - fix

raw->size is not used, this patch just cleanup it

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/builtin-kmem.c | 38 +++++++-----------
tools/perf/builtin-sched.c | 94 +++++++++++++++++++------------------------
2 files changed, 56 insertions(+), 76 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index f84d7a3..7551a5f 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -57,11 +57,6 @@ static struct rb_root root_caller_sorted;
static unsigned long total_requested, total_allocated;
static unsigned long nr_allocs, nr_cross_allocs;

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define PATH_SYS_NODE "/sys/devices/system/node"

static void init_cpunode_map(void)
@@ -201,7 +196,7 @@ static void insert_caller_stat(unsigned long call_site,
}
}

-static void process_alloc_event(struct raw_event_sample *raw,
+static void process_alloc_event(void *data,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -214,10 +209,10 @@ static void process_alloc_event(struct raw_event_sample *raw,
int bytes_alloc;
int node1, node2;

- ptr = raw_field_value(event, "ptr", raw->data);
- call_site = raw_field_value(event, "call_site", raw->data);
- bytes_req = raw_field_value(event, "bytes_req", raw->data);
- bytes_alloc = raw_field_value(event, "bytes_alloc", raw->data);
+ ptr = raw_field_value(event, "ptr", data);
+ call_site = raw_field_value(event, "call_site", data);
+ bytes_req = raw_field_value(event, "bytes_req", data);
+ bytes_alloc = raw_field_value(event, "bytes_alloc", data);

insert_alloc_stat(call_site, ptr, bytes_req, bytes_alloc, cpu);
insert_caller_stat(call_site, bytes_req, bytes_alloc);
@@ -227,7 +222,7 @@ static void process_alloc_event(struct raw_event_sample *raw,

if (node) {
node1 = cpunode_map[cpu];
- node2 = raw_field_value(event, "node", raw->data);
+ node2 = raw_field_value(event, "node", data);
if (node1 != node2)
nr_cross_allocs++;
}
@@ -262,7 +257,7 @@ static struct alloc_stat *search_alloc_stat(unsigned long ptr,
return NULL;
}

-static void process_free_event(struct raw_event_sample *raw,
+static void process_free_event(void *data,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -271,7 +266,7 @@ static void process_free_event(struct raw_event_sample *raw,
unsigned long ptr;
struct alloc_stat *s_alloc, *s_caller;

- ptr = raw_field_value(event, "ptr", raw->data);
+ ptr = raw_field_value(event, "ptr", data);

s_alloc = search_alloc_stat(ptr, 0, &root_alloc_stat, ptr_cmp);
if (!s_alloc)
@@ -289,35 +284,30 @@ static void process_free_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, u32 size, void *data,
+process_raw_event(event_t *raw_event __used, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw;
struct event *event;
int type;

- raw = malloc_or_die(sizeof(*raw)+size);
- raw->size = size;
- memcpy(raw->data, data, size);
-
- type = trace_parse_common_type(raw->data);
+ type = trace_parse_common_type(data);
event = trace_find_event(type);

if (!strcmp(event->name, "kmalloc") ||
!strcmp(event->name, "kmem_cache_alloc")) {
- process_alloc_event(raw, event, cpu, timestamp, thread, 0);
+ process_alloc_event(data, event, cpu, timestamp, thread, 0);
return;
}

if (!strcmp(event->name, "kmalloc_node") ||
!strcmp(event->name, "kmem_cache_alloc_node")) {
- process_alloc_event(raw, event, cpu, timestamp, thread, 1);
+ process_alloc_event(data, event, cpu, timestamp, thread, 1);
return;
}

if (!strcmp(event->name, "kfree") ||
!strcmp(event->name, "kmem_cache_free")) {
- process_free_event(raw, event, cpu, timestamp, thread);
+ process_free_event(data, event, cpu, timestamp, thread);
return;
}
}
@@ -349,7 +339,7 @@ static int process_sample_event(event_t *event)

dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);

- process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ process_raw_event(event, data.raw_data, data.cpu,
data.time, thread);

return 0;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 4655e16..19f43fa 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -628,11 +628,6 @@ static void test_calibrations(void)
printf("the sleep test took %Ld nsecs\n", T1-T0);
}

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define FILL_FIELD(ptr, field, event, data) \
ptr.field = (typeof(ptr.field)) raw_field_value(event, #field, data)

@@ -1356,7 +1351,7 @@ static void sort_lat(void)
static struct trace_sched_handler *trace_handler;

static void
-process_sched_wakeup_event(struct raw_event_sample *raw,
+process_sched_wakeup_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1364,13 +1359,13 @@ process_sched_wakeup_event(struct raw_event_sample *raw,
{
struct trace_wakeup_event wakeup_event;

- FILL_COMMON_FIELDS(wakeup_event, event, raw->data);
+ FILL_COMMON_FIELDS(wakeup_event, event, data);

- FILL_ARRAY(wakeup_event, comm, event, raw->data);
- FILL_FIELD(wakeup_event, pid, event, raw->data);
- FILL_FIELD(wakeup_event, prio, event, raw->data);
- FILL_FIELD(wakeup_event, success, event, raw->data);
- FILL_FIELD(wakeup_event, cpu, event, raw->data);
+ FILL_ARRAY(wakeup_event, comm, event, data);
+ FILL_FIELD(wakeup_event, pid, event, data);
+ FILL_FIELD(wakeup_event, prio, event, data);
+ FILL_FIELD(wakeup_event, success, event, data);
+ FILL_FIELD(wakeup_event, cpu, event, data);

if (trace_handler->wakeup_event)
trace_handler->wakeup_event(&wakeup_event, event, cpu, timestamp, thread);
@@ -1469,7 +1464,7 @@ map_switch_event(struct trace_switch_event *switch_event,


static void
-process_sched_switch_event(struct raw_event_sample *raw,
+process_sched_switch_event(void *data,
struct event *event,
int this_cpu,
u64 timestamp __used,
@@ -1477,15 +1472,15 @@ process_sched_switch_event(struct raw_event_sample *raw,
{
struct trace_switch_event switch_event;

- FILL_COMMON_FIELDS(switch_event, event, raw->data);
+ FILL_COMMON_FIELDS(switch_event, event, data);

- FILL_ARRAY(switch_event, prev_comm, event, raw->data);
- FILL_FIELD(switch_event, prev_pid, event, raw->data);
- FILL_FIELD(switch_event, prev_prio, event, raw->data);
- FILL_FIELD(switch_event, prev_state, event, raw->data);
- FILL_ARRAY(switch_event, next_comm, event, raw->data);
- FILL_FIELD(switch_event, next_pid, event, raw->data);
- FILL_FIELD(switch_event, next_prio, event, raw->data);
+ FILL_ARRAY(switch_event, prev_comm, event, data);
+ FILL_FIELD(switch_event, prev_pid, event, data);
+ FILL_FIELD(switch_event, prev_prio, event, data);
+ FILL_FIELD(switch_event, prev_state, event, data);
+ FILL_ARRAY(switch_event, next_comm, event, data);
+ FILL_FIELD(switch_event, next_pid, event, data);
+ FILL_FIELD(switch_event, next_prio, event, data);

if (curr_pid[this_cpu] != (u32)-1) {
/*
@@ -1502,7 +1497,7 @@ process_sched_switch_event(struct raw_event_sample *raw,
}

static void
-process_sched_runtime_event(struct raw_event_sample *raw,
+process_sched_runtime_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1510,17 +1505,17 @@ process_sched_runtime_event(struct raw_event_sample *raw,
{
struct trace_runtime_event runtime_event;

- FILL_ARRAY(runtime_event, comm, event, raw->data);
- FILL_FIELD(runtime_event, pid, event, raw->data);
- FILL_FIELD(runtime_event, runtime, event, raw->data);
- FILL_FIELD(runtime_event, vruntime, event, raw->data);
+ FILL_ARRAY(runtime_event, comm, event, data);
+ FILL_FIELD(runtime_event, pid, event, data);
+ FILL_FIELD(runtime_event, runtime, event, data);
+ FILL_FIELD(runtime_event, vruntime, event, data);

if (trace_handler->runtime_event)
trace_handler->runtime_event(&runtime_event, event, cpu, timestamp, thread);
}

static void
-process_sched_fork_event(struct raw_event_sample *raw,
+process_sched_fork_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1528,12 +1523,12 @@ process_sched_fork_event(struct raw_event_sample *raw,
{
struct trace_fork_event fork_event;

- FILL_COMMON_FIELDS(fork_event, event, raw->data);
+ FILL_COMMON_FIELDS(fork_event, event, data);

- FILL_ARRAY(fork_event, parent_comm, event, raw->data);
- FILL_FIELD(fork_event, parent_pid, event, raw->data);
- FILL_ARRAY(fork_event, child_comm, event, raw->data);
- FILL_FIELD(fork_event, child_pid, event, raw->data);
+ FILL_ARRAY(fork_event, parent_comm, event, data);
+ FILL_FIELD(fork_event, parent_pid, event, data);
+ FILL_ARRAY(fork_event, child_comm, event, data);
+ FILL_FIELD(fork_event, child_pid, event, data);

if (trace_handler->fork_event)
trace_handler->fork_event(&fork_event, event, cpu, timestamp, thread);
@@ -1550,7 +1545,7 @@ process_sched_exit_event(struct event *event,
}

static void
-process_sched_migrate_task_event(struct raw_event_sample *raw,
+process_sched_migrate_task_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1558,46 +1553,42 @@ process_sched_migrate_task_event(struct raw_event_sample *raw,
{
struct trace_migrate_task_event migrate_task_event;

- FILL_COMMON_FIELDS(migrate_task_event, event, raw->data);
+ FILL_COMMON_FIELDS(migrate_task_event, event, data);

- FILL_ARRAY(migrate_task_event, comm, event, raw->data);
- FILL_FIELD(migrate_task_event, pid, event, raw->data);
- FILL_FIELD(migrate_task_event, prio, event, raw->data);
- FILL_FIELD(migrate_task_event, cpu, event, raw->data);
+ FILL_ARRAY(migrate_task_event, comm, event, data);
+ FILL_FIELD(migrate_task_event, pid, event, data);
+ FILL_FIELD(migrate_task_event, prio, event, data);
+ FILL_FIELD(migrate_task_event, cpu, event, data);

if (trace_handler->migrate_task_event)
trace_handler->migrate_task_event(&migrate_task_event, event, cpu, timestamp, thread);
}

static void
-process_raw_event(event_t *raw_event __used, u32 size, void *data,
+process_raw_event(event_t *raw_event __used, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw;
struct event *event;
int type;

- raw = malloc_or_die(sizeof(*raw)+size);
- raw->size = size;
- memcpy(raw->data, data, size);

- type = trace_parse_common_type(raw->data);
+ type = trace_parse_common_type(data);
event = trace_find_event(type);

if (!strcmp(event->name, "sched_switch"))
- process_sched_switch_event(raw, event, cpu, timestamp, thread);
+ process_sched_switch_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_stat_runtime"))
- process_sched_runtime_event(raw, event, cpu, timestamp, thread);
+ process_sched_runtime_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_wakeup"))
- process_sched_wakeup_event(raw, event, cpu, timestamp, thread);
+ process_sched_wakeup_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_wakeup_new"))
- process_sched_wakeup_event(raw, event, cpu, timestamp, thread);
+ process_sched_wakeup_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_process_fork"))
- process_sched_fork_event(raw, event, cpu, timestamp, thread);
+ process_sched_fork_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_process_exit"))
process_sched_exit_event(event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_migrate_task"))
- process_sched_migrate_task_event(raw, event, cpu, timestamp, thread);
+ process_sched_migrate_task_event(data, event, cpu, timestamp, thread);
}

static int process_sample_event(event_t *event)
@@ -1633,8 +1624,7 @@ static int process_sample_event(event_t *event)
if (profile_cpu != -1 && profile_cpu != (int)data.cpu)
return 0;

- process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
- data.time, thread);
+ process_raw_event(event, data.raw_data, data.cpu, data.time, thread);

return 0;
}
--
1.6.1.2

2009-12-07 05:19:08

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH 3/3] perf_event: fix __dsos__write_buildid_table()

Xiao Guangrong <[email protected]> writes:

> The remain buff size is 'len - pos->long_name_len - 1', not
> 'len - pos->long_name_len + 1'
>
> This bug is imported by:
> Commit-ID: 7691b1ec2e4a8d4bd88dcf88b29792399ebe1c91
> Gitweb: http://git.kernel.org/tip/7691b1ec2e4a8d4bd88dcf88b29792399ebe1c91
> Author: OGAWA Hirofumi <[email protected]>
> AuthorDate: Sun, 6 Dec 2009 20:10:49 +0900
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Sun, 6 Dec 2009 18:15:02 +0100
>
> perf tools: Misc small fixes

Oops, I forgot parens. Thanks for fixing it.

> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> tools/perf/util/header.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 08b6759..59a9c0b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -209,7 +209,7 @@ static int __dsos__write_buildid_table(struct list_head *head, int fd)
> err = do_write(fd, pos->long_name, pos->long_name_len + 1);
> if (err < 0)
> return err;
> - err = do_write(fd, zero_buf, len - pos->long_name_len + 1);
> + err = do_write(fd, zero_buf, len - pos->long_name_len - 1);
> if (err < 0)
> return err;
> }

--
OGAWA Hirofumi <[email protected]>

2009-12-07 05:32:58

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/urgent] perf/sched: Fix 'perf sched trace'

Commit-ID: c0777c5aa835a97ccc77d82e55388940f0140a61
Gitweb: http://git.kernel.org/tip/c0777c5aa835a97ccc77d82e55388940f0140a61
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 7 Dec 2009 12:04:49 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 7 Dec 2009 06:26:22 +0100

perf/sched: Fix 'perf sched trace'

If we use 'perf sched trace', it will call symbol__init() again,
and can lead to a perf tool crash:

[root@localhost perf]# ./perf sched trace
*** glibc detected *** ./perf: free(): invalid next size (normal): 0x094c1898 ***
======= Backtrace: =========
/lib/libc.so.6[0xb7602404]
/lib/libc.so.6(cfree+0x96)[0xb76043b6]
./perf[0x80730fe]
./perf[0x8074c97]
./perf[0x805eb59]
./perf[0x80536fd]
./perf[0x804b618]
./perf[0x804bdc3]
/lib/libc.so.6(__libc_start_main+0xe5)[0xb75a9735]
./perf[0x804af81]
======= Memory map: ========
08048000-08158000 r-xp 00000000 fe:00 556831 /home/eric/....
08158000-08168000 rw-p 0010f000 fe:00 556831 /home/eric/...
08168000-085fe000 rw-p 00000000 00:00 0
094ab000-094cc000 rw-p 00000000 00:00 0 [heap]

Signed-off-by: Xiao Guangrong <[email protected]>
LKML-Reference: <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/builtin-sched.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 45c46c7..7481ebd 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1888,13 +1888,18 @@ static int __cmd_record(int argc, const char **argv)

int cmd_sched(int argc, const char **argv, const char *prefix __used)
{
- symbol__init(0);
-
argc = parse_options(argc, argv, sched_options, sched_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
if (!argc)
usage_with_options(sched_usage, sched_options);

+ /*
+ * Aliased to 'perf trace' for now:
+ */
+ if (!strcmp(argv[0], "trace"))
+ return cmd_trace(argc, argv, prefix);
+
+ symbol__init(0);
if (!strncmp(argv[0], "rec", 3)) {
return __cmd_record(argc, argv);
} else if (!strncmp(argv[0], "lat", 3)) {
@@ -1918,11 +1923,6 @@ int cmd_sched(int argc, const char **argv, const char *prefix __used)
usage_with_options(replay_usage, replay_options);
}
__cmd_replay();
- } else if (!strcmp(argv[0], "trace")) {
- /*
- * Aliased to 'perf trace' for now:
- */
- return cmd_trace(argc, argv, prefix);
} else {
usage_with_options(sched_usage, sched_options);
}

2009-12-07 05:33:08

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/urgent] perf_event: Fix raw event processing

Commit-ID: d8bd9e0aedabcb47887712497bc386a06ddcbd12
Gitweb: http://git.kernel.org/tip/d8bd9e0aedabcb47887712497bc386a06ddcbd12
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 7 Dec 2009 12:06:29 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 7 Dec 2009 06:26:24 +0100

perf_event: Fix raw event processing

We use 'data.raw_data' parameter to call process_raw_event(),
but data.raw_data buffer not include data size. it can make perf
tool crash.

This bug was introduced by commit 180f95e29a ("perf: Make common
SAMPLE_EVENT parser").

Signed-off-by: Xiao Guangrong <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Eduard - Gabriel Munteanu <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: OGAWA Hirofumi <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Li Zefan <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/builtin-kmem.c | 11 ++++++++---
tools/perf/builtin-sched.c | 11 ++++++++---
2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index f218990..f84d7a3 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -289,13 +289,17 @@ static void process_free_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, u32 size, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
+ struct raw_event_sample *raw;
struct event *event;
int type;

+ raw = malloc_or_die(sizeof(*raw)+size);
+ raw->size = size;
+ memcpy(raw->data, data, size);
+
type = trace_parse_common_type(raw->data);
event = trace_find_event(type);

@@ -345,7 +349,8 @@ static int process_sample_event(event_t *event)

dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ data.time, thread);

return 0;
}
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 7481ebd..4655e16 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1570,13 +1570,17 @@ process_sched_migrate_task_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, u32 size, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
+ struct raw_event_sample *raw;
struct event *event;
int type;

+ raw = malloc_or_die(sizeof(*raw)+size);
+ raw->size = size;
+ memcpy(raw->data, data, size);
+
type = trace_parse_common_type(raw->data);
event = trace_find_event(type);

@@ -1629,7 +1633,8 @@ static int process_sample_event(event_t *event)
if (profile_cpu != -1 && profile_cpu != (int)data.cpu)
return 0;

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ data.time, thread);

return 0;
}

2009-12-07 05:31:56

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/urgent] perf_event: Fix __dsos__write_buildid_table()

Commit-ID: d9541ed3241bb6c2b805d3ea0e87563cf2a0c5c3
Gitweb: http://git.kernel.org/tip/d9541ed3241bb6c2b805d3ea0e87563cf2a0c5c3
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 7 Dec 2009 12:07:15 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 7 Dec 2009 06:26:24 +0100

perf_event: Fix __dsos__write_buildid_table()

The remain buff size is 'len - pos->long_name_len - 1', not
'len - pos->long_name_len + 1'

This bug was introduced by commit 7691b1e ("perf tools: Misc small
fixes").

Signed-off-by: Xiao Guangrong <[email protected]>
Acked-by: OGAWA Hirofumi <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Li Zefan <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/util/header.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 08b6759..59a9c0b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -209,7 +209,7 @@ static int __dsos__write_buildid_table(struct list_head *head, int fd)
err = do_write(fd, pos->long_name, pos->long_name_len + 1);
if (err < 0)
return err;
- err = do_write(fd, zero_buf, len - pos->long_name_len + 1);
+ err = do_write(fd, zero_buf, len - pos->long_name_len - 1);
if (err < 0)
return err;
}

2009-12-07 05:32:44

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/urgent] perf_event: Eliminate raw->size

Commit-ID: f48f669d42e133db839af16656fd720107ef6742
Gitweb: http://git.kernel.org/tip/f48f669d42e133db839af16656fd720107ef6742
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 7 Dec 2009 13:04:04 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 7 Dec 2009 06:26:25 +0100

perf_event: Eliminate raw->size

raw->size is not used, this patch just cleans it up.

Signed-off-by: Xiao Guangrong <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: OGAWA Hirofumi <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Li Zefan <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/builtin-kmem.c | 38 +++++++-----------
tools/perf/builtin-sched.c | 94 +++++++++++++++++++------------------------
2 files changed, 56 insertions(+), 76 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index f84d7a3..7551a5f 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -57,11 +57,6 @@ static struct rb_root root_caller_sorted;
static unsigned long total_requested, total_allocated;
static unsigned long nr_allocs, nr_cross_allocs;

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define PATH_SYS_NODE "/sys/devices/system/node"

static void init_cpunode_map(void)
@@ -201,7 +196,7 @@ static void insert_caller_stat(unsigned long call_site,
}
}

-static void process_alloc_event(struct raw_event_sample *raw,
+static void process_alloc_event(void *data,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -214,10 +209,10 @@ static void process_alloc_event(struct raw_event_sample *raw,
int bytes_alloc;
int node1, node2;

- ptr = raw_field_value(event, "ptr", raw->data);
- call_site = raw_field_value(event, "call_site", raw->data);
- bytes_req = raw_field_value(event, "bytes_req", raw->data);
- bytes_alloc = raw_field_value(event, "bytes_alloc", raw->data);
+ ptr = raw_field_value(event, "ptr", data);
+ call_site = raw_field_value(event, "call_site", data);
+ bytes_req = raw_field_value(event, "bytes_req", data);
+ bytes_alloc = raw_field_value(event, "bytes_alloc", data);

insert_alloc_stat(call_site, ptr, bytes_req, bytes_alloc, cpu);
insert_caller_stat(call_site, bytes_req, bytes_alloc);
@@ -227,7 +222,7 @@ static void process_alloc_event(struct raw_event_sample *raw,

if (node) {
node1 = cpunode_map[cpu];
- node2 = raw_field_value(event, "node", raw->data);
+ node2 = raw_field_value(event, "node", data);
if (node1 != node2)
nr_cross_allocs++;
}
@@ -262,7 +257,7 @@ static struct alloc_stat *search_alloc_stat(unsigned long ptr,
return NULL;
}

-static void process_free_event(struct raw_event_sample *raw,
+static void process_free_event(void *data,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -271,7 +266,7 @@ static void process_free_event(struct raw_event_sample *raw,
unsigned long ptr;
struct alloc_stat *s_alloc, *s_caller;

- ptr = raw_field_value(event, "ptr", raw->data);
+ ptr = raw_field_value(event, "ptr", data);

s_alloc = search_alloc_stat(ptr, 0, &root_alloc_stat, ptr_cmp);
if (!s_alloc)
@@ -289,35 +284,30 @@ static void process_free_event(struct raw_event_sample *raw,
}

static void
-process_raw_event(event_t *raw_event __used, u32 size, void *data,
+process_raw_event(event_t *raw_event __used, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw;
struct event *event;
int type;

- raw = malloc_or_die(sizeof(*raw)+size);
- raw->size = size;
- memcpy(raw->data, data, size);
-
- type = trace_parse_common_type(raw->data);
+ type = trace_parse_common_type(data);
event = trace_find_event(type);

if (!strcmp(event->name, "kmalloc") ||
!strcmp(event->name, "kmem_cache_alloc")) {
- process_alloc_event(raw, event, cpu, timestamp, thread, 0);
+ process_alloc_event(data, event, cpu, timestamp, thread, 0);
return;
}

if (!strcmp(event->name, "kmalloc_node") ||
!strcmp(event->name, "kmem_cache_alloc_node")) {
- process_alloc_event(raw, event, cpu, timestamp, thread, 1);
+ process_alloc_event(data, event, cpu, timestamp, thread, 1);
return;
}

if (!strcmp(event->name, "kfree") ||
!strcmp(event->name, "kmem_cache_free")) {
- process_free_event(raw, event, cpu, timestamp, thread);
+ process_free_event(data, event, cpu, timestamp, thread);
return;
}
}
@@ -349,7 +339,7 @@ static int process_sample_event(event_t *event)

dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);

- process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
+ process_raw_event(event, data.raw_data, data.cpu,
data.time, thread);

return 0;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 4655e16..19f43fa 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -628,11 +628,6 @@ static void test_calibrations(void)
printf("the sleep test took %Ld nsecs\n", T1-T0);
}

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define FILL_FIELD(ptr, field, event, data) \
ptr.field = (typeof(ptr.field)) raw_field_value(event, #field, data)

@@ -1356,7 +1351,7 @@ static void sort_lat(void)
static struct trace_sched_handler *trace_handler;

static void
-process_sched_wakeup_event(struct raw_event_sample *raw,
+process_sched_wakeup_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1364,13 +1359,13 @@ process_sched_wakeup_event(struct raw_event_sample *raw,
{
struct trace_wakeup_event wakeup_event;

- FILL_COMMON_FIELDS(wakeup_event, event, raw->data);
+ FILL_COMMON_FIELDS(wakeup_event, event, data);

- FILL_ARRAY(wakeup_event, comm, event, raw->data);
- FILL_FIELD(wakeup_event, pid, event, raw->data);
- FILL_FIELD(wakeup_event, prio, event, raw->data);
- FILL_FIELD(wakeup_event, success, event, raw->data);
- FILL_FIELD(wakeup_event, cpu, event, raw->data);
+ FILL_ARRAY(wakeup_event, comm, event, data);
+ FILL_FIELD(wakeup_event, pid, event, data);
+ FILL_FIELD(wakeup_event, prio, event, data);
+ FILL_FIELD(wakeup_event, success, event, data);
+ FILL_FIELD(wakeup_event, cpu, event, data);

if (trace_handler->wakeup_event)
trace_handler->wakeup_event(&wakeup_event, event, cpu, timestamp, thread);
@@ -1469,7 +1464,7 @@ map_switch_event(struct trace_switch_event *switch_event,


static void
-process_sched_switch_event(struct raw_event_sample *raw,
+process_sched_switch_event(void *data,
struct event *event,
int this_cpu,
u64 timestamp __used,
@@ -1477,15 +1472,15 @@ process_sched_switch_event(struct raw_event_sample *raw,
{
struct trace_switch_event switch_event;

- FILL_COMMON_FIELDS(switch_event, event, raw->data);
+ FILL_COMMON_FIELDS(switch_event, event, data);

- FILL_ARRAY(switch_event, prev_comm, event, raw->data);
- FILL_FIELD(switch_event, prev_pid, event, raw->data);
- FILL_FIELD(switch_event, prev_prio, event, raw->data);
- FILL_FIELD(switch_event, prev_state, event, raw->data);
- FILL_ARRAY(switch_event, next_comm, event, raw->data);
- FILL_FIELD(switch_event, next_pid, event, raw->data);
- FILL_FIELD(switch_event, next_prio, event, raw->data);
+ FILL_ARRAY(switch_event, prev_comm, event, data);
+ FILL_FIELD(switch_event, prev_pid, event, data);
+ FILL_FIELD(switch_event, prev_prio, event, data);
+ FILL_FIELD(switch_event, prev_state, event, data);
+ FILL_ARRAY(switch_event, next_comm, event, data);
+ FILL_FIELD(switch_event, next_pid, event, data);
+ FILL_FIELD(switch_event, next_prio, event, data);

if (curr_pid[this_cpu] != (u32)-1) {
/*
@@ -1502,7 +1497,7 @@ process_sched_switch_event(struct raw_event_sample *raw,
}

static void
-process_sched_runtime_event(struct raw_event_sample *raw,
+process_sched_runtime_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1510,17 +1505,17 @@ process_sched_runtime_event(struct raw_event_sample *raw,
{
struct trace_runtime_event runtime_event;

- FILL_ARRAY(runtime_event, comm, event, raw->data);
- FILL_FIELD(runtime_event, pid, event, raw->data);
- FILL_FIELD(runtime_event, runtime, event, raw->data);
- FILL_FIELD(runtime_event, vruntime, event, raw->data);
+ FILL_ARRAY(runtime_event, comm, event, data);
+ FILL_FIELD(runtime_event, pid, event, data);
+ FILL_FIELD(runtime_event, runtime, event, data);
+ FILL_FIELD(runtime_event, vruntime, event, data);

if (trace_handler->runtime_event)
trace_handler->runtime_event(&runtime_event, event, cpu, timestamp, thread);
}

static void
-process_sched_fork_event(struct raw_event_sample *raw,
+process_sched_fork_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1528,12 +1523,12 @@ process_sched_fork_event(struct raw_event_sample *raw,
{
struct trace_fork_event fork_event;

- FILL_COMMON_FIELDS(fork_event, event, raw->data);
+ FILL_COMMON_FIELDS(fork_event, event, data);

- FILL_ARRAY(fork_event, parent_comm, event, raw->data);
- FILL_FIELD(fork_event, parent_pid, event, raw->data);
- FILL_ARRAY(fork_event, child_comm, event, raw->data);
- FILL_FIELD(fork_event, child_pid, event, raw->data);
+ FILL_ARRAY(fork_event, parent_comm, event, data);
+ FILL_FIELD(fork_event, parent_pid, event, data);
+ FILL_ARRAY(fork_event, child_comm, event, data);
+ FILL_FIELD(fork_event, child_pid, event, data);

if (trace_handler->fork_event)
trace_handler->fork_event(&fork_event, event, cpu, timestamp, thread);
@@ -1550,7 +1545,7 @@ process_sched_exit_event(struct event *event,
}

static void
-process_sched_migrate_task_event(struct raw_event_sample *raw,
+process_sched_migrate_task_event(void *data,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1558,46 +1553,42 @@ process_sched_migrate_task_event(struct raw_event_sample *raw,
{
struct trace_migrate_task_event migrate_task_event;

- FILL_COMMON_FIELDS(migrate_task_event, event, raw->data);
+ FILL_COMMON_FIELDS(migrate_task_event, event, data);

- FILL_ARRAY(migrate_task_event, comm, event, raw->data);
- FILL_FIELD(migrate_task_event, pid, event, raw->data);
- FILL_FIELD(migrate_task_event, prio, event, raw->data);
- FILL_FIELD(migrate_task_event, cpu, event, raw->data);
+ FILL_ARRAY(migrate_task_event, comm, event, data);
+ FILL_FIELD(migrate_task_event, pid, event, data);
+ FILL_FIELD(migrate_task_event, prio, event, data);
+ FILL_FIELD(migrate_task_event, cpu, event, data);

if (trace_handler->migrate_task_event)
trace_handler->migrate_task_event(&migrate_task_event, event, cpu, timestamp, thread);
}

static void
-process_raw_event(event_t *raw_event __used, u32 size, void *data,
+process_raw_event(event_t *raw_event __used, void *data,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw;
struct event *event;
int type;

- raw = malloc_or_die(sizeof(*raw)+size);
- raw->size = size;
- memcpy(raw->data, data, size);

- type = trace_parse_common_type(raw->data);
+ type = trace_parse_common_type(data);
event = trace_find_event(type);

if (!strcmp(event->name, "sched_switch"))
- process_sched_switch_event(raw, event, cpu, timestamp, thread);
+ process_sched_switch_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_stat_runtime"))
- process_sched_runtime_event(raw, event, cpu, timestamp, thread);
+ process_sched_runtime_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_wakeup"))
- process_sched_wakeup_event(raw, event, cpu, timestamp, thread);
+ process_sched_wakeup_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_wakeup_new"))
- process_sched_wakeup_event(raw, event, cpu, timestamp, thread);
+ process_sched_wakeup_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_process_fork"))
- process_sched_fork_event(raw, event, cpu, timestamp, thread);
+ process_sched_fork_event(data, event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_process_exit"))
process_sched_exit_event(event, cpu, timestamp, thread);
if (!strcmp(event->name, "sched_migrate_task"))
- process_sched_migrate_task_event(raw, event, cpu, timestamp, thread);
+ process_sched_migrate_task_event(data, event, cpu, timestamp, thread);
}

static int process_sample_event(event_t *event)
@@ -1633,8 +1624,7 @@ static int process_sample_event(event_t *event)
if (profile_cpu != -1 && profile_cpu != (int)data.cpu)
return 0;

- process_raw_event(event, data.raw_size, data.raw_data, data.cpu,
- data.time, thread);
+ process_raw_event(event, data.raw_data, data.cpu, data.time, thread);

return 0;
}

2009-12-07 06:33:27

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix for processing raw event - fix

Xiao Guangrong <[email protected]> writes:

> raw->size is not used, this patch just cleanup it

Oops, I didn't notice those. Thanks.

> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> tools/perf/builtin-kmem.c | 38 +++++++-----------
> tools/perf/builtin-sched.c | 94 +++++++++++++++++++------------------------
> 2 files changed, 56 insertions(+), 76 deletions(-)
>
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index f84d7a3..7551a5f 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -57,11 +57,6 @@ static struct rb_root root_caller_sorted;
> static unsigned long total_requested, total_allocated;
> static unsigned long nr_allocs, nr_cross_allocs;
>
> -struct raw_event_sample {
> - u32 size;
> - char data[0];
> -};
> -
> #define PATH_SYS_NODE "/sys/devices/system/node"
>
> static void init_cpunode_map(void)
> @@ -201,7 +196,7 @@ static void insert_caller_stat(unsigned long call_site,
> }
> }
>
> -static void process_alloc_event(struct raw_event_sample *raw,
> +static void process_alloc_event(void *data,
> struct event *event,
> int cpu,
> u64 timestamp __used,

How about the following patch instead of playing with unsafe "void *"?
With this, I guess it does type check, and filed handling functions can
check "size" by passing "struct sample_raw_data" instead of "void *" in
future.

[Sorry, this patch was almost compiled only, and tested slightly.]

Thanks.
--
OGAWA Hirofumi <[email protected]>



Signed-off-by: OGAWA Hirofumi <[email protected]>
---

tools/perf/builtin-kmem.c | 14 ++++----------
tools/perf/builtin-sched.c | 20 +++++++-------------
tools/perf/builtin-timechart.c | 4 ++--
tools/perf/builtin-trace.c | 4 ++--
tools/perf/util/event.c | 8 ++------
tools/perf/util/event.h | 8 ++++++--
6 files changed, 23 insertions(+), 35 deletions(-)

diff -puN tools/perf/util/event.h~perf-raw_sample tools/perf/util/event.h
--- linux-2.6/tools/perf/util/event.h~perf-raw_sample 2009-12-07 14:48:09.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/util/event.h 2009-12-07 15:25:51.000000000 +0900
@@ -61,6 +61,11 @@ struct sample_event {
u64 array[];
};

+struct sample_raw_data {
+ u32 size;
+ unsigned char data[];
+};
+
struct sample_data {
u64 ip;
u32 pid, tid;
@@ -71,8 +76,7 @@ struct sample_data {
u32 cpu;
u64 period;
struct ip_callchain *callchain;
- u32 raw_size;
- void *raw_data;
+ struct sample_raw_data *raw;
};

#define BUILD_ID_SIZE 20
diff -puN tools/perf/builtin-sched.c~perf-raw_sample tools/perf/builtin-sched.c
--- linux-2.6/tools/perf/builtin-sched.c~perf-raw_sample 2009-12-07 14:49:09.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/builtin-sched.c 2009-12-07 14:58:04.000000000 +0900
@@ -628,11 +628,6 @@ static void test_calibrations(void)
printf("the sleep test took %Ld nsecs\n", T1-T0);
}

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define FILL_FIELD(ptr, field, event, data) \
ptr.field = (typeof(ptr.field)) raw_field_value(event, #field, data)

@@ -1356,7 +1351,7 @@ static void sort_lat(void)
static struct trace_sched_handler *trace_handler;

static void
-process_sched_wakeup_event(struct raw_event_sample *raw,
+process_sched_wakeup_event(struct sample_raw_data *raw,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1469,7 +1464,7 @@ map_switch_event(struct trace_switch_eve


static void
-process_sched_switch_event(struct raw_event_sample *raw,
+process_sched_switch_event(struct sample_raw_data *raw,
struct event *event,
int this_cpu,
u64 timestamp __used,
@@ -1502,7 +1497,7 @@ process_sched_switch_event(struct raw_ev
}

static void
-process_sched_runtime_event(struct raw_event_sample *raw,
+process_sched_runtime_event(struct sample_raw_data *raw,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1520,7 +1515,7 @@ process_sched_runtime_event(struct raw_e
}

static void
-process_sched_fork_event(struct raw_event_sample *raw,
+process_sched_fork_event(struct sample_raw_data *raw,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1550,7 +1545,7 @@ process_sched_exit_event(struct event *e
}

static void
-process_sched_migrate_task_event(struct raw_event_sample *raw,
+process_sched_migrate_task_event(struct sample_raw_data *raw,
struct event *event,
int cpu __used,
u64 timestamp __used,
@@ -1570,10 +1565,9 @@ process_sched_migrate_task_event(struct
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, struct sample_raw_data *raw,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
struct event *event;
int type;

@@ -1629,7 +1623,7 @@ static int process_sample_event(event_t
if (profile_cpu != -1 && profile_cpu != (int)data.cpu)
return 0;

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw, data.cpu, data.time, thread);

return 0;
}
diff -puN tools/perf/builtin-trace.c~perf-raw_sample tools/perf/builtin-trace.c
--- linux-2.6/tools/perf/builtin-trace.c~perf-raw_sample 2009-12-07 14:50:25.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/builtin-trace.c 2009-12-07 14:50:53.000000000 +0900
@@ -95,8 +95,8 @@ static int process_sample_event(event_t
* field, although it should be the same than this perf
* event pid
*/
- scripting_ops->process_event(data.cpu, data.raw_data,
- data.raw_size,
+ scripting_ops->process_event(data.cpu, data.raw->data,
+ data.raw->size,
data.time, thread->comm);
}
event__stats.total += data.period;
diff -puN tools/perf/builtin-timechart.c~perf-raw_sample tools/perf/builtin-timechart.c
--- linux-2.6/tools/perf/builtin-timechart.c~perf-raw_sample 2009-12-07 14:51:05.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/builtin-timechart.c 2009-12-07 14:51:26.000000000 +0900
@@ -497,8 +497,8 @@ process_sample_event(event_t *event)
last_time = data.time;
}

- te = (void *)data.raw_data;
- if (sample_type & PERF_SAMPLE_RAW && data.raw_size > 0) {
+ te = (void *)data.raw->data;
+ if (sample_type & PERF_SAMPLE_RAW && data.raw->size > 0) {
char *event_str;
struct power_entry *pe;

diff -puN tools/perf/builtin-kmem.c~perf-raw_sample tools/perf/builtin-kmem.c
--- linux-2.6/tools/perf/builtin-kmem.c~perf-raw_sample 2009-12-07 14:51:38.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/builtin-kmem.c 2009-12-07 14:53:57.000000000 +0900
@@ -57,11 +57,6 @@ static struct rb_root root_caller_sorted
static unsigned long total_requested, total_allocated;
static unsigned long nr_allocs, nr_cross_allocs;

-struct raw_event_sample {
- u32 size;
- char data[0];
-};
-
#define PATH_SYS_NODE "/sys/devices/system/node"

static void init_cpunode_map(void)
@@ -201,7 +196,7 @@ static void insert_caller_stat(unsigned
}
}

-static void process_alloc_event(struct raw_event_sample *raw,
+static void process_alloc_event(struct sample_raw_data *raw,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -262,7 +257,7 @@ static struct alloc_stat *search_alloc_s
return NULL;
}

-static void process_free_event(struct raw_event_sample *raw,
+static void process_free_event(struct sample_raw_data *raw,
struct event *event,
int cpu,
u64 timestamp __used,
@@ -289,10 +284,9 @@ static void process_free_event(struct ra
}

static void
-process_raw_event(event_t *raw_event __used, void *more_data,
+process_raw_event(event_t *raw_event __used, struct sample_raw_data *raw,
int cpu, u64 timestamp, struct thread *thread)
{
- struct raw_event_sample *raw = more_data;
struct event *event;
int type;

@@ -345,7 +339,7 @@ static int process_sample_event(event_t

dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);

- process_raw_event(event, data.raw_data, data.cpu, data.time, thread);
+ process_raw_event(event, data.raw, data.cpu, data.time, thread);

return 0;
}
diff -puN tools/perf/util/event.c~perf-raw_sample tools/perf/util/event.c
--- linux-2.6/tools/perf/util/event.c~perf-raw_sample 2009-12-07 14:52:56.000000000 +0900
+++ linux-2.6-hirofumi/tools/perf/util/event.c 2009-12-07 14:53:06.000000000 +0900
@@ -368,12 +368,8 @@ int event__parse_sample(event_t *event,
array += 1 + data->callchain->nr;
}

- if (type & PERF_SAMPLE_RAW) {
- u32 *p = (u32 *)array;
- data->raw_size = *p;
- p++;
- data->raw_data = p;
- }
+ if (type & PERF_SAMPLE_RAW)
+ data->raw = (struct sample_raw_data *)array;

return 0;
}
_

2009-12-07 06:35:56

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix for processing raw event - fix

OGAWA Hirofumi <[email protected]> writes:

> Xiao Guangrong <[email protected]> writes:
>
>> raw->size is not used, this patch just cleanup it
>
> Oops, I didn't notice those. Thanks.
>
>> Signed-off-by: Xiao Guangrong <[email protected]>
>> ---
>> tools/perf/builtin-kmem.c | 38 +++++++-----------
>> tools/perf/builtin-sched.c | 94 +++++++++++++++++++------------------------
>> 2 files changed, 56 insertions(+), 76 deletions(-)
>>
>> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
>> index f84d7a3..7551a5f 100644
>> --- a/tools/perf/builtin-kmem.c
>> +++ b/tools/perf/builtin-kmem.c
>> @@ -57,11 +57,6 @@ static struct rb_root root_caller_sorted;
>> static unsigned long total_requested, total_allocated;
>> static unsigned long nr_allocs, nr_cross_allocs;
>>
>> -struct raw_event_sample {
>> - u32 size;
>> - char data[0];
>> -};
>> -
>> #define PATH_SYS_NODE "/sys/devices/system/node"
>>
>> static void init_cpunode_map(void)
>> @@ -201,7 +196,7 @@ static void insert_caller_stat(unsigned long call_site,
>> }
>> }
>>
>> -static void process_alloc_event(struct raw_event_sample *raw,
>> +static void process_alloc_event(void *data,
>> struct event *event,
>> int cpu,
>> u64 timestamp __used,
>
> How about the following patch instead of playing with unsafe "void *"?
> With this, I guess it does type check, and filed handling functions can

s/filed handling/field handling of sample raw data/

> check "size" by passing "struct sample_raw_data" instead of "void *" in
> future.
--
OGAWA Hirofumi <[email protected]>

2009-12-07 08:14:19

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [tip:perf/urgent] perf_event: Eliminate raw->size

On Mon, 2009-12-07 at 05:31 +0000, tip-bot for Xiao Guangrong wrote:
> Commit-ID: f48f669d42e133db839af16656fd720107ef6742
> Gitweb: http://git.kernel.org/tip/f48f669d42e133db839af16656fd720107ef6742
> Author: Xiao Guangrong <[email protected]>
> AuthorDate: Mon, 7 Dec 2009 13:04:04 +0800
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Mon, 7 Dec 2009 06:26:25 +0100
>
> perf_event: Eliminate raw->size
>
> raw->size is not used, this patch just cleans it up.

This patch feels wrong.. I would suspect it to be used to validate the
data parsing, like do we consume past the end and did we consume
everything.

2009-12-07 08:31:44

by Xiao Guangrong

[permalink] [raw]
Subject: Re: [tip:perf/urgent] perf_event: Eliminate raw->size



Peter Zijlstra wrote:
> On Mon, 2009-12-07 at 05:31 +0000, tip-bot for Xiao Guangrong wrote:
>> Commit-ID: f48f669d42e133db839af16656fd720107ef6742
>> Gitweb: http://git.kernel.org/tip/f48f669d42e133db839af16656fd720107ef6742
>> Author: Xiao Guangrong <[email protected]>
>> AuthorDate: Mon, 7 Dec 2009 13:04:04 +0800
>> Committer: Ingo Molnar <[email protected]>
>> CommitDate: Mon, 7 Dec 2009 06:26:25 +0100
>>
>> perf_event: Eliminate raw->size
>>
>> raw->size is not used, this patch just cleans it up.
>
> This patch feels wrong.. I would suspect it to be used to validate the
> data parsing, like do we consume past the end and did we consume
> everything.
>

But as i noticed in current code, it just use raw->data, maybe i miss something?
what i do is just using 'data' instead of 'raw->data' :-(

Thanks,
Xiao

>
>

2009-12-07 08:46:16

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [tip:perf/urgent] perf_event: Eliminate raw->size

Xiao Guangrong <[email protected]> writes:

> Peter Zijlstra wrote:
>> On Mon, 2009-12-07 at 05:31 +0000, tip-bot for Xiao Guangrong wrote:
>>> Commit-ID: f48f669d42e133db839af16656fd720107ef6742
>>> Gitweb: http://git.kernel.org/tip/f48f669d42e133db839af16656fd720107ef6742
>>> Author: Xiao Guangrong <[email protected]>
>>> AuthorDate: Mon, 7 Dec 2009 13:04:04 +0800
>>> Committer: Ingo Molnar <[email protected]>
>>> CommitDate: Mon, 7 Dec 2009 06:26:25 +0100
>>>
>>> perf_event: Eliminate raw->size
>>>
>>> raw->size is not used, this patch just cleans it up.
>>
>> This patch feels wrong.. I would suspect it to be used to validate the
>> data parsing, like do we consume past the end and did we consume
>> everything.
>>
>
> But as i noticed in current code, it just use raw->data, maybe i miss something?
> what i do is just using 'data' instead of 'raw->data' :-(

Well, it's not user fault at all though. To pass "struct sample_raw_data"
instead of "void *" to raw_field_value/raw_field_ptr/etc., then bounds
check of sample_raw_data would be sane. It is what I intended...

Thanks.
--
OGAWA Hirofumi <[email protected]>