Q: perf_event && task->ptrace_bps[]

I am puzzled by PF_EXITING check in find_lively_task_by_vpid().

How can it help? The task can call do_exit() right after the check.

And why do we need it? The comment only says "Can't attach events to a dying task". Maybe it tries protect sys_perf_event_open() against perf_event_exit_task_context(), but it can't.

Hmm. jump_label_inc/dec looks obviously racy too. Say, free_event() races with perf_event_alloc(). There is a window between atomic_xxx() and jump_label_update(), afaics it is possible to call jump_label_disable() when perf_task_events/perf_swevent_enabled != 0.

find_get_context() must not install the new perf_event_context if the task has already passed perf_event_exit_task().

If nothing else, this means the memory leak. Initially ctx->refcount == 2, it is supposed that perf_event_exit_task_context() should participate and do the necessary put_ctx().

find_lively_task_by_vpid() checks PF_EXITING but this buys nothing, by the time we call find_get_context() this task can be already dead. To the point, cmpxchg() can succeed when the task has already done the last schedule().

Change find_get_context() to populate task->perf_event_ctxp[] under task->perf_event_mutex, this way we can trust PF_EXITING because perf_event_exit_task() takes the same mutex.

Also, change perf_event_exit_task_context() to use rcu_dereference(). Probably this is not strictly needed, but with or without this change find_get_context() can race with setup_new_exec()->perf_event_exit_task(), rcu_dereference() looks better.

Commit-ID:  dbe08d82ce3967ccdf459f7951d02589cf967300
Gitweb:     http://git.kernel.org/tip/dbe08d82ce3967ccdf459f7951d02589cf967300
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Wed, 19 Jan 2011 19:22:07 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 19 Jan 2011 20:04:27 +0100

perf: Fix find_get_context() vs perf_event_exit_task() race

find_get_context() must not install the new perf_event_context if the task has already passed perf_event_exit_task().

If nothing else, this means the memory leak. Initially ctx->refcount == 2, it is supposed that perf_event_exit_task_context() should participate and do the necessary put_ctx().

find_lively_task_by_vpid() checks PF_EXITING but this buys nothing, by the time we call find_get_context() this task can be already dead. To the point, cmpxchg() can succeed when the task has already done the last schedule().

Change find_get_context() to populate task->perf_event_ctxp[] under task->perf_event_mutex, this way we can trust PF_EXITING because perf_event_exit_task() takes the same mutex.

Also, change perf_event_exit_task_context() to use rcu_dereference(). Probably this is not strictly needed, but with or without this change find_get_context() can race with setup_new_exec()->perf_event_exit_task(), rcu_dereference() looks better.

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Cc Alan Stern <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Prasad <[email protected]>

Cc Roland McGrath <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/perf_event.c |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)

kernel/perf_event.c +19 -13

index 84522c7..4ec55ef 100644
@@ -2201,13 +2201,6 @@ find_lively_task_by_vpid(pid_t vpid)
 	if (!task)
 		return ERR_PTR(-ESRCH);
 
-	/*
-	 * Can't attach events to a dying task.
-	 */
-	err = -ESRCH;
-	if (task->flags & PF_EXITING)
-		goto errout;
-
 	/* Reuse ptrace permission checks for now. */
 	err = -EACCES;
 	if (!ptrace_may_access(task, PTRACE_MODE_READ))

@@ -2268,14 +2261,27 @@ retry:
 
 		get_ctx(ctx);
 
-		if (cmpxchg(&task->perf_event_ctxp[ctxn], NULL, ctx)) {
-			/*
-			 * We raced with some other task; use
-			 * the context they set.
-			 */
+		err = 0;
+		mutex_lock(&task->perf_event_mutex);
+		/*
+		 * If it has already passed perf_event_exit_task().
+		 * we must see PF_EXITING, it takes this mutex too.
+		 */
+		if (task->flags & PF_EXITING)
+			err = -ESRCH;
+		else if (task->perf_event_ctxp[ctxn])
+			err = -EAGAIN;
+		else
+			rcu_assign_pointer(task->perf_event_ctxp[ctxn], ctx);
+		mutex_unlock(&task->perf_event_mutex);
+
+		if (unlikely(err)) {
 			put_task_struct(task);
 			kfree(ctx);
-			goto retry;
+
+			if (err == -EAGAIN)
+				goto retry;
+			goto errout;
 		}
 	}
 

Quoted 8 lines · tip-bot for Oleg Nesterov

kernel/perf_event.c +1 -1

@@ -6127,7 +6133,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
 	 * scheduled, so we are now safe from rescheduling changing
 	 * our context.
 	 */
-	child_ctx = child->perf_event_ctxp[ctxn];
+	child_ctx = rcu_dereference(child->perf_event_ctxp[ctxn]);
 	task_ctx_sched_out(child_ctx, EVENT_ALL);
 

Review static void perf_event_exit_task_context(struct task_struct *child, int ctxn) · line 6139

Quoted 1 line

