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