ftrace: new features

This patch adds the ability to function trace a single thread. The file:

/debugfs/tracing/set_ftrace_pid

contains the pid to trace.

Quoted 6 lines · On Tue, 25 Nov 2008 17:34:22 -0500 Steven Rostedt <[email protected]> wrote:

This patch adds the ability to function trace a single thread. The file:

/debugfs/tracing/set_ftrace_pid

contains the pid to trace.

What happens if the same pid exists in two or more pid namespaces?

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Tue, 25 Nov 2008 17:34:22 -0500 Steven Rostedt <[email protected]> wrote:

This patch adds the ability to function trace a single thread. The file:

Quoted 5 lines

Quoted 3 lines

/debugfs/tracing/set_ftrace_pid

contains the pid to trace.

What happens if the same pid exists in two or more pid namespaces?

I think we had this discussion before.

It tests current->pid, would that
be different among the name spaces?

Quoted 9 lines · On Tue, 25 Nov 2008 18:01:06 -0500 (EST) Steven Rostedt <[email protected]>…

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Tue, 25 Nov 2008 17:34:22 -0500 Steven Rostedt <[email protected]> wrote:

This patch adds the ability to function trace a single thread. The file:

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

/debugfs/tracing/set_ftrace_pid

contains the pid to trace.

What happens if the same pid exists in two or more pid namespaces?

I think we had this discussion before.

Oh. What did we end up concluding?

Quoted 2 lines

It tests current->pid, would that
be different among the name spaces?

I think those are non-unique. [email protected] would have better ideas..

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines · Steven Rostedt

This patch adds the ability to function trace a single thread. The file:

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

/debugfs/tracing/set_ftrace_pid

contains the pid to trace.

What happens if the same pid exists in two or more pid namespaces?

I think we had this discussion before.

Quoted 7 lines

Oh. What did we end up concluding?

Quoted 2 lines

It tests current->pid, would that
be different among the name spaces?

I think those are non-unique. [email protected] would have better ideas..

Added list in CC.

I think the end result was, if this file can only be changed by root, then we do not need to worry about namespaces. This file is a privileged file that can only be modified by root.

If someday we decide to let non admin users touch this file, then we would need to care about this. This file may actually be modified in the future by users, so this may become an issue.

Quoted 8 lines

I think the end result was, if this file can only be changed by root, then we do not need to worry about namespaces. This file is a privileged file that can only be modified by root.

If someday we decide to let non admin users touch this file, then we would need to care about this. This file may actually be modified in the future by users, so this may become an issue.

This really has very little to do with root vs non-root users. In fact, we're working towards having cases where we have many "root" users, even those inside namespaces. It is also quite possible for a normal root user to fork into a new pid namespace. In that case, root simply won't be able to use this feature because something like:

echo $$ /debugfs/tracing/set_ftrace_pid

just won't work. Let's look at a bit of the code.

kernel/trace/ftrace.c +7 -0

+static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
+{
+       if (current->pid != ftrace_pid_trace)
+               return;
+
+       ftrace_pid_function(ip, parent_ip);
+}

One thing this doesn't deal with is pid wraparound. Does that matter?

If you want to fix this a bit, instead of saving off the pid_t in ftrace_pid_trace, you should save a 'struct pid'. You can get the 'struct pid' for a particular task by doing a find_get_pid(pid_t). You can then compare that pid_t to current by doing a pid_task(struct_pid_that_i_saved, PIDTYPE_PID). That will also protect against pid wraparound.

The find_get_pid() is handy because it will do the pid_t lookup in the context of the current task's pid namespace, which is what you want, I think.

Quoted 2 lines · Eric W. Biederman

I'm speechless too.

Quoted 3 lines · Steven Rostedt

On Tue, 25 Nov 2008, Eric W. Biederman wrote:

I'm speechless too.

I'm a bit tired so probably am pushing to hard.

At the same time I don't see a single reason not to use struct pid for what it was designed for. Identifying tasks. pid_t's really only belong at the border.

I can see in the tracer when grabbing numbers you might not be able to follow pointers. For that I see justification for using task->pid. For the comparison I just don't see it.

i dont see the point of the complexity you are advocating. 99.9% of the users run a unique PID space.

Tracing is about keeping stuff simple. On containers we could also trace the namespace ID (is there an easy ID for the namespace, as an easy extension to the very nice PID concept that Unix introduced decades ago?) and be done with it.

Quoted 2 lines · Ingo Molnar

i dont see the point of the complexity you are advocating. 99.9% of the users run a unique PID space.