(quoted diff) +0 -0

 	/*

hm, this one's causing:

 [   25.557579] ===================================================
 [   25.561361] [ INFO: suspicious rcu_dereference_check() usage. ]

In theory, almost every user of task->child->perf_event_ctxp[] is wrong. find_get_context() can install the new context at any moment, we need read_barrier_depends().

dbe08d82ce3967ccdf459f7951d02589cf967300 "perf: Fix find_get_context() vs perf_event_exit_task() race" added rcu_dereference() into perf_event_exit_task_context() to make the precedent, but this makes __rcu_dereference_check() unhappy. Use rcu_dereference_raw() to shut up the warning.

Quoted 4 lines · Oleg Nesterov

In theory, almost every user of task->child->perf_event_ctxp[] is wrong. find_get_context() can install the new context at any moment, we need read_barrier_depends().

And perhaps it makes sense to fix them all, although the problem is only theoretical.

Quoted 5 lines

dbe08d82ce3967ccdf459f7951d02589cf967300 "perf: Fix find_get_context() vs perf_event_exit_task() race" added rcu_dereference() into perf_event_exit_task_context() to make the precedent, but this makes __rcu_dereference_check() unhappy. Use rcu_dereference_raw() to shut up the warning.

But rcu_dereference_raw() looks a bit confusing, and it is not very convenient to use read_barrier_depends() directly.

Paul, may be it makes sense to add the new trivial helper which can be used instead?

Yes, this is only cosmetic issue, I know ;)

Quoted 4 lines · Oleg Nesterov

But rcu_dereference_raw() looks a bit confusing, and it is not very convenient to use read_barrier_depends() directly.

Quoted 4 lines

Paul, may be it makes sense to add the new trivial helper which can be used instead?

Yes, this is only cosmetic issue, I know ;)

Cosmetic issues can be pretty important to the poor guy trying to read the code. ;-)

What keeps the structure that rcu_dereference_raw() returns a pointer to from going away?

Best would be if a lockdep condition could be
constructed from the answer to this question and added to the appropriate rcu_dereference() primitive.

perf_event_init_task() should clear child->perf_event_ctxp[] before anything else. Otherwise, if perf_event_init_context(perf_hw_context) fails, perf_event_free_task() can free perf_event_ctxp[perf_sw_context] copied from parent->perf_event_ctxp[] by dup_task_struct().

Also move the initialization of perf_event_mutex and perf_event_list from perf_event_init_context() to perf_event_init_context().

Signed-off-by Oleg Nesterov <[email protected]>

Also. I believe there are more problems in perf_install_event(), but I need to recheck.

Quoted 3 lines · Oleg Nesterov

Also. I believe there are more problems in perf_install_event(), but I need to recheck.

Help! I can't believe it can be so trivially wrong, but otoh I can't understand how this can be correct.

So, ignoring details and !task case, __perf_install_in_context() does:

	if (cpuctx->task_ctx || ctx->task != current)
		return;

	cpuctx->task_ctx = ctx;
	event_sched_in(event);

Stupid question, what if this task has already passed perf_event_exit_task() and thus it doesn't have ->perf_event_ctxp[] ? Given that perf_event_context_sched_out() does nothing if !ctx, who will event_sched_out() this event?

OK, even if I am right this is trivial, we just need the additional check.

But, it seems, there is another problem. Forget about the exiting, I can't understand why we can trust current in the code above. With __ARCH_WANT_INTERRUPTS_ON_CTXSW schedule() does:

	// sets cpuctx->task_ctx = NULL
	perf_event_task_sched_out();

	// enables irqs
	prepare_lock_switch();


	// updates current_task
	switch_to();

What if IPI comes in the window before switch_to() ?

(the same questions for __perf_event_enable).

Quoted 16 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

Also. I believe there are more problems in perf_install_event(), but I need to recheck.

Help! I can't believe it can be so trivially wrong, but otoh I can't understand how this can be correct.

So, ignoring details and !task case, __perf_install_in_context() does:

	if (cpuctx->task_ctx || ctx->task != current)
		return;

	cpuctx->task_ctx = ctx;
	event_sched_in(event);

Quoted 7 lines

Stupid question, what if this task has already passed perf_event_exit_task() and thus it doesn't have ->perf_event_ctxp[] ? Given that perf_event_context_sched_out() does nothing if !ctx, who will event_sched_out() this event?

OK, even if I am right this is trivial, we just need the additional check.

Indeed (or do the cleanup from put_ctx(), but that's too complex a change I think).

Quoted 14 lines

But, it seems, there is another problem. Forget about the exiting, I can't understand why we can trust current in the code above. With __ARCH_WANT_INTERRUPTS_ON_CTXSW schedule() does:

	// sets cpuctx->task_ctx = NULL
	perf_event_task_sched_out();

	// enables irqs
	prepare_lock_switch();


	// updates current_task
	switch_to();

Quoted 3 lines

What if IPI comes in the window before switch_to() ?

(the same questions for __perf_event_enable).

Ingo, do you have any insights in that, I think you wrote all that initially?

Quoted 14 lines

Quoted 14 lines

But, it seems, there is another problem. Forget about the exiting, I can't understand why we can trust current in the code above. With __ARCH_WANT_INTERRUPTS_ON_CTXSW schedule() does:

    // sets cpuctx->task_ctx = NULL
    perf_event_task_sched_out();

    // enables irqs
    prepare_lock_switch();


    // updates current_task
    switch_to();

Quoted 6 lines

Quoted 3 lines

What if IPI comes in the window before switch_to() ?

(the same questions for __perf_event_enable).

Ingo, do you have any insights in that, I think you wrote all that initially?

Not sure. Can an IPI come there - we have irqs disabled usually, dont we?

Quoted 14 lines · Ingo Molnar

Quoted 14 lines

Quoted 14 lines

But, it seems, there is another problem. Forget about the exiting, I can't understand why we can trust current in the code above. With __ARCH_WANT_INTERRUPTS_ON_CTXSW schedule() does:

    // sets cpuctx->task_ctx = NULL
    perf_event_task_sched_out();

    // enables irqs
    prepare_lock_switch();


    // updates current_task
    switch_to();

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

What if IPI comes in the window before switch_to() ?

(the same questions for __perf_event_enable).

Ingo, do you have any insights in that, I think you wrote all that initially?

Not sure. Can an IPI come there - we have irqs disabled usually, dont we?

Ah, I think I see how that works:

  __perf_event_task_sched_out()
    perf_event_context_sched_out()
      if (do_switch)
        cpuctx->task_ctx = NULL;

vs

__perf_install_in_context()
if (cpu_ctx->task_ctx != ctx)

Quoted 2 lines

Ah, I think I see how that works:

Hmm. I don't...

Quoted 5 lines

  __perf_event_task_sched_out()
    perf_event_context_sched_out()
      if (do_switch)
        cpuctx->task_ctx = NULL;

exactly, this clears ->task_ctx

Quoted 4 lines

vs

__perf_install_in_context()
if (cpu_ctx->task_ctx != ctx)

And then __perf_install_in_context() sets cpuctx->task_ctx = ctx, because ctx->task == current && cpuctx->task_ctx == NULL.

Quoted 13 lines · Oleg Nesterov

Quoted 2 lines

Ah, I think I see how that works:

Hmm. I don't...

Quoted 5 lines

  __perf_event_task_sched_out()
    perf_event_context_sched_out()
      if (do_switch)
        cpuctx->task_ctx = NULL;

exactly, this clears ->task_ctx

Quoted 7 lines

Quoted 4 lines

vs

__perf_install_in_context()
if (cpu_ctx->task_ctx != ctx)

And then __perf_install_in_context() sets cpuctx->task_ctx = ctx, because ctx->task == current && cpuctx->task_ctx == NULL.

Hrm,. right, so the comment suggests it should do what it doesn't :-)

It looks like Paul's a63eaf34ae60bd (perf_counter: Dynamically allocate tasks' perf_counter_context struct), relevant hunk below, wrecked it:

(quoted diff) +8 -2

@@ -568,11 +582,17 @@ static void __perf_install_in_context(void *info)
         * If this is a task context, we need to check whether it is
         * the current task context of this cpu. If not it has been
         * scheduled out before the smp call arrived.
+        * Or possibly this is the right context but it isn't
+        * on this cpu because it had no counters.
         */
