Re: [PATCH 1/3] ftrace: add function tracing to single thread

Steven Rostedt <rostedt@xxxxxxxxxxx> writes:

On Tue, 25 Nov 2008, Dave Hansen wrote:

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.

+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?

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

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


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.

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.

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.

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.

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

Relevant Pages

  • Re: [PATCH 1/3] ftrace: add function tracing to single thread
    ... I think the end result was, if this file can only be changed by root, then ... One thing this doesn't deal with is pid wraparound. ... 'struct pid' for a particular task by doing a find_get_pid. ... context of the current task's pid namespace, which is what you want, I ...
    (Linux-Kernel)
  • Re: exiting chroot()
    ... > Only root can ptrace another process. ... ## Args: ebx -> status ... # ptrace_attach (pid) failed ...
    (comp.unix.programmer)
  • Re: [PATCH 1/3] ftrace: add function tracing to single thread
    ... I think the end result was, if this file can only be changed by root, then ... One thing this doesn't deal with is pid wraparound. ... This is just a way to trace a particular process. ... you should save a 'struct pid'. ...
    (Linux-Kernel)
  • Re: [PATCH] cleanup find_task_by_pid variants
    ... modular caller of he old pre pid namespace find_task_by_pid (gfs2) ... was switched to pid_task which operates on a struct pid pointer instead ... Given the confusion about pid_t values vs namespace that's ...
    (Linux-Kernel)
  • Re: [PATCH -v2] ftrace: Documentation
    ... task_pid(tsk) yields the struct pid. ... struct pid is interesting because it is immune from pid roll over conflicts. ... stuct pid contains an embedded array of the pid_nrs one for each namespace the ... I think ftrace should be updated before 2.6.27 is ...
    (Linux-Kernel)