Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 1/4] ksym_tracer: Fix to make the tracer work

4 views
Skip to first unread message

Li Zefan

unread,
Dec 30, 2009, 1:30:02 AM12/30/09
to
ksym tracer doesn't work:

# echo tasklist_lock:rw- > ksym_trace_filter
-bash: echo: write error: No such device

It's because we pass to perf_event_create_kernel_counter()
a cpu number which is not present.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>
---
kernel/hw_breakpoint.c | 10 +++++++---
kernel/trace/trace_ksym.c | 1 -
2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index dbcbf6a..50dbd59 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -40,6 +40,7 @@
#include <linux/percpu.h>
#include <linux/sched.h>
#include <linux/init.h>
+#include <linux/cpu.h>
#include <linux/smp.h>

#include <linux/hw_breakpoint.h>
@@ -388,7 +389,8 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
if (!cpu_events)
return ERR_PTR(-ENOMEM);

- for_each_possible_cpu(cpu) {
+ get_online_cpus();
+ for_each_online_cpu(cpu) {
pevent = per_cpu_ptr(cpu_events, cpu);
bp = perf_event_create_kernel_counter(attr, cpu, -1, triggered);

@@ -399,18 +401,20 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
goto fail;
}
}
+ put_online_cpus();

return cpu_events;

fail:
- for_each_possible_cpu(cpu) {
+ for_each_online_cpu(cpu) {
pevent = per_cpu_ptr(cpu_events, cpu);
if (IS_ERR(*pevent))
break;
unregister_hw_breakpoint(*pevent);
}
+ put_online_cpus();
+
free_percpu(cpu_events);
- /* return the error if any */
return ERR_PTR(err);
}
EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index faf37fa..340b6ff 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -197,7 +197,6 @@ int process_new_ksym_entry(char *ksymname, int op, unsigned long addr)
entry->attr.bp_addr = addr;
entry->attr.bp_len = HW_BREAKPOINT_LEN_4;

- ret = -EAGAIN;
entry->ksym_hbp = register_wide_hw_breakpoint(&entry->attr,
ksym_hbp_handler);

--
1.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Li Zefan

unread,
Dec 30, 2009, 1:30:01 AM12/30/09
to
trace_stat is problematic. Don't use it, use seqfile instead.

This fixes a race that reading the stat file is not protected by
any lock, which can lead to use after free.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>
---

kernel/trace/trace_ksym.c | 127 ++++++++++++++++++---------------------------
1 files changed, 50 insertions(+), 77 deletions(-)

diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index 67d79f7..94103cd 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -26,7 +26,6 @@
#include <linux/fs.h>

#include "trace_output.h"
-#include "trace_stat.h"
#include "trace.h"

#include <linux/hw_breakpoint.h>
@@ -444,103 +443,77 @@ struct tracer ksym_tracer __read_mostly =
.print_line = ksym_trace_output
};

-__init static int init_ksym_trace(void)
-{
- struct dentry *d_tracer;
- struct dentry *entry;
-
- d_tracer = tracing_init_dentry();
- ksym_filter_entry_count = 0;
-
- entry = debugfs_create_file("ksym_trace_filter", 0644, d_tracer,
- NULL, &ksym_tracing_fops);
- if (!entry)
- pr_warning("Could not create debugfs "
- "'ksym_trace_filter' file\n");
-
- return register_tracer(&ksym_tracer);
-}
-device_initcall(init_ksym_trace);
-
-
#ifdef CONFIG_PROFILE_KSYM_TRACER
-static int ksym_tracer_stat_headers(struct seq_file *m)
+static int ksym_profile_show(struct seq_file *m, void *v)
{
+ struct hlist_node *node;
+ struct trace_ksym *entry;
+ int access_type = 0;
+ char fn_name[KSYM_NAME_LEN];
+
seq_puts(m, " Access Type ");
seq_puts(m, " Symbol Counter\n");
seq_puts(m, " ----------- ");
seq_puts(m, " ------ -------\n");
- return 0;
-}

