[PATCH] ftrace: do_each_pid_task() needs rcu lock

[PATCH] ftrace: do_each_pid_task() needs rcu lock

[Oleg Nesterov]

"ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
converted ftrace_pid_trace to "struct pid*". But we can't use
do_each_pid_task() without rcu_read_lock() even if we know the pid
itself can't go away (it was pinned in ftrace_pid_write). The exiting
task can detach itself from this pid at any moment.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/trace/ftrace.c~FTRACE_PID	2009-01-12 23:07:51.000000000 +0100
+++ 6.29-rc3/kernel/trace/ftrace.c	2009-02-03 20:23:59.000000000 +0100
@@ -1736,9 +1736,12 @@ static void clear_ftrace_pid(struct pid 
 {
 	struct task_struct *p;
 
+	rcu_read_lock();
 	do_each_pid_task(pid, PIDTYPE_PID, p) {
 		clear_tsk_trace_trace(p);
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	rcu_read_unlock();
+
 	put_pid(pid);
 }
 
@@ -1746,9 +1749,11 @@ static void set_ftrace_pid(struct pid *p
 {
 	struct task_struct *p;
 
+	rcu_read_lock();
 	do_each_pid_task(pid, PIDTYPE_PID, p) {
 		set_tsk_trace_trace(p);
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	rcu_read_unlock();
 }
 
 static void clear_ftrace_pid_task(struct pid **pid)

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Oleg Nesterov]

On 02/03, Oleg Nesterov wrote:
>
> "ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
> converted ftrace_pid_trace to "struct pid*". But we can't use
> do_each_pid_task() without rcu_read_lock() even if we know the pid
> itself can't go away (it was pinned in ftrace_pid_write). The exiting
> task can detach itself from this pid at any moment.

Q: why do we use do_each_pid_task(PIDTYPE_PID) ? We can never have more
than 1 task in the loop. Perhaps,

	static void set_ftrace_pid(struct pid *pid)
	{
		struct task_struct *p;

		rcu_read_lock();
		p = pid_task(pid, PIDTYPE_PID);
		if (p)
			set_tsk_trace_trace(p);
		rcu_read_unlock();
	}

looks better?

Oleg.

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/03, Oleg Nesterov wrote:
>>
>> "ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
>> converted ftrace_pid_trace to "struct pid*". But we can't use
>> do_each_pid_task() without rcu_read_lock() even if we know the pid
>> itself can't go away (it was pinned in ftrace_pid_write). The exiting
>> task can detach itself from this pid at any moment.
>
> Q: why do we use do_each_pid_task(PIDTYPE_PID) ? We can never have more
> than 1 task in the loop. Perhaps,

That is a bug in do_each_pid_task(PIDTYPE_PID).
For ftrace we really want to grab all tasks with a given pid even
in the crazy exec case.

Eric

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> "ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
> converted ftrace_pid_trace to "struct pid*". But we can't use
> do_each_pid_task() without rcu_read_lock() even if we know the pid
> itself can't go away (it was pinned in ftrace_pid_write). The exiting
> task can detach itself from this pid at any moment.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Applied to tip/tracing/urgent (for v2.6.29), thanks Oleg!

	Ingo

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Oleg Nesterov]

On 02/03, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 02/03, Oleg Nesterov wrote:
> >>
> >> "ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
> >> converted ftrace_pid_trace to "struct pid*". But we can't use
> >> do_each_pid_task() without rcu_read_lock() even if we know the pid
> >> itself can't go away (it was pinned in ftrace_pid_write). The exiting
> >> task can detach itself from this pid at any moment.
> >
> > Q: why do we use do_each_pid_task(PIDTYPE_PID) ? We can never have more
> > than 1 task in the loop. Perhaps,
>
> That is a bug in do_each_pid_task(PIDTYPE_PID).
> For ftrace we really want to grab all tasks with a given pid even
> in the crazy exec case.

Yes, I thought about de_thread() too. But we can't "fix" do_each_pid_task()
to avoid the race?

IOW. If we want to continue to trace the task with the same pid after
exec reliably, then we should do something like

	void ftrace_transfer_trace(struct task_struct *leader)
	{
		mutex_lock(&ftrace_start_lock);
		if (test_tsk_trace_trace(leader))
			set_tsk_trace_trace(current);
		mutex_unlock(&ftrace_start_lock);
	}

and, in de_thread,

		write_unlock_irq(&tasklist_lock);
	+
	+	ftrace_transfer_trace(leader);
	+
		release_task(leader);

No?

(the above is not right of course, we can race with clear_ftrace_pid(),
 just for illustration)

Oleg.

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/03, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > On 02/03, Oleg Nesterov wrote:
>> >>
>> >> "ftrace: use struct pid" commit 978f3a45d9499c7a447ca7615455cefb63d44165
>> >> converted ftrace_pid_trace to "struct pid*". But we can't use
>> >> do_each_pid_task() without rcu_read_lock() even if we know the pid
>> >> itself can't go away (it was pinned in ftrace_pid_write). The exiting
>> >> task can detach itself from this pid at any moment.
>> >
>> > Q: why do we use do_each_pid_task(PIDTYPE_PID) ? We can never have more
>> > than 1 task in the loop. Perhaps,
>>
>> That is a bug in do_each_pid_task(PIDTYPE_PID).
>> For ftrace we really want to grab all tasks with a given pid even
>> in the crazy exec case.
>
> Yes, I thought about de_thread() too. But we can't "fix" do_each_pid_task()
> to avoid the race?

For the case of exec there is that.  There is also the case that
ftrace unlike everything else wants to trace be able to trace all of
the idle threads with pid 0.  I think that is a special case
currently, but for that case the only correct version I can think
of do_each_task_pid(), and current do_each_task_pid is wrong because
it does not allow that.

Eric

Re: [PATCH] ftrace: do_each_pid_task() needs rcu lock

[Oleg Nesterov]

On 02/03, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Yes, I thought about de_thread() too. But we can't "fix" do_each_pid_task()
> > to avoid the race?
>
> For the case of exec there is that.  There is also the case that
> ftrace unlike everything else wants to trace be able to trace all of
> the idle threads with pid 0.  I think that is a special case
> currently, but for that case the only correct version I can think
> of do_each_task_pid(), and current do_each_task_pid is wrong because
> it does not allow that.

Well, yes, I (partly) agree.

But, if we are talking about idle threads, we should change copy_process()
first. Because fork_idle()->copy_process(..., pid => init_struct_pid, ...)
means we don't really attach the idle thread to init_struct_pid.

I must admit, I think we should keep init_struct_pid "special" anyway,
but in any case you are right imho, there are nasty oddities here.

Oleg.