I'm not advocating complexity. I'm advocating using the same APIs as the rest of the kernel, for doing the same functions.

Quoted 4 lines

Tracing is about keeping stuff simple. On containers we could also trace the namespace ID (is there an easy ID for the namespace, as an easy extension to the very nice PID concept that Unix introduced decades ago?) and be done with it.

I don't really care about the pid namespace in this context.

I am just asking that we compare a different field in the task struct.

I am asking that we don't accumulate new users of an old crufty bug prone API, for no good reason.

Quoted 18 lines · Eric W. Biederman

Quoted 2 lines · Ingo Molnar

i dont see the point of the complexity you are advocating. 99.9% of the users run a unique PID space.

I'm not advocating complexity. I'm advocating using the same APIs as the rest of the kernel, for doing the same functions.

Quoted 4 lines

Tracing is about keeping stuff simple. On containers we could also trace the namespace ID (is there an easy ID for the namespace, as an easy extension to the very nice PID concept that Unix introduced decades ago?) and be done with it.

I don't really care about the pid namespace in this context.

I am just asking that we compare a different field in the task struct.

Quoted 2 lines

I am asking that we don't accumulate new users of an old crufty bug prone API, for no good reason.

i dont disagree about the change, but i'm curious, what's bug-prone about current->pid? It certainly worked quite well for the first 15 years.

Quoted 27 lines · Dave Hansen

Quoted 8 lines

I think the end result was, if this file can only be changed by root, then we do not need to worry about namespaces. This file is a privileged file that can only be modified by root.

If someday we decide to let non admin users touch this file, then we would need to care about this. This file may actually be modified in the future by users, so this may become an issue.

This really has very little to do with root vs non-root users. In fact, we're working towards having cases where we have many "root" users, even those inside namespaces. It is also quite possible for a normal root user to fork into a new pid namespace. In that case, root simply won't be able to use this feature because something like:

echo $$ /debugfs/tracing/set_ftrace_pid

just won't work. Let's look at a bit of the code.

(quoted diff) +7 -0

+static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
+{
+       if (current->pid != ftrace_pid_trace)
+               return;
+
+       ftrace_pid_function(ip, parent_ip);
+}

Quoted 1 line

One thing this doesn't deal with is pid wraparound. Does that matter?

Should not. This is just a way to trace a particular process. Currently it traces all processes. If we wrap, then we trace the process with the new pid. This should not be an issue.

Quoted 7 lines

If you want to fix this a bit, instead of saving off the pid_t in ftrace_pid_trace, you should save a 'struct pid'. You can get the 'struct pid' for a particular task by doing a find_get_pid(pid_t). You can then compare that pid_t to current by doing a pid_task(struct_pid_that_i_saved, PIDTYPE_PID). That will also protect against pid wraparound.

Quoted 3 lines

The find_get_pid() is handy because it will do the pid_t lookup in the context of the current task's pid namespace, which is what you want, I think.

Nope, we can not call that in this context. ftrace_pid_func is called directly from mcount, without any protection.

struct pid *find_get_pid(pid_t nr)

{
	struct pid *pid;

	rcu_read_lock();
	pid = get_pid(find_vpid(nr));
	rcu_read_unlock();

	return pid;
}

EXPORT_SYMBOL_GPL(find_get_pid);

This means find_get_pid will call mcount which will call ftrace_pid_func, and back again. This can also happen with rcu_read_{un}lock() and get_pid() and find_vpid().

We can not do anything special here.

Quoted 28 lines · Steven Rostedt

Quoted 27 lines · Dave Hansen

Quoted 8 lines

I think the end result was, if this file can only be changed by root, then we do not need to worry about namespaces. This file is a privileged file that can only be modified by root.

If someday we decide to let non admin users touch this file, then we would need to care about this. This file may actually be modified in the future by users, so this may become an issue.

This really has very little to do with root vs non-root users. In fact, we're working towards having cases where we have many "root" users, even those inside namespaces. It is also quite possible for a normal root user to fork into a new pid namespace. In that case, root simply won't be able to use this feature because something like:

echo $$ /debugfs/tracing/set_ftrace_pid

just won't work. Let's look at a bit of the code.

(quoted diff) +7 -0

+static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
+{
+       if (current->pid != ftrace_pid_trace)
+               return;
+
+       ftrace_pid_function(ip, parent_ip);
+}

Quoted 5 lines

Quoted 1 line

One thing this doesn't deal with is pid wraparound. Does that matter?