-       if (ctx->task && cpuctx->task_ctx != ctx)
-               return;
+       if (ctx->task && cpuctx->task_ctx != ctx) {
+               if (cpuctx->task_ctx || ctx->task != current)
+                       return;
+               cpuctx->task_ctx = ctx;
+       }
 
        spin_lock_irqsave(&ctx->lock, flags);
+       ctx->is_active = 1;
        update_context_time(ctx);
 
        /*

I can't really seem to come up with a sane test that isn't racy with something, my cold seems to have clogged not only my nose :/

+static DEFINE_PER_CPU(int, task_events_schedulable);

Frederic, All, can't we simplify this?

First, we modify __perf_install_in_context() so that it never tries to install the event into !is_active context. IOW, it never tries to set cpuctx->task_ctx = ctx.

Then we add the new trivial helper stop_resched_task(task) which simply wakeups the stop thread on task_cpu(task), and thus forces this task to reschedule.

...

Yes, stop_resched_task() can't help if this task itself is the stop thread. But the stop thread shouldn't run for a long time without rescheduling, otherwise we already have the problems.

Please see the untested patch below. It doesn't change perf_event_enable(), only perf_install_in_context().

Quoted 3 lines · Oleg Nesterov

Please see the untested patch below. It doesn't change perf_event_enable(), only perf_install_in_context().

Forgot to mention... Also, it doesn't try to fix the race with do_exit(), this needs another change.

And, damn, can't resist. This is mostly cosmetic issue, but I feel discomfort every time I look at task_oncpu_function_call(). It _looks_ obviously wrong, even if the problem doesn't exist in practice. I'll send the pedantic fix to keep the maintainers busy ;)

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

Please see the untested patch below. It doesn't change perf_event_enable(), only perf_install_in_context().

Quoted 7 lines

Forgot to mention... Also, it doesn't try to fix the race with do_exit(), this needs another change.

And, damn, can't resist. This is mostly cosmetic issue, but I feel discomfort every time I look at task_oncpu_function_call(). It _looks_ obviously wrong, even if the problem doesn't exist in practice. I'll send the pedantic fix to keep the maintainers busy ;)

I've been trying to sit down and work my way through it today, your last suggestion very nearly seemed to make sense, but I kept getting distracted.

FWIW I think perf_event_enable() has the very same issue...

Wouldn't something like the below cure things too?

kernel/sched.c +5 -0

+struct task_function_call {
+	struct task_struct *p;
+	void (*func)(void *info);
+	void *info;
+};

Quoted 14 lines · Peter Zijlstra

Quoted 12 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

Please see the untested patch below. It doesn't change perf_event_enable(), only perf_install_in_context().

Forgot to mention... Also, it doesn't try to fix the race with do_exit(), this needs another change.

And, damn, can't resist. This is mostly cosmetic issue, but I feel discomfort every time I look at task_oncpu_function_call(). It _looks_ obviously wrong, even if the problem doesn't exist in practice. I'll send the pedantic fix to keep the maintainers busy ;)

Quoted 5 lines

I've been trying to sit down and work my way through it today, your last suggestion very nearly seemed to make sense, but I kept getting distracted.

FWIW I think perf_event_enable() has the very same issue...

kernel/sched.c void kick_process(struct task_struct *p) · line 2279

Quoted 7 lines

kernel/sched.c +6 -0

+void task_function_trampoline(void *data)
+{
+	struct task_function_call *tfc = data;
+
+	if (this_rq()->curr != tfc->p)
+		return;

Yes, I was thinking about checking rq->curr too, but this doesn't really help. This closes the race with "prev", but we have the similar race with "next".

__perf_install_in_context() should not set ->task_ctx before next does perf_event_context_sched_in(). Otherwise it will do nothing, it checks cpuctx->task_ctx == ctx.

This is what I had in mind initially but I didn't dare to add the new member into rq, it is only needed for perf.

Quoted 3 lines · Oleg Nesterov

This is what I had in mind initially but I didn't dare to add the new member into rq, it is only needed for perf.

Right, but its a weakness in the task_oncpu_function_call() implementation, wouldn't any user run into this problem eventually?

kernel/sched.c +3 -0

+void task_function_trampoline(void *data)
+{
+	struct task_function_call *tfc = data;

Quoted 3 lines

 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
      local_irq_enable();

+ rq->in_ctxsw = 0;

If we think that context_switch finishes here, probably it would be more clean to clear ->in_ctxsw before local_irq_enable().

Quoted 2 lines

 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
      finish_lock_switch(rq, prev);

But, otoh, maybe finish_lock_switch() can clear in_ctxsw, it already checks __ARCH_WANT_INTERRUPTS_ON_CTXSW. Likewise, perhaps it can be set in prepare_lock_switch() which enables irqs.

But this is cosmetic and up to you.

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines

 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
      local_irq_enable();

+ rq->in_ctxsw = 0;

If we think that context_switch finishes here, probably it would be more clean to clear ->in_ctxsw before local_irq_enable().

It must in fact be done before,

Quoted 5 lines

But, otoh, maybe finish_lock_switch() can clear in_ctxsw, it already checks __ARCH_WANT_INTERRUPTS_ON_CTXSW. Likewise, perhaps it can be set in prepare_lock_switch() which enables irqs.

But this is cosmetic and up to you.

Can't do because of the above thing..

Quoted 3 lines · Oleg Nesterov

This is what I had in mind initially but I didn't dare to add the new member into rq, it is only needed for perf.

Right, but its a weakness in the task_oncpu_function_call() implementation, wouldn't any user run into this problem eventually?

        void task_force_schedule(struct task_struct *p)
        {
                struct rq *rq;

                preempt_disable();
                rq = task_rq(p);
                if (rq->curr == p)
                        wake_up_process(rq->stop);
                preempt_enable();
        }

        static void
        perf_install_in_context(struct perf_event_context *ctx,
                                struct perf_event *event,
                                int cpu)
        {
                struct task_struct *task = ctx->task;

                event->ctx = ctx;

                if (!task) {
                        /*
                         * Per cpu events are installed via an smp call and
                         * the install is always successful.
                         */
                        smp_call_function_single(cpu, __perf_install_in_context,
                                                 event, 1);
                        return;
                }

                for (;;) {
                        raw_spin_lock_irq(&ctx->lock);
                        /*
                         * The lock prevents that this context is
                         * scheduled in, we can add the event safely.
                         */
                        if (!ctx->is_active)
                                add_event_to_ctx(event, ctx);
                        raw_spin_unlock_irq(&ctx->lock);

                        if (event->attach_state & PERF_ATTACH_CONTEXT) {
                                task_force_schedule(task);
                                break;

                        }

                        task_oncpu_function_call(task, __perf_install_in_context,
                                                        event);
                        if (event->attach_state & PERF_ATTACH_CONTEXT)
                                break;
                }
        } 

