Skip to contentSkip to conversation

uprobes: Teach debug/tracing/uprobe_events to do the filtering

PATCH In review Oleg Nesterov 2 participants 8 messages Jan 27, 2013 – Jan 31, 2013 Tracing

lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

cover uprobes: Teach debug/tracing/uprobe_events to do the filtering

Hello.

Based on git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core on top of pre-filtering patches in kernel/uprobes.c.

Please review.

After that I'll try to change tools/perf to use this new feature.

Oleg.

author
#1 lore raw

PATCH 1/4 uprobes: Fix dentry/mount leak in create_trace_uprobe()

create_trace_uprobe() does kern_path() to find ->d_inode, but forgets to do path_put(). We can do this right after igrab().

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_uprobe.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/trace/trace_uprobe.c +3 -2
index f3fd8ff..53afabe 100644
@@ -255,12 +255,13 @@ static int create_trace_uprobe(int argc, char **argv)
 	if (ret)
 		goto fail_address_parse;
 
+	inode = igrab(path.dentry->d_inode);
+	path_put(&path);
+
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)
 		goto fail_address_parse;
 
-	inode = igrab(path.dentry->d_inode);
-
 	argc -= 2;
 	argv += 2;
Signature · 1 line
1.5.5.1
author
#3 lore raw

PATCH 2/4 uprobes: Fully initialize uprobe_trace_consumer before uprobe_register()

In reply to Oleg Nesterov

probe_event_enable() does uprobe_register() and only after that sets utc->tu and tu->consumer/flags. This can race with uprobe_dispatcher() which can miss these assignments or see them out of order. Nothing really bad can happen, but this doesn't look clean/safe.

And this does not allow to use uprobe_consumer->filter() we are going to add, it is called by uprobe_register() and it needs utc->tu.

Change this code to initialize everything before uprobe_register(), and reset tu->consumer/flags if it fails. We can't race with event_disable(), the caller holds event_mutex, and if we could the code would be wrong anyway.

In fact I think uprobe_trace_consumer should die, it buys nothing but complicate the code. We can simply add uprobe_consumer into trace_uprobe.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_uprobe.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
kernel/trace/trace_uprobe.c +7 -6
index 53afabe..94d4ea2 100644
@@ -552,17 +552,18 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 		return -EINTR;
 
 	utc->cons.handler = uprobe_dispatcher;
+	utc->tu = tu;
+	tu->consumer = utc;
+	tu->flags |= flag;
+
 	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
 	if (ret) {
+		tu->consumer = NULL;
+		tu->flags &= ~flag;
 		kfree(utc);
-		return ret;
 	}
 
-	tu->flags |= flag;
-	utc->tu = tu;
-	tu->consumer = utc;
-
-	return 0;
+	return ret;
 }
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
Signature · 1 line
1.5.5.1
author
#6 lore raw

PATCH 4/4 uprobes: Teach uprobe_trace_consumer to support the pre-filtering

In reply to Oleg Nesterov

Implement trace_uprobe_filter_func() and change probe_event_enable() to setup uprobe_consumer->filter() if necessary.

Also change uprobe_dispatcher() to skip the tasks we do not want to probe and return UPROBE_HANDLER_REMOVE to indicate that this "int3" should be removed unless there is another consumer which wants to trace this task.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_uprobe.c |   48 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)
kernel/trace/trace_uprobe.c +48 -0
index f7a2dcb..78e918f 100644
@@ -112,6 +112,50 @@ static void probes_seq_show_filter(struct trace_uprobe_filter *filter,
 		seq_printf(m, " pid=%d", pid_vnr(filter->tgid));
 }
 