-static int ksym_tracer_stat_show(struct seq_file *m, void *v)
-{
- struct hlist_node *stat = v;
- struct trace_ksym *entry;
- int access_type = 0;
- char fn_name[KSYM_NAME_LEN];
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(entry, node, &ksym_filter_head, ksym_hlist) {

- entry = hlist_entry(stat, struct trace_ksym, ksym_hlist);
+ access_type = entry->attr.bp_type;

- access_type = entry->attr.bp_type;
+ switch (access_type) {
+ case HW_BREAKPOINT_R:
+ seq_puts(m, " R ");
+ break;
+ case HW_BREAKPOINT_W:
+ seq_puts(m, " W ");
+ break;
+ case HW_BREAKPOINT_R | HW_BREAKPOINT_W:
+ seq_puts(m, " RW ");
+ break;
+ default:
+ seq_puts(m, " NA ");
+ }

- switch (access_type) {
- case HW_BREAKPOINT_R:
- seq_puts(m, " R ");
- break;
- case HW_BREAKPOINT_W:
- seq_puts(m, " W ");
- break;
- case HW_BREAKPOINT_R | HW_BREAKPOINT_W:
- seq_puts(m, " RW ");
- break;
- default:
- seq_puts(m, " NA ");
+ if (lookup_symbol_name(entry->attr.bp_addr, fn_name) >= 0)
+ seq_printf(m, " %-36s", fn_name);
+ else
+ seq_printf(m, " %-36s", "<NA>");
+ seq_printf(m, " %15llu\n",
+ (unsigned long long)atomic64_read(&entry->counter));
}
-
- if (lookup_symbol_name(entry->attr.bp_addr, fn_name) >= 0)
- seq_printf(m, " %-36s", fn_name);
- else
- seq_printf(m, " %-36s", "<NA>");
- seq_printf(m, " %15llu\n",
- (unsigned long long)atomic64_read(&entry->counter));
+ rcu_read_unlock();

return 0;
}

-static void *ksym_tracer_stat_start(struct tracer_stat *trace)
+static int ksym_profile_open(struct inode *node, struct file *file)
{
- return ksym_filter_head.first;
-}
-
-static void *
-ksym_tracer_stat_next(void *v, int idx)
-{
- struct hlist_node *stat = v;
-
- return stat->next;
+ return single_open(file, ksym_profile_show, NULL);
}

-static struct tracer_stat ksym_tracer_stats = {
- .name = "ksym_tracer",
- .stat_start = ksym_tracer_stat_start,
- .stat_next = ksym_tracer_stat_next,
- .stat_headers = ksym_tracer_stat_headers,
- .stat_show = ksym_tracer_stat_show
+static const struct file_operations ksym_profile_fops = {
+ .open = ksym_profile_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
};
+#endif /* CONFIG_PROFILE_KSYM_TRACER */

-__init static int ksym_tracer_stat_init(void)
+__init static int init_ksym_trace(void)
{
- int ret;
+ struct dentry *d_tracer;

- ret = register_stat_tracer(&ksym_tracer_stats);
- if (ret) {
- printk(KERN_WARNING "Warning: could not register "
- "ksym tracer stats\n");
- return 1;
- }
+ d_tracer = tracing_init_dentry();

- return 0;
+ trace_create_file("ksym_trace_filter", 0644, d_tracer,
+ NULL, &ksym_tracing_fops);
+
+#ifdef CONFIG_PROFILE_KSYM_TRACER
+ trace_create_file("ksym_profile", 0444, d_tracer,
+ NULL, &ksym_profile_fops);
+#endif
+
+ return register_tracer(&ksym_tracer);
}
-fs_initcall(ksym_tracer_stat_init);
-#endif /* CONFIG_PROFILE_KSYM_TRACER */
+device_initcall(init_ksym_trace);

Li Zefan

unread,
Dec 30, 2009, 1:30:02 AM12/30/09
to
It used to work, but now doesn't:

# echo > ksym_filter
bash: echo: write error: Invalid argument

It's caused by d954fbf0ff6b5fdfb32350e85a2f15d3db976506
("tracing: Fix wrong usage of strstrip in trace_ksyms").

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>
---

kernel/trace/trace_ksym.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index 340b6ff..160a8d8 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -299,8 +299,8 @@ static ssize_t ksym_trace_filter_write(struct file *file,
* 2: echo 0 > ksym_trace_filter
* 3: echo "*:---" > ksym_trace_filter
*/
- if (!buf[0] || !strcmp(buf, "0") ||
- !strcmp(buf, "*:---")) {
+ if (!input_string[0] || !strcmp(input_string, "0") ||
+ !strcmp(input_string, "*:---")) {
__ksym_trace_reset();
ret = 0;
goto out;

Li Zefan

unread,
Dec 30, 2009, 1:30:02 AM12/30/09
to
We are under rcu read section but not holding the write lock, so
count++ is not atomic. Use atomic64_t instead.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>
---

kernel/trace/trace_ksym.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index 160a8d8..67d79f7 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -32,6 +32,8 @@
#include <linux/hw_breakpoint.h>
#include <asm/hw_breakpoint.h>

+#include <asm/atomic.h>
+
/*
* For now, let us restrict the no. of symbols traced simultaneously to number
* of available hardware breakpoint registers.
@@ -44,7 +46,7 @@ struct trace_ksym {
struct perf_event **ksym_hbp;
struct perf_event_attr attr;
#ifdef CONFIG_PROFILE_KSYM_TRACER
- unsigned long counter;
+ atomic64_t counter;
#endif
struct hlist_node ksym_hlist;
};
@@ -69,9 +71,8 @@ void ksym_collect_stats(unsigned long hbp_hit_addr)

rcu_read_lock();
hlist_for_each_entry_rcu(entry, node, &ksym_filter_head, ksym_hlist) {
- if ((entry->attr.bp_addr == hbp_hit_addr) &&
- (entry->counter <= MAX_UL_INT)) {
- entry->counter++;
+ if (entry->attr.bp_addr == hbp_hit_addr) {
+ atomic64_inc(&entry->counter);
break;
}
}
@@ -501,7 +502,8 @@ static int ksym_tracer_stat_show(struct seq_file *m, void *v)
seq_printf(m, " %-36s", fn_name);
else


seq_printf(m, " %-36s", "<NA>");

- seq_printf(m, " %15lu\n", entry->counter);


+ seq_printf(m, " %15llu\n",
+ (unsigned long long)atomic64_read(&entry->counter));

return 0;

tip-bot for Li Zefan

unread,
Dec 30, 2009, 4:40:03 AM12/30/09
to
Commit-ID: 53ab668064edaeef99c0ee22799483d45f4c81f6
Gitweb: http://git.kernel.org/tip/53ab668064edaeef99c0ee22799483d45f4c81f6
Author: Li Zefan <li...@cn.fujitsu.com>
AuthorDate: Wed, 30 Dec 2009 14:24:03 +0800
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Wed, 30 Dec 2009 07:50:50 +0100

ksym_tracer: Remove trace_stat

trace_stat is problematic. Don't use it, use seqfile instead.

This fixes a race that reading the stat file is not protected by
any lock, which can lead to use after free.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>
Cc: Steven Rostedt <ros...@goodmis.org>
Cc: K.Prasad <pra...@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fwei...@gmail.com>
LKML-Reference: <4B3AF20...@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>

tip-bot for Li Zefan

unread,
Dec 30, 2009, 4:40:02 AM12/30/09
to
Commit-ID: 3d13ec2efdb5843ad91e57b60d50b44d922cf063
Gitweb: http://git.kernel.org/tip/3d13ec2efdb5843ad91e57b60d50b44d922cf063
Author: Li Zefan <li...@cn.fujitsu.com>
AuthorDate: Wed, 30 Dec 2009 14:23:19 +0800
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Wed, 30 Dec 2009 07:50:49 +0100

ksym_tracer: Fix to allow writing newline to ksym_trace_filter

It used to work, but now doesn't:

# echo > ksym_filter
bash: echo: write error: Invalid argument

It's caused by d954fbf0ff6b5fdfb32350e85a2f15d3db976506
("tracing: Fix wrong usage of strstrip in trace_ksyms").

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>


Cc: Steven Rostedt <ros...@goodmis.org>
Cc: K.Prasad <pra...@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fwei...@gmail.com>

LKML-Reference: <4B3AF1D7...@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>


---
kernel/trace/trace_ksym.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index 340b6ff..160a8d8 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -299,8 +299,8 @@ static ssize_t ksym_trace_filter_write(struct file *file,
* 2: echo 0 > ksym_trace_filter
* 3: echo "*:---" > ksym_trace_filter
*/
- if (!buf[0] || !strcmp(buf, "0") ||
- !strcmp(buf, "*:---")) {
+ if (!input_string[0] || !strcmp(input_string, "0") ||
+ !strcmp(input_string, "*:---")) {
__ksym_trace_reset();
ret = 0;
goto out;
--

tip-bot for Li Zefan

unread,
Dec 30, 2009, 4:40:01 AM12/30/09
to
Commit-ID: e6d9491bf8ba6728cc86aeabbc688d20ec0563b5
Gitweb: http://git.kernel.org/tip/e6d9491bf8ba6728cc86aeabbc688d20ec0563b5
Author: Li Zefan <li...@cn.fujitsu.com>
AuthorDate: Wed, 30 Dec 2009 14:23:40 +0800

Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Wed, 30 Dec 2009 07:50:49 +0100

ksym_tracer: Fix race when incrementing count

We are under rcu read section but not holding the write lock, so
count++ is not atomic. Use atomic64_t instead.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>


Cc: Steven Rostedt <ros...@goodmis.org>
Cc: K.Prasad <pra...@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fwei...@gmail.com>

LKML-Reference: <4B3AF1EC...@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>

tip-bot for Li Zefan

unread,
Dec 30, 2009, 4:40:02 AM12/30/09
to
Commit-ID: 88f7a890d74137ab0d126a5d65679cd620f1a289
Gitweb: http://git.kernel.org/tip/88f7a890d74137ab0d126a5d65679cd620f1a289
Author: Li Zefan <li...@cn.fujitsu.com>
AuthorDate: Wed, 30 Dec 2009 14:22:22 +0800
Committer: Ingo Molnar <mi...@elte.hu>
CommitDate: Wed, 30 Dec 2009 07:50:47 +0100

ksym_tracer: Fix to make the tracer work

ksym tracer doesn't work:

# echo tasklist_lock:rw- > ksym_trace_filter
-bash: echo: write error: No such device

It's because we pass to perf_event_create_kernel_counter()
a cpu number which is not present.

Signed-off-by: Li Zefan <li...@cn.fujitsu.com>


Cc: Steven Rostedt <ros...@goodmis.org>
Cc: K.Prasad <pra...@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fwei...@gmail.com>

LKML-Reference: <4B3AF19E...@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mi...@elte.hu>


---
kernel/hw_breakpoint.c | 10 +++++++---
kernel/trace/trace_ksym.c | 1 -
2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 366eedf..48fb0bb 100644

K.Prasad

unread,
Dec 30, 2009, 10:10:02 AM12/30/09
to

Frederic must have used for_each_possible_cpu() to account for CPUs that
are offline at the time of registration, but may eventually turn online.
Since register_wide_hw_breakpoint() interface is designed to deliver
system-wide breakpoints, the debug registers of a new online CPU will
should have the breakpoints populated to comprehensively notify all
memory accesses over target address.

I'd rather wait to hear from Frederic to know why
perf_event_create_kernel_counter() returns an error when run for an
offline cpu and how it can be solved.

Thanks,
K.Prasad

Li Zefan

unread,
Dec 31, 2009, 1:00:01 AM12/31/09
to

See the comment in find_get_context() in kernel/perf_event.c:

/*
* We could be clever and allow to attach a event to an
* offline CPU and activate it when the CPU comes up, but
* that's for later.
*/
if (!cpu_online(cpu))
return ERR_PTR(-ENODEV);

So I think we can use for_each_possible_cpu() in the future, but not now.

Frederic Weisbecker

unread,
Jan 9, 2010, 10:20:02 PM1/9/10
to
On Thu, Dec 31, 2009 at 01:54:35PM +0800, Li Zefan wrote:
> K.Prasad wrote:
> > Frederic must have used for_each_possible_cpu() to account for CPUs that
> > are offline at the time of registration, but may eventually turn online.
> > Since register_wide_hw_breakpoint() interface is designed to deliver
> > system-wide breakpoints, the debug registers of a new online CPU will
> > should have the breakpoints populated to comprehensively notify all
> > memory accesses over target address.
> >
> > I'd rather wait to hear from Frederic to know why
> > perf_event_create_kernel_counter() returns an error when run for an
> > offline cpu and how it can be solved.
> >
>
> See the comment in find_get_context() in kernel/perf_event.c:
>
> /*
> * We could be clever and allow to attach a event to an
> * offline CPU and activate it when the CPU comes up, but
> * that's for later.
> */
> if (!cpu_online(cpu))
> return ERR_PTR(-ENODEV);
>
> So I think we can use for_each_possible_cpu() in the future, but not now.
>


Ah, right I indeed missed that.

0 new messages