With, however, things are more interesting. 2 seems to be adequately covered by the patch I just send, the IPI will bail and the next sched-in of the relevant task will pick matters up. 1 otoh doesn't seem covered, the IPI will bail, leaving us stranded.

<crazy idea here>

So I've likely fallen off a cliff somewhere along the line, but just in case, here's the patch:

Right, so the fact of introducing extra scheduling makes me feel uncomfortable... the whole purpose is to observe without perturbing (as much as possible).

Should I think suffice to get the ctx in sync with the task state, we've got the following cases:
1) task is in the middle of scheduling in
2) task is in the middle of scheduling out
3) task is running

Without __ARCH_WANT_INTERRUPT_ON_CTXSW everything is boring and works, 1: the IPI will be delayed until 3, 2: the IPI finds another task and the next schedule in will sort things.

With, however, things are more interesting. 2 seems to be adequately covered by the patch I just send, the IPI will bail and the next sched-in of the relevant task will pick matters up. 1 otoh doesn't seem covered, the IPI will bail, leaving us stranded.

Quoted 5 lines

With, however, things are more interesting. 2 seems to be adequately covered by the patch I just send, the IPI will bail and the next sched-in of the relevant task will pick matters up. 1 otoh doesn't seem covered, the IPI will bail, leaving us stranded.

Hmm, yes... Strangely, I missed that when I was thinking about in_ctxsw.

Perhaps, we can change task_oncpu_function_call() so that it returns -EAGAIN in case it hits in_ctxsw != 0? If the caller sees -EAGAIN, it should always retry even if !ctx->is_active.

Quoted 6 lines · Oleg Nesterov

Quoted 5 lines

With, however, things are more interesting. 2 seems to be adequately covered by the patch I just send, the IPI will bail and the next sched-in of the relevant task will pick matters up. 1 otoh doesn't seem covered, the IPI will bail, leaving us stranded.

Quoted 5 lines

Hmm, yes... Strangely, I missed that when I was thinking about in_ctxsw.

Perhaps, we can change task_oncpu_function_call() so that it returns -EAGAIN in case it hits in_ctxsw != 0? If the caller sees -EAGAIN, it should always retry even if !ctx->is_active.

That would be very easy to do, we can pass a return value through the task_function_call structure.

Quoted 14 lines · Peter Zijlstra

Quoted 11 lines · Oleg Nesterov

Quoted 5 lines

With, however, things are more interesting. 2 seems to be adequately covered by the patch I just send, the IPI will bail and the next sched-in of the relevant task will pick matters up. 1 otoh doesn't seem covered, the IPI will bail, leaving us stranded.

Hmm, yes... Strangely, I missed that when I was thinking about in_ctxsw.

Perhaps, we can change task_oncpu_function_call() so that it returns -EAGAIN in case it hits in_ctxsw != 0? If the caller sees -EAGAIN, it should always retry even if !ctx->is_active.

Quoted 2 lines

That would be very easy to do, we can pass a return value through the task_function_call structure.

Yes.

Perhaps task_oncpu_function_call() should retry itself to simplify the callers. I wonder if we should also retry if rq->curr != p...

Oh. You know, I am starting to think I will never understand this.

perf_install_in_context() does task_oncpu_function_call() and then

	// ctx->is_active == 0

	/*
	 * The lock prevents that this context is scheduled in so we
	 * can add the event safely, if it the call above did not
	 * succeed.
	 */
	if (list_empty(&event->group_entry))
		add_event_to_ctx(event, ctx);

This assumes that the task is not running.

Why? Because (I guess) we assume that either task_oncpu_function_call() should see task_curr() == T, or if the task becomes running after that it should see the new ->perf_event_ctxp[ctxn] != NULL. And I do not see how we can prove this.

If find_get_context() sets the new context, the only guarantee we have
is: perf_event_exit_task() can't miss this context. The task, however, can be scheduled in and miss the new value in perf_event_ctxp[]. And, task_oncpu_function_call() can equally miss rq->curr == task.

But. I think this all falls into the absolutely theoretical category, and in the worst case nothing really bad can happen, just event_sched_in() will be delayed until this task reshedules.

So, I think your patch should fix all problems with schedule. Just it needs the couple of changes we already discussed:

- finish_task_switch() should clear rq->in_ctxsw before local_irq_enable()

	- task_oncpu_function_call() (or its callers) should always
	  retry the "if (task_curr(p))" code if ->in_ctxsw is true.

If you think we have other problems here please don't tell me, I already got lost ;)

Right, so in case the perf_event_task_sched_in() missed the assignment of ->perf_event_ctxp[n], then our above story falls flat on its face.

Because then we can not rely on ->in_active being set for running tasks.

So we need a task_curr() test under that lock, which would need perf_event_task_sched_out() to be done _before_ we set rq->curr = next, I _think_.