+static bool trace_uprobe_filter_current(struct trace_uprobe_filter *filter)
+{
+	return !filter->tgid || filter->tgid == task_tgid(current);
+}
+
+static bool trace_uprobe_filter_func(struct uprobe_consumer *uc,
+					enum uprobe_filter_ctx ctx,
+					struct mm_struct *mm)
+{
+	struct uprobe_trace_consumer *utc;
+	struct trace_uprobe_filter *filter;
+	struct task_struct *p, *t;
+	bool ret;
+
+	utc = container_of(uc, struct uprobe_trace_consumer, cons);
+	filter = &utc->tu->filter;
+
+	if (ctx == UPROBE_FILTER_MMAP)
+		return trace_uprobe_filter_current(filter);
+
+	ret = false;
+	rcu_read_lock();
+	p = pid_task(filter->tgid, PIDTYPE_PID);
+	if (p) {
+		t = p;
+		do {
+			if (t->mm) {
+				ret = (t->mm == mm);
+				break;
+			}
+		} while_each_thread(p, t);
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static inline void set_trace_uprobe_filter_func(struct uprobe_consumer *uc,
+					struct trace_uprobe_filter *filter)
+{
+	if (filter->tgid)
+		uc->filter = trace_uprobe_filter_func;
+}
+
 /*
  * Allocate new trace_uprobe and initialize it (including uprobes).
  */

@@ -613,6 +657,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 	if (!utc)
 		return -EINTR;
 
+	set_trace_uprobe_filter_func(&utc->cons, &tu->filter);
 	utc->cons.handler = uprobe_dispatcher;
 	utc->tu = tu;
 	tu->consumer = utc;

@@ -784,6 +829,9 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 	if (!tu || tu->consumer != utc)
 		return 0;
 
+	if (!trace_uprobe_filter_current(&tu->filter))
+		return UPROBE_HANDLER_REMOVE;
+
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);
Signature · 1 line
1.5.5.1
author
#5 lore raw

PATCH 3/4 uprobes: Teach tracing/uprobe_events to accept pid=TGID argument

In reply to Oleg Nesterov

A separate change to simplify the review.

With this patch create_trace_uprobe() parses the new pid=TGID argument and stores the found "struct pid *" in "struct trace_uprobe" along with inode/offset.

However, to simplify the potential extensions, we add the new structure to hold this pid, and the set of its methods. This way we can, say, change this code to accept the list of pid's without touching the non-filtering code again.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_uprobe.c |   66 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 2 deletions(-)
kernel/trace/trace_uprobe.c +64 -2
index 94d4ea2..f7a2dcb 100644
@@ -23,6 +23,7 @@
 #include <linux/uprobes.h>
 #include <linux/namei.h>
 #include <linux/string.h>
+#include <linux/pid.h>
 
 #include "trace_probe.h"
 

@@ -37,11 +38,16 @@ struct uprobe_trace_consumer {
 	struct trace_uprobe		*tu;
 };
 
+struct trace_uprobe_filter {
+	struct pid *tgid;
+};
+
 struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
 	struct uprobe_trace_consumer	*consumer;
+	struct trace_uprobe_filter	filter;
 	struct inode			*inode;
 	char				*filename;
 	unsigned long			offset;

@@ -64,6 +70,48 @@ static LIST_HEAD(uprobe_list);
 
 static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
 
+static int init_trace_uprobe_filter(struct trace_uprobe_filter *filter,
+					const char *arg)
+{
+	struct task_struct *p;
+	pid_t tgid;
+
+	if (!arg)
+		return 0;
+
+	if (kstrtouint(arg, 0, &tgid))
+		goto err;
+
+	filter->tgid = find_get_pid(tgid);
+	if (!filter->tgid)
+		goto err;
+
+	rcu_read_lock();
+	p = pid_task(filter->tgid, PIDTYPE_PID);
+	if (!p || !has_group_leader_pid(p) || (p->flags & PF_KTHREAD))
+		p = NULL;
+	rcu_read_unlock();
+
+	if (p)
+		return 0;
+err:
+	pr_info("Failed to find the process by pid=%s\n", arg);
+	/* free_trace_uprobe() will take care of ->tgid != NULL */
+	return -ESRCH;
+}
+
+static void free_trace_uprobe_filter(struct trace_uprobe_filter *filter)
+{
+	put_pid(filter->tgid);
+}
+
+static void probes_seq_show_filter(struct trace_uprobe_filter *filter,
+				struct seq_file *m)
+{
+	if (filter->tgid)
+		seq_printf(m, " pid=%d", pid_vnr(filter->tgid));
+}
+
 /*
  * Allocate new trace_uprobe and initialize it (including uprobes).
  */

@@ -111,6 +159,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
 		traceprobe_free_probe_arg(&tu->args[i]);
 
 	iput(tu->inode);
+	free_trace_uprobe_filter(&tu->filter);
 	kfree(tu->call.class->system);
 	kfree(tu->call.name);
 	kfree(tu->filename);

@@ -167,7 +216,7 @@ end:
 
 /*
  * Argument syntax:
- *  - Add uprobe: p[:[GRP/]EVENT] PATH:SYMBOL[+offs] [FETCHARGS]
+ *  - Add uprobe: p[:[GRP/]EVENT] PATH:OFFSET [pid=TGID] [FETCHARGS]
  *
  *  - Remove uprobe: -:[GRP/]EVENT
  */

@@ -176,6 +225,7 @@ static int create_trace_uprobe(int argc, char **argv)
 	struct trace_uprobe *tu;
 	struct inode *inode;
 	char *arg, *event, *group, *filename;
+	char *filter_arg;
 	char buf[MAX_EVENT_NAME_LEN];
 	struct path path;
 	unsigned long offset;

@@ -265,6 +315,13 @@ static int create_trace_uprobe(int argc, char **argv)
 	argc -= 2;
 	argv += 2;
 
+	filter_arg = NULL;
+	if (argc && strncmp(argv[0], "pid=", 4) == 0) {
+		filter_arg = argv[0] + 4;
+		argc--;
+		argv++;
+	}
+
 	/* setup a probe */
 	if (!event) {
 		char *tail;

@@ -301,8 +358,11 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto error;
 	}
 
+	ret = init_trace_uprobe_filter(&tu->filter, filter_arg);
+	if (ret)
+		goto error;
+
 	/* parse arguments */
-	ret = 0;
 	for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
 		/* Increment count for freeing args in error case */
 		tu->nr_args++;

@@ -402,6 +462,8 @@ static int probes_seq_show(struct seq_file *m, void *v)
 	seq_printf(m, "p:%s/%s", tu->call.class->system, tu->call.name);
 	seq_printf(m, " %s:0x%p", tu->filename, (void *)tu->offset);
 
+	probes_seq_show_filter(&tu->filter, m);
+
 	for (i = 0; i < tu->nr_args; i++)
 		seq_printf(m, " %s=%s", tu->args[i].name, tu->args[i].comm);
Signature · 1 line
1.5.5.1
Acked-by on v1 1/4
#2 lore raw

Re: [PATCH 1/4] uprobes: Fix dentry/mount leak in create_trace_uprobe()

In reply to Oleg Nesterov
Quoted 4 lines · Oleg Nesterov

create_trace_uprobe() does kern_path() to find ->d_inode, but forgets to do path_put(). We can do this right after igrab().

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 27 lines
---
 kernel/trace/trace_uprobe.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/trace/trace_uprobe.c +3 -2
index f3fd8ff..53afabe 100644
@@ -255,12 +255,13 @@ static int create_trace_uprobe(int argc, char **argv)
 	if (ret)
 		goto fail_address_parse;
 
+	inode = igrab(path.dentry->d_inode);
+	path_put(&path);
+
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)
 		goto fail_address_parse;
 
