[PATCH 0/4] uprobes: Teach debug/tracing/uprobe_events to do the filtering

[PATCH 0/4] uprobes: Teach debug/tracing/uprobe_events to do the filtering

[Oleg Nesterov]

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.

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

[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 <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f3fd8ff..53afabe 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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;
 
-- 
1.5.5.1

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

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-01-27 20:48:33]:

> 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 <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index f3fd8ff..53afabe 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -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;
> 
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

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

[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 <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 53afabe..94d4ea2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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)
-- 
1.5.5.1

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

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-01-27 20:48:37]:

> 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 <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 53afabe..94d4ea2 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -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)
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

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

[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 <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   66 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 94d4ea2..f7a2dcb 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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);
 
-- 
1.5.5.1

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

[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 <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   48 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f7a2dcb..78e918f 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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);
 
-- 
1.5.5.1

Re: [PATCH 0/4] uprobes: Teach debug/tracing/uprobe_events to do the filtering

[Oleg Nesterov]

On 01/27, Oleg Nesterov wrote:
>
> 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:

> 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.