Quoted 8 lines · Peter Zijlstra

Right, so in case the perf_event_task_sched_in() missed the assignment of ->perf_event_ctxp[n], then our above story falls flat on its face.

Because then we can not rely on ->in_active being set for running tasks.

So we need a task_curr() test under that lock, which would need perf_event_task_sched_out() to be done _before_ we set rq->curr = next, I _think_.

Ok, so how about something like this:

if task_oncpu_function_call() managed to execute the function proper, we're done. Otherwise, if while holding the lock, task_curr() is true, it means the task is now current and we should try again, if its not, it cannot become current because us holding ctx->lock blocks perf_event_task_sched_in().

Hmm?

kernel/perf_event.c +5 -2

@@ -1031,25 +1032,29 @@ perf_install_in_context(struct perf_event_context *ctx,
 	}
 
 retry:
-	task_oncpu_function_call(task, __perf_install_in_context,
-				 event);
+	ret = task_oncpu_function_call(task,
+			__perf_install_in_context, event);
+
+	if (!ret)
+		return;

I _feel_ this patch should be right. To me, this even makes the code more understandable. But I'll try to re-read it once again, somehow I can't concentrace today.

Just to give you more food for through, I couldn't help myself..

kernel/perf_event.c +2 -6

-static void __perf_install_in_context(void *info)
+static int  __perf_install_in_context(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;

@@ -942,20 +1015,15 @@ static void __perf_install_in_context(void *info)
 	int err;
 
 	/*
-	 * If this is a task context, we need to check whether it is
-	 * the current task context of this cpu. If not it has been
-	 * scheduled out before the smp call arrived.
-	 * Or possibly this is the right context but it isn't
-	 * on this cpu because it had no events.
+	 * In case we're installing a new context to an already running task,

Quoted 2 lines · Peter Zijlstra

Just to give you more food for through, I couldn't help myself..

Hmm. So far I am only trying to understand the perf_install_in_context() paths. And, after I spent almost 2 hours, I am starting to believe this change is probably good ;)

I do not understand the point of cpu_function_call() though, it looks equal to smp_call_function_single() ?

Quoted 22 lines

kernel/perf_event.c +4 -10

-static void __perf_install_in_context(void *info)
+static int  __perf_install_in_context(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;

@@ -942,20 +1015,15 @@ static void __perf_install_in_context(void *info)
 	int err;
 
 	/*
-	 * If this is a task context, we need to check whether it is
-	 * the current task context of this cpu. If not it has been
-	 * scheduled out before the smp call arrived.
-	 * Or possibly this is the right context but it isn't
-	 * on this cpu because it had no events.
+	 * In case we're installing a new context to an already running task,
+	 * could also happen before perf_event_task_sched_in() on architectures
+	 * which do context switches with IRQs enabled.
 	 */
-	if (ctx->task && cpuctx->task_ctx != ctx) {
-		if (cpuctx->task_ctx || ctx->task != current)
-			return;
-		cpuctx->task_ctx = ctx;

kernel/perf_event.c static void __perf_install_in_context(void *info) · line 1023

Quoted 3 lines

kernel/perf_event.c +2 -1

-	}
+	if (ctx->task && !cpuctx->task_ctx)
+		perf_event_context_sched_in(ctx);

OK... This eliminates the 2nd race with __ARCH_WANT_INTERRUPTS_ON_CTXSW (we must not set "cpuctx->task_ctx = ctx" in case "next" is going to
do perf_event_context_sched_in() later). So it is enough to check
rq->curr in remote_function().

kernel/perf_event.c static void __perf_install_in_context(void *info) · line 1026

Quoted 4 lines

kernel/perf_event.c +1 -1

 	raw_spin_lock(&ctx->lock);
-	ctx->is_active = 1;
+	WARN_ON_ONCE(!ctx->is_active);

This looks wrong if ctx->task == NULL.

So. With this patch it is possible that perf_event_context_sched_in() is called right after prepare_lock_switch(). Stupid question, why can't we always do this then? I mean, what if we change prepare_task_switch() to do

	perf_event_task_sched_out();
	perf_event_task_sched_in();

?

Probably we can unify the COND_STMT(perf_task_events) check and simplify the things further.

Quoted 2 lines · Oleg Nesterov

I do not understand the point of cpu_function_call() though, it looks equal to smp_call_function_single() ?

Very nearly so, except it takes a function that returns an int..

Quoted 5 lines

kernel/perf_event.c static void __perf_install_in_context(void *info) · line 1026

Quoted 3 lines

kernel/perf_event.c +1 -1

 	raw_spin_lock(&ctx->lock);
-	ctx->is_active = 1;
+	WARN_ON_ONCE(!ctx->is_active);

This looks wrong if ctx->task == NULL.

cpuctx->ctx should still have ->is_active = 1 I think.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

raw_spin_lock(&ctx->lock);
- ctx->is_active = 1;
+ WARN_ON_ONCE(!ctx->is_active);

This looks wrong if ctx->task == NULL.

cpuctx->ctx should still have ->is_active = 1 I think.

Unless this is the first cpu counter, no?

Oleg, I've actually run-tested the below and all seems well (clearly I've never actually hit the races found before either, so in that respect its not a conclusive test).

Can you agree with this patch?

kernel/perf_event.c +1 -1