Should not. This is just a way to trace a particular process. Currently it traces all processes. If we wrap, then we trace the process with the new pid. This should not be an issue.

So. Using raw pid numbers in the kernel is bad form. The internal representation should be struct pid pointers as much as we can make them.

I would 100% prefer it if ftrace_pid_func was written in terms of struct pid. That does guarantee you don't have pid wrap around issues. It almost makes it clear

(quoted diff) +7 -0

+static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
+{
+	if (task_pid(current) == ftrace_pid_trace)
+               return;
+
+       ftrace_pid_function(ip, parent_ip);
+}

We don't need locks to access the pid of current.

Quoted 9 lines

Quoted 9 lines

If you want to fix this a bit, instead of saving off the pid_t in ftrace_pid_trace, you should save a 'struct pid'. You can get the 'struct pid' for a particular task by doing a find_get_pid(pid_t). You can then compare that pid_t to current by doing a pid_task(struct_pid_that_i_saved, PIDTYPE_PID). That will also protect against pid wraparound.

Quoted 6 lines

Quoted 3 lines

The find_get_pid() is handy because it will do the pid_t lookup in the context of the current task's pid namespace, which is what you want, I think.

Nope, we can not call that in this context. ftrace_pid_func is called directly from mcount, without any protection.

Of course you can't. But at the same time find_get_pid() is always supposed to be called on the user space pid ingress path.

Quoted 10 lines

struct pid *find_get_pid(pid_t nr)

{
	struct pid *pid;

	rcu_read_lock();
	pid = get_pid(find_vpid(nr));
	rcu_read_unlock();

	return pid;
}

Quoted 7 lines

EXPORT_SYMBOL_GPL(find_get_pid);

This means find_get_pid will call mcount which will call ftrace_pid_func, and back again. This can also happen with rcu_read_{un}lock() and get_pid() and find_vpid().

We can not do anything special here.

I don't see the whole path. But here is the deal.
1) Using struct pid and the proper find_get_pid() means that a user with the proper capabilities/permissions who happens to be running in a pid namespace can call this and it will just work.

2) The current best practices in the current are to:
- call find_get_pid() when you capture a user space pid.
- call put_pid() when you are done with it.

   Perhaps that is just:
   put_pid(ftrace_pid_trace);
   ftrace_pid_trace = find_get_pid(user_provided_pid_value);

3) If you ultimately want to support the full gamut:
thread/process/process group/session. You will need to use struct pid pointer comparisons.

4) When I looked at the place you were concerned about races
a) you were concerned about the wrong race.
b) I don't see a race.
c) You were filtering for the tid of a linux task not the tgid of a process. So the code didn't seem to be doing what you thought it was doing.

5) I keep thinking current->pid should be removed some day.

So let's do this properly if we can. This is a privileged operation
so we don't need to support people without the proper capabilities doing this. Or multiple comparisons or anything silly like that. But doing this with struct pid comparisons seems cleaner and more maintainable. And that really should matter.

From: Steven Rostedt <[email protected]>

Impact: feature to function trace a single thread

This patch adds the ability to function trace a single thread. The file:

/debugfs/tracing/set_ftrace_pid

2008/11/25 Steven Rostedt <[email protected]>:

Quoted 8 lines

From: Steven Rostedt <[email protected]>

Impact: feature to function trace a single thread

This patch adds the ability to function trace a single thread. The file:

/debugfs/tracing/set_ftrace_pid

That's a good idea!

About the pid, I think they are unique (which is not the case for tgid for multiple threads).
pid are tgid + number of the thread....

But perhaps I'm wrong....

From: Steven Rostedt <[email protected]>

Impact: more efficient code for ftrace return tracer

This patch uses the dynamic patching, when available, to patch the function return code into the kernel.

This patch will ease the way for letting both function tracing and function return tracing run together.

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kernel/entry_32.S |    5 ++++
 arch/x86/kernel/ftrace.c   |   46 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/ftrace.h     |    5 ++++
 kernel/trace/ftrace.c      |   34 ++++++++++++++------------------
 4 files changed, 70 insertions(+), 20 deletions(-)

arch/x86/kernel/entry_32.S +0 -0

index 7a934c0..d6f0333 100644

From: Steven Rostedt <[email protected]>
int __init ftrace_dyn_arch_init(void *data)
{

arch/x86/kernel/ftrace.c +0 -0

 	extern const unsigned char ftrace_test_p6nop[];

include/linux/ftrace.h +0 -0

index 7854d87..73147eb 100644