-	inode = igrab(path.dentry->d_inode);
-
 	argc -= 2;
 	argv += 2;
Signature · 1 line
1.5.5.1
Signature · 2 lines
Thanks and Regards
Srikar Dronamraju
Acked-by on v1 2/4
#4 lore raw

Re: [PATCH 2/4] uprobes: Fully initialize uprobe_trace_consumer before uprobe_register()

In reply to Oleg Nesterov
Quoted 13 lines · Oleg Nesterov

probe_event_enable() does uprobe_register() and only after that sets utc->tu and tu->consumer/flags. This can race with uprobe_dispatcher() which can miss these assignments or see them out of order. Nothing really bad can happen, but this doesn't look clean/safe.

And this does not allow to use uprobe_consumer->filter() we are going to add, it is called by uprobe_register() and it needs utc->tu.

Change this code to initialize everything before uprobe_register(), and reset tu->consumer/flags if it fails. We can't race with event_disable(), the caller holds event_mutex, and if we could the code would be wrong anyway.

Quoted 4 lines

In fact I think uprobe_trace_consumer should die, it buys nothing but complicate the code. We can simply add uprobe_consumer into trace_uprobe.

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
Quoted 36 lines
---
 kernel/trace/trace_uprobe.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
kernel/trace/trace_uprobe.c +7 -6
index 53afabe..94d4ea2 100644
@@ -552,17 +552,18 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 		return -EINTR;
 
 	utc->cons.handler = uprobe_dispatcher;
+	utc->tu = tu;
+	tu->consumer = utc;
+	tu->flags |= flag;
+
 	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
 	if (ret) {
+		tu->consumer = NULL;
+		tu->flags &= ~flag;
 		kfree(utc);
-		return ret;
 	}
 
-	tu->flags |= flag;
-	utc->tu = tu;
-	tu->consumer = utc;
-
-	return 0;
+	return ret;
 }
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
Signature · 1 line
1.5.5.1
Signature · 2 lines
Thanks and Regards
Srikar Dronamraju
author
#7 lore raw
In reply to Oleg Nesterov
Quoted 5 lines · Oleg Nesterov

Based on git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core on top of pre-filtering patches in kernel/uprobes.c.

Please review.

Self-NACK.

Everything seems to work, including the additional cleanupe I sent, but:

Quoted 1 line

After that I'll try to change tools/perf to use this new feature.

this can't help perf.

I never used it before so I naively thought that it writes to tracing/uprobe_events and does perf_event_open(id) in the single session, but this is not true.

I'll try to do something else. It seems that we can use TRACE_REG_PERF_OPEN rather than TRACE_REG_PERF_REGISTER and exploit event->ctx->task->mm to implement the pre-filtering...

Oleg.