-static void __perf_event_remove_from_context(void *info)
+static int __perf_remove_from_context(void *info)
 {
 	struct perf_event *event = info;

kernel/perf_event.c +4 -3

-static void perf_event_remove_from_context(struct perf_event *event)
+static void perf_remove_from_context(struct perf_event *event)
 {
...
 	raw_spin_lock_irq(&ctx->lock);
 	/*
-	 * If the context is active we need to retry the smp call.
+	 * If we failed to find a running task, but find it running now that
+	 * we've acquired the ctx->lock, retry.
 	 */
-	if (ctx->nr_active && !list_empty(&event->group_entry)) {
+	if (task_curr(task)) {
 		raw_spin_unlock_irq(&ctx->lock);
 		goto retry;
 	}
 

Quoted 6 lines · Peter Zijlstra

Oleg, I've actually run-tested the below and all seems well (clearly I've never actually hit the races found before either, so in that respect its not a conclusive test).

Can you agree with this patch?

You know, I already wrote the i-think-it-is-correct email. But then I decided to read it once again.

Quoted 7 lines

kernel/perf_event.c +1 -1

-static void __perf_event_remove_from_context(void *info)
+static int __perf_remove_from_context(void *info)
 {
 	struct perf_event *event = info;
 	struct perf_event_context *ctx = event->ctx;
 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);

kernel/perf_event.c __get_cpu_context(struct perf_event_context *ctx) · line 633

Quoted 7 lines

kernel/perf_event.c +0 -7

-	/*
-	 * If this is a task context, we need to check whether it is
-	 * the current task context of this cpu. If not it has been
-	 * scheduled out before the smp call arrived.
-	 */
-	if (ctx->task && cpuctx->task_ctx != ctx)
-		return;

OK, I think this is right... event_sched_out() will see PERF_EVENT_STATE_INACTIVE if perf_event_task_sched_in() was not called yet.

But,

Quoted 16 lines

kernel/perf_event.c +4 -3

-static void perf_event_remove_from_context(struct perf_event *event)
+static void perf_remove_from_context(struct perf_event *event)
 {
...
 	raw_spin_lock_irq(&ctx->lock);
 	/*
-	 * If the context is active we need to retry the smp call.
+	 * If we failed to find a running task, but find it running now that
+	 * we've acquired the ctx->lock, retry.
 	 */
-	if (ctx->nr_active && !list_empty(&event->group_entry)) {
+	if (task_curr(task)) {
 		raw_spin_unlock_irq(&ctx->lock);
 		goto retry;
 	}
 

kernel/perf_event.c static void __perf_event_remove_from_context(void *info) · line 756

Quoted 10 lines

kernel/perf_event.c +3 -5

 	/*
-	 * The lock prevents that this context is scheduled in so we
-	 * can remove the event safely, if the call above did not
-	 * succeed.
+	 * Since the task isn't running, its safe to remove the event, us
+	 * holding the ctx->lock ensures the task won't get scheduled in.
 	 */
-	if (!list_empty(&event->group_entry))
-		list_del_event(event, ctx);
+	list_del_event(event, ctx);

this looks suspicious (the same for perf_install_in_context).

Unlike the IPI handler, this can see schedule-in-progress in any state. In particular, we can see rq->curr == next (so that task_curr() == F), but before "prev" has already called perf_event_task_sched_out().

So we have to check ctx->is_active, or schedule() should change rq->curr after perf_event_task_sched_out().

Quoted 5 lines

kernel/perf_event.c +0 -0

@@ -753,13 +819,13 @@ void perf_event_disable(struct perf_event *event)
...
 	 */
 	if (event->state == PERF_EVENT_STATE_ACTIVE) {
 		raw_spin_unlock_irq(&ctx->lock);

kernel/perf_event.c retry: · line 841

Quoted 6 lines

kernel/perf_event.c +5 -0

+		/*
+		 * Reload the task pointer, it might have been changed by
+		 * a concurrent perf_event_context_sched_out().
+		 */
+		task = ctx->task;
 		goto retry;

I am wondering why only perf_event_disable() needs this...

Just curious, this is equally needed without this patch?

Quoted 25 lines

Quoted 8 lines

kernel/perf_event.c +3 -0

+static void perf_remove_from_context(struct perf_event *event)
 {
...
    raw_spin_lock_irq(&ctx->lock);
    /*
+    * If we failed to find a running task, but find it running now that
+    * we've acquired the ctx->lock, retry.
     */

kernel/perf_event.c static void __perf_event_remove_from_context(void *info) · line 756

Quoted 10 lines

kernel/perf_event.c +4 -0

+   if (task_curr(task)) {
            raw_spin_unlock_irq(&ctx->lock);
            goto retry;
    }
 
    /*
+    * Since the task isn't running, its safe to remove the event, us
+    * holding the ctx->lock ensures the task won't get scheduled in.
     */
+   list_del_event(event, ctx);

this looks suspicious (the same for perf_install_in_context).

Unlike the IPI handler, this can see schedule-in-progress in any state. In particular, we can see rq->curr == next (so that task_curr() == F), but before "prev" has already called perf_event_task_sched_out().

Quoted 2 lines

So we have to check ctx->is_active, or schedule() should change rq->curr after perf_event_task_sched_out().

I only considered current == next in that case, not current == prev, let me undo some of those sched.c bits and put a comment.

perf_install_in_context() works on a ctx obtained by find_get_context(), that context is either new (uncloned) or existing in which case it called unclone_ctx(). So I was thinking there was no race with the ctx flipping in perf_event_context_sched_out(), _however_ since it only acquires ctx->mutex after calling unclone_ctx() there is a race window with perf_event_init_task().

This race we should fix with perf_pin_task_context()

Another thing I can't understand, event->owner/owner_entry.

Say, some thread calls sys_perf_event_open() and creates the event. It becomes its owner. Now this thread exits, but fd/event are still here, and event->owner refers to the dead task_struct.

ptrace looks even more strange. Debugger can attach the breakpoint to the tracee and then exit/detach. ->ptrace_bps events still point to the same (may be dead) task. Even if another debugger attaches and reuses these events.

Perhaps the real owner should be the task on which we attach our breakpoint.

As to removing event->owner as Oleg suggests, its a published ABI and there might be people using it.

Quoted 5 lines · Oleg Nesterov

Another thing I can't understand, event->owner/owner_entry.

Say, some thread calls sys_perf_event_open() and creates the event. It becomes its owner. Now this thread exits, but fd/event are still here, and event->owner refers to the dead task_struct.

Hmm, it seems to me that the last reference to the events are put in __perf_event_exit_task,

and then free_event() is called
there, which rcu queues the event to be released.

Not sure where is the issue here.

Quoted 4 lines

ptrace looks even more strange. Debugger can attach the breakpoint to the tracee and then exit/detach. ->ptrace_bps events still point to the same (may be dead) task. Even if another debugger attaches and reuses these events.

Hmm, in this case ptrace_bps will continue to trigger on the task they were applied.

On the other hand, you're right, I'm not sure that the debugger is the correct owner for the breakpoints.
I think it works though, looking at perf_event_create_kernel_counter():

	event->owner = current;
	get_task_struct(current);

(current is the debugger)

On perf_event_release_kernel():

	put_task_struct(event->owner);

So even if the debugger dies, we keep a valid owner, it works but just makes few sense as the debugger can change.

Perhaps the real owner should be the task on which we attach our breakpoint.

If the creator of perf_event dies, nobody can use its ->perf_event_list anyway. What is the point to keep the reference to the dead task_struct and preserve this ->perf_event_list?

Quoted 4 lines · Oleg Nesterov

If the creator of perf_event dies, nobody can use its ->perf_event_list anyway. What is the point to keep the reference to the dead task_struct and preserve this ->perf_event_list?

But when the owner dies it will close all its fds, which means it will clear its tsk->perf_event_list, no? (With exception of the case where the fd was passed through a unix-socket to another process).

Quoted 7 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

If the creator of perf_event dies, nobody can use its ->perf_event_list anyway. What is the point to keep the reference to the dead task_struct and preserve this ->perf_event_list?

Quoted 3 lines

But when the owner dies it will close all its fds, which means it will clear its tsk->perf_event_list, no? (With exception of the case where the fd was passed through a unix-socket to another process).

fork(), pthread_create(). Only __fput() calls ->release, when the last reference to file goes away.

And ptrace(), it doesn't use sys_perf_event_open() to create the event.

Ah,.. quite so. So how about we explicitly destroy the list when the task dies?

Quoted 1 line

And ptrace(), it doesn't use sys_perf_event_open() to create the event.

Right, I guess it uses kernel based things, I guess we could not add kernel based counters to the list.

Quoted 3 lines · Peter Zijlstra

Ah,.. quite so. So how about we explicitly destroy the list when the task dies?

Yes, I think it makes sense to destroy the list and set ->owner = NULL. If we reset the owner, we can also avoid get_task_struct().

The only problem is perf_event_release_kernel(), it can race with the exiting event->owner. It can do get_task_struct() under rcu lock temporary, just to take the mutex and remove the entry.

Quoted 4 lines

Quoted 1 line

And ptrace(), it doesn't use sys_perf_event_open() to create the event.

Right, I guess it uses kernel based things, I guess we could not add kernel based counters to the list.

Agreed, another case when event->owner should be NULL.

Hmm. With or without these changes. Shouldn't perf_event_release_kernel() remove the event from list before anything else? Otherwise, afaics a thread which does close(event_fd) can race with creator doing prctl(EVENTS_ENABLE), no?

I think you're right, how about something like this?

Quoted 2 lines · Peter Zijlstra

I think you're right, how about something like this?

I need to read it with a fresh head ;)

Either sys_perf_open() should do get_task_struct() like we currently do, or perf_event_exit_task() should clear event->owner and then perf_release() should do something like

        rcu_read_lock();
        owner = event->owner;
        if (owner)
                get_task_struct(owner);
        rcu_read_unlock();

        if (owner) {
                mutex_lock(&event->owner->perf_event_mutex);
                list_del_init(&event->owner_entry);
                mutex_unlock(&event->owner->perf_event_mutex);
                put_task_struct(owner);
        }

Probably this can be simplified...

Quoted 17 lines · Oleg Nesterov

Either sys_perf_open() should do get_task_struct() like we currently do, or perf_event_exit_task() should clear event->owner and then perf_release() should do something like

        rcu_read_lock();
        owner = event->owner;
        if (owner)
                get_task_struct(owner);
        rcu_read_unlock();

        if (owner) {
                mutex_lock(&event->owner->perf_event_mutex);
                list_del_init(&event->owner_entry);
                mutex_unlock(&event->owner->perf_event_mutex);
                put_task_struct(owner);
        }

Quoted 1 line

Probably this can be simplified...

I think that's still racy, suppose we do:

void perf_event_exit_task(struct task_struct *child)

{
	struct perf_event *event, *tmp;
	int ctxn;

	mutex_lock(&child->perf_event_mutex);
	list_for_each_entry_safe(event, tmp, &child->perf_event_list,
				 owner_entry) {
		event->owner = NULL;
		list_del_init(&event->owner_entry);
	}
	mutex_unlock(&child->perf_event_mutex);

	for_each_task_context_nr(ctxn)
		perf_event_exit_task_context(child, ctxn);
}

and the close() races with an exit, then couldn't we observe event->owner after the last put_task_struct()?

But the code is racy, yes. owner != NULL case is fine. But perf_release() can see event->owner == NULL before list_del() was completed. perf_event_exit_task() needs wmb() in between, I think.

Utter paranoia took over and I'm still not sure its solid...

Subject: perf: Fix owner-list vs exit
From: Peter Zijlstra <[email protected]>
Date: Tue, 09 Nov 2010 19:01:43 +0100

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <1289325703.2191.60.camel@laptop>

---
 kernel/perf_event.c |   63 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 12 deletions(-)

Index: linux-2.6/kernel/perf_event.c
===================================================================

kernel/perf_event.c +51 -12

@@ -2234,11 +2234,6 @@ int perf_event_release_kernel(struct per
 	raw_spin_unlock_irq(&ctx->lock);
 	mutex_unlock(&ctx->mutex);
 
-	mutex_lock(&event->owner->perf_event_mutex);
-	list_del_init(&event->owner_entry);
-	mutex_unlock(&event->owner->perf_event_mutex);
-	put_task_struct(event->owner);
-
 	free_event(event);
 
 	return 0;

@@ -2251,9 +2246,43 @@ EXPORT_SYMBOL_GPL(perf_event_release_ker
 static int perf_release(struct inode *inode, struct file *file)
 {
 	struct perf_event *event = file->private_data;
+	struct task_struct *owner;
 
 	file->private_data = NULL;
 
+	rcu_read_lock();
+	owner = ACCESS_ONCE(event->owner);
+	/*
+	 * Matches the smp_wmb() in perf_event_exit_task(). If we observe
+	 * !owner it means the list deletion is complete and we can indeed
+	 * free this event, otherwise we need to serialize on
+	 * owner->perf_event_mutex.
+	 */
+	smp_read_barrier_depends();
+	if (owner) {
+		/*
+		 * Since delayed_put_task_struct() also drops the last
+		 * task reference we can safely take a new reference
+		 * while holding the rcu_read_lock().
+		 */
+		get_task_struct(owner);
+	}
+	rcu_read_unlock();
+
+	if (owner) {
+		mutex_lock(&owner->perf_event_mutex);
+		/*
+		 * We have to re-check the event->owner field, if it is cleared
+		 * we raced with perf_event_exit_task(), acquiring the mutex
+		 * ensured they're done, and we can proceed with freeing the
+		 * event.
+		 */
+		if (event->owner)
+			list_del_init(&event->owner_entry);
+		mutex_unlock(&owner->perf_event_mutex);
+		put_task_struct(owner);
+	}
+
 	return perf_event_release_kernel(event);
 }
 

@@ -5677,7 +5706,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	mutex_unlock(&ctx->mutex);
 
 	event->owner = current;
-	get_task_struct(current);
+
 	mutex_lock(&current->perf_event_mutex);
 	list_add_tail(&event->owner_entry, &current->perf_event_list);
 	mutex_unlock(&current->perf_event_mutex);

@@ -5745,12 +5774,6 @@ perf_event_create_kernel_counter(struct
 	++ctx->generation;
 	mutex_unlock(&ctx->mutex);
 
-	event->owner = current;
-	get_task_struct(current);
-	mutex_lock(&current->perf_event_mutex);
-	list_add_tail(&event->owner_entry, &current->perf_event_list);
-	mutex_unlock(&current->perf_event_mutex);
-
 	return event;
 
 err_free:

@@ -5901,8 +5924,24 @@ static void perf_event_exit_task_context
  */
 void perf_event_exit_task(struct task_struct *child)
 {
+	struct perf_event *event, *tmp;
 	int ctxn;
 
+	mutex_lock(&child->perf_event_mutex);
+	list_for_each_entry_safe(event, tmp, &child->perf_event_list,
+				 owner_entry) {
+		list_del_init(&event->owner_entry);
+
+		/*
+		 * Ensure the list deletion is visible before we clear
+		 * the owner, closes a race against perf_release() where
+		 * we need to serialize on the owner->perf_event_mutex.
+		 */
+		smp_wmb();
+		event->owner = NULL;
+	}
+	mutex_unlock(&child->perf_event_mutex);
+
 	for_each_task_context_nr(ctxn)
 		perf_event_exit_task_context(child, ctxn);
 }

Hello.

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

It is not clear to me what should be done. Looking more, I do not understand the scope of perf_event/ctx at all, sys_perf_event_open() looks wrong too, see the next email I am going to send.

Oleg.

Quoted 17 lines · Oleg Nesterov

Hello.

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

Quoted 5 lines

It is not clear to me what should be done. Looking more, I do not understand the scope of perf_event/ctx at all, sys_perf_event_open() looks wrong too, see the next email I am going to send.

Oleg.

But I don't understand how ptrace_set_debugreg() and flush_old_exec() can happen at the same time.

The parent can only do the ptrace request when
the child is stopped, right?

I am certainly missing something obvious.

Quoted 19 lines · Frederic Weisbecker

Quoted 17 lines · Oleg Nesterov

Hello.

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

Quoted 8 lines

Quoted 5 lines

It is not clear to me what should be done. Looking more, I do not understand the scope of perf_event/ctx at all, sys_perf_event_open() looks wrong too, see the next email I am going to send.

Oleg.

But I don't understand how ptrace_set_debugreg() and flush_old_exec() can happen at the same time.

This can't happen. But I meant do_exit()->flush_ptrace_hw_breakpoint()

Quoted 2 lines

The parent can only do the ptrace request when
the child is stopped, right?

Yes. But nothing can "pin" TASK_TRACED.

We know that a) the tracee was stopped() when sys_ptrace() was called and b) its task_struct can't go away. That is all. The tracee can be killed at any moment, and sys_ptrace() can race with with flush_ptrace_hw_breakpoint().

Aah, so we check if the task is stopped when sys_ptrace() is called, but right after we do this check, the tracee can be resumed at any time? (with either SIGCONT or SIGKILL), even if we are servicing the ptrace request at the same time?

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

Quoted 7 lines · Oleg Nesterov

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Quoted 8 lines

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

Ping.

Any idea how to fix this cleanly? May be we can reuse perf_event_mutex, but this looks soooo ugly. And do_exit()->flush_ptrace_hw_breakpoint() has the strange "FIXME:" comment which doesn't help me to understand what can we do.

Probably the best fix is to change this code so that the tracer owns ->ptrace_bps[], not the tracee. But this is not trivial, and needs a lot of changes in ptrace code.

I am reading perf_event.c, but all I found so far is a couple of trivial methods to crash the kernel via sys_perf_event_open(), will report tomorrow...

Quoted 19 lines · Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

I am trying to understand the usage of hw-breakpoints in arch_ptrace(). ptrace_set_debugreg() and related code looks obviously racy. Nothing protects us against flush_ptrace_hw_breakpoint() called by the dying tracee. Afaics we can leak perf_event or use the already freed memory or both.

Quoted 8 lines

Am I missed something?

Looking into the git history, I don't even know which patch should be blamed (if I am right), there were too many changes. I noticed that 2ebd4ffb6d0cb877787b1e42be8485820158857e "perf events: Split out task search into helper" moved the PF_EXITING check from find_get_context(). This check coould help if sys_ptrace() races with SIGKILL, but it was racy anyway.

Ping.

Quoted 8 lines

Any idea how to fix this cleanly? May be we can reuse perf_event_mutex, but this looks soooo ugly. And do_exit()->flush_ptrace_hw_breakpoint() has the strange "FIXME:" comment which doesn't help me to understand what can we do.

Probably the best fix is to change this code so that the tracer owns ->ptrace_bps[], not the tracee. But this is not trivial, and needs a lot of changes in ptrace code.

Wasn't this sorted by: 8882135bcd332f294df5455747ea43ba9e6f77ad?

Or is this purely related to the ptrace muck? in which case I guess Frederic is you man, I never looked at the hw_breakpoint stuff in general and the ptrace bits in particular.

Wasn't this sorted by: 8882135bcd332f294df5455747ea43ba9e6f77ad?

Quoted 3 lines

I am reading perf_event.c, but all I found so far is a couple of trivial methods to crash the kernel via sys_perf_event_open(), will report tomorrow...

Ow, that's not too pretty..