perf: use-after-free in perf_release

Quoted 57 lines · Peter Zijlstra

Quoted 6 lines · Dmitry Vyukov

==================================================================

BUG: KASAN: use-after-free in atomic_dec_and_test
arch/x86/include/asm/atomic.h:123 [inline] at addr ffff880079c30158
BUG: KASAN: use-after-free in put_task_struct
include/linux/sched/task.h:93 [inline] at addr ffff880079c30158
BUG: KASAN: use-after-free in put_ctx+0xcf/0x110

FWIW, this output is very confusing, is this a result of your post-processing replicating the line for every 'inlined' part?

Quoted 12 lines

kernel/events/core.c:1131 at addr ffff880079c30158

Write of size 4 by task syz-executor6/25698

 atomic_dec_and_test arch/x86/include/asm/atomic.h:123 [inline]
 put_task_struct include/linux/sched/task.h:93 [inline]
 put_ctx+0xcf/0x110 kernel/events/core.c:1131
 perf_event_release_kernel+0x3ad/0xc90 kernel/events/core.c:4322
 perf_release+0x37/0x50 kernel/events/core.c:4338
 __fput+0x332/0x800 fs/file_table.c:209
 ____fput+0x15/0x20 fs/file_table.c:245
 task_work_run+0x197/0x260 kernel/task_work.c:116
 exit_task_work include/linux/task_work.h:21 [inline]

Quoted 8 lines

 do_exit+0xb38/0x29c0 kernel/exit.c:880
 do_group_exit+0x149/0x420 kernel/exit.c:984
 get_signal+0x7e0/0x1820 kernel/signal.c:2318
 do_signal+0xd2/0x2190 arch/x86/kernel/signal.c:808
 exit_to_usermode_loop+0x200/0x2a0 arch/x86/entry/common.c:157
 syscall_return_slowpath arch/x86/entry/common.c:191 [inline]
 do_syscall_64+0x6fc/0x930 arch/x86/entry/common.c:286
 entry_SYSCALL64_slow_path+0x25/0x25

So this is fput()..

Quoted 17 lines

Freed:
PID = 25681

 save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:513
 set_track mm/kasan/kasan.c:525 [inline]
 kasan_slab_free+0x6f/0xb0 mm/kasan/kasan.c:589
 __cache_free mm/slab.c:3514 [inline]
 kmem_cache_free+0x71/0x240 mm/slab.c:3774
 free_task_struct kernel/fork.c:158 [inline]
 free_task+0x151/0x1d0 kernel/fork.c:370
 copy_process.part.38+0x18e5/0x4aa0 kernel/fork.c:1931
 copy_process kernel/fork.c:1531 [inline]
 _do_fork+0x200/0x1010 kernel/fork.c:1994
 SYSC_clone kernel/fork.c:2104 [inline]
 SyS_clone+0x37/0x50 kernel/fork.c:2098
 do_syscall_64+0x2e8/0x930 arch/x86/entry/common.c:281
 return_from_SYSCALL_64+0x0/0x7a

and this is a failed fork().

Quoted 8 lines

However, inherited events don't have a filedesc to fput(), and similarly, a task that fails for has never been visible to attach a perf event to because it never hits the pid-hash.
Or so it is assumed.
I'm forever getting lost in the PID code. Oleg, is there any way find_task_by_vpid() can return a task that can still fail fork() ?

So I _think_ find_task_by_vpid() can return an already dead task; and we'll happily increase task->usage.

Dmitry; I have no idea how easy it is for you to reproduce the thing; but so far I've not had much success. Could you perhaps stick the below in?

Once we convert task_struct to refcount_t that should generate a WARN of its own I suppose.

---

include/linux/perf_event.h +1 -0

index 000fdb2..612d652 100644
@@ -763,6 +763,7 @@ struct perf_event_context {
 #ifdef CONFIG_CGROUP_PERF
 	int				nr_cgroups;	 /* cgroup evts */
 #endif
+	int				switches;
 	void				*task_ctx_data; /* pmu specific data */
 	struct rcu_head			rcu_head;
 };

kernel/events/core.c +12 -0

index 6f41548f..6455b7a 100644
@@ -2902,6 +2902,8 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
 	if (!parent && !next_parent)
 		goto unlock;
 
+	ctx->switches++;
+
 	if (next_parent == ctx || next_ctx == parent || next_parent == parent) {
 		/*
 		 * Looks like the two contexts are clones, so we might be

@@ -3780,6 +3782,12 @@ find_lively_task_by_vpid(pid_t vpid)
 		task = current;
 	else
 		task = find_task_by_vpid(vpid);
+
+	if (task) {
+		if (WARN_ON_ONCE(task->flags & PF_EXITING))
+			task = NULL;
+	}
+
 	if (task)
 		get_task_struct(task);
 	rcu_read_unlock();

@@ -10432,6 +10440,10 @@ void perf_event_free_task(struct task_struct *task)
 
 		mutex_unlock(&ctx->mutex);
 
+		WARN_ON_ONCE(ctx->switches);
+		WARN_ON_ONCE(atomic_read(&ctx->refcount) != 1);
+		WARN_ON_ONCE(ctx->task != task);
+
 		put_ctx(ctx);
 	}
 }

Peter Zijlstra

maintainer

2017-03-07 14:00 UTC #14 lore raw

Quoted 2 lines · Peter Zijlstra

So I _think_ find_task_by_vpid() can return an already dead task; and we'll happily increase task->usage.

Hurm, so find_get_context() already does the PF_EXITING test. And then the put_ctx would've been from find_get_context(), not fput().

So still puzzled.

Oleg Nesterov

2017-03-07 14:06 UTC #15 lore raw

In reply to Oleg Nesterov

Quoted 7 lines · Peter Zijlstra

and this is a failed fork().
However, inherited events don't have a filedesc to fput(), and similarly, a task that fails for has never been visible to attach a perf event to because it never hits the pid-hash.

Yes, it is not visible to find_task_by_vpid() until copy_process() does attach_pid(PIDTYPE_PID), and copy_process() can't fail after that.

Oleg.

Dmitry Vyukov

author

2017-03-07 14:27 UTC #16 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 7 lines · Peter Zijlstra
and this is a failed fork().
However, inherited events don't have a filedesc to fput(), and similarly, a task that fails for has never been visible to attach a perf event to because it never hits the pid-hash.
Yes, it is not visible to find_task_by_vpid() until copy_process() does attach_pid(PIDTYPE_PID), and copy_process() can't fail after that.

I would what is that that is failed in copy_process. Could it be perf_event_init_task itself? Maybe it leaves a pointer to p in some shared state on some error conditions?

Oleg Nesterov

2017-03-07 17:45 UTC #17 lore raw

Quoted 11 lines · Dmitry Vyukov

Quoted 9 lines · Oleg Nesterov
Quoted 7 lines · Peter Zijlstra
and this is a failed fork().
However, inherited events don't have a filedesc to fput(), and similarly, a task that fails for has never been visible to attach a perf event to because it never hits the pid-hash.

Quoted 7 lines

Quoted 2 lines
Yes, it is not visible to find_task_by_vpid() until copy_process() does attach_pid(PIDTYPE_PID), and copy_process() can't fail after that.
I would what is that that is failed in copy_process. Could it be perf_event_init_task itself? Maybe it leaves a pointer to p in some shared state on some error conditions?

I am looking at perf_event_init_task() too and I can't understand the error handling...

perf_event_init_context() can return success even if inherit_task_group() in the first list_for_each_entry(pinned_groups) fails, "ret" will be overwritten by the 2nd list_for_each_entry(flexible_groups) loop. "inherited_all" should be cleared, still this looks confusing at least.

inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR(). Is it correct?

Oleg.

Peter Zijlstra

maintainer

2017-03-07 17:30 UTC #18 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Dmitry Vyukov
I would what is that that is failed in copy_process. Could it be perf_event_init_task itself? Maybe it leaves a pointer to p in some shared state on some error conditions?
I am looking at perf_event_init_task() too and I can't understand the error handling...

Quoted 7 lines

perf_event_init_context() can return success even if inherit_task_group() in the first list_for_each_entry(pinned_groups) fails, "ret" will be overwritten by the 2nd list_for_each_entry(flexible_groups) loop. "inherited_all" should be cleared, still this looks confusing at least.
inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR(). Is it correct?

Urgh, there was something tricky there, but I cannot remember, and it seems we didn't put a comment in either :/

Alexander, can you remember?

But yes, this all looks a tad dodgy, I'll try and have a look, but I feel like I'm coming down with something :-(

Peter Zijlstra

maintainer

2017-03-14 12:55 UTC #19 lore raw

Quoted 7 lines · Oleg Nesterov

I am looking at perf_event_init_task() too and I can't understand the error handling...
perf_event_init_context() can return success even if inherit_task_group() in the first list_for_each_entry(pinned_groups) fails, "ret" will be overwritten by the 2nd list_for_each_entry(flexible_groups) loop. "inherited_all" should be cleared, still this looks confusing at least.

Yes, this looks buggy. But I cannot explain how that would result in the observed use-after-free. If we 'loose' an error this way, perf_event_init_context() will not fail, and hence we'll not hit that perf_event_free_task() path.

The task would continue living with an incorrectly copied context, and counter behaviour would be affected.

Quoted 2 lines

inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR(). Is it correct?

Yes. This is all a tad tricky, but it seems to be correct.

By returning NULL, not an error, we affect the silent discard of orphaned events. This is correct, because otherwise perf_event_release_kernel() would have come by and explicitly discarded those events for us anyway.

My current patch is below; won't do much good I suspect..

---
 kernel/events/core.c | 55 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 18 deletions(-)

kernel/events/core.c +37 ~~-18~~

index a17ed56c8ce1..3cd907e00377 100644
@@ -4256,7 +4256,7 @@ int perf_event_release_kernel(struct perf_event *event)
 
 	raw_spin_lock_irq(&ctx->lock);
 	/*
-	 * Mark this even as STATE_DEAD, there is no external reference to it
+	 * Mark this event as STATE_DEAD, there is no external reference to it
 	 * anymore.
 	 *
 	 * Anybody acquiring event->child_mutex after the below loop _must_

@@ -10417,21 +10417,9 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 
 		mutex_lock(&ctx->mutex);
-again:
-		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
-				group_entry)
-			perf_free_event(event, ctx);
-
-		list_for_each_entry_safe(event, tmp, &ctx->flexible_groups,
-				group_entry)
+		list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry)
 			perf_free_event(event, ctx);
-
-		if (!list_empty(&ctx->pinned_groups) ||
-				!list_empty(&ctx->flexible_groups))
-			goto again;
-
 		mutex_unlock(&ctx->mutex);
-
 		put_ctx(ctx);
 	}
 }

@@ -10469,7 +10457,12 @@ const struct perf_event_attr *perf_event_attrs(struct perf_event *event)
 }
 
 /*
- * inherit a event from parent task to child task:
+ * Inherit a event from parent task to child task.
+ *
+ * Returns:
+ *  - valid pointer on success
+ *  - NULL for orphaned events
+ *  - IS_ERR() on error
  */
 static struct perf_event *
 inherit_event(struct perf_event *parent_event,

@@ -10563,6 +10556,16 @@ inherit_event(struct perf_event *parent_event,
 	return child_event;
 }
 
+/*
+ * Inherits an event group.
+ *
+ * This will quietly suppress orphaned events; !inherit_event() is not an error.
+ * This matches with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int inherit_group(struct perf_event *parent_event,
 	      struct task_struct *parent,
 	      struct perf_event_context *parent_ctx,

@@ -10577,6 +10580,11 @@ static int inherit_group(struct perf_event *parent_event,
 				 child, NULL, child_ctx);
 	if (IS_ERR(leader))
 		return PTR_ERR(leader);
+	/*
+	 * @leader can be NULL here because of is_orphaned_event(). In this
+	 * case inherit_event() will create individual events, similar to what
+	 * perf_group_detach() would do anyway.
+	 */
 	list_for_each_entry(sub, &parent_event->sibling_list, group_entry) {
 		child_ctr = inherit_event(sub, parent, parent_ctx,
 					    child, leader, child_ctx);

@@ -10586,6 +10594,17 @@ static int inherit_group(struct perf_event *parent_event,
 	return 0;
 }
 
+/*
+ * Creates the child task context and tries to inherit the event-group.
+ *
+ * Clears @inherited_all on !attr.inherited or error. Note that we'll leave
+ * inherited_all set when we 'fail' to inherit an orphaned event; this is
+ * consistent with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int
 inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		   struct perf_event_context *parent_ctx,

@@ -10608,7 +10627,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-
 		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
 		if (!child_ctx)
 			return -ENOMEM;

@@ -10670,7 +10688,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	/*

@@ -10686,7 +10704,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	raw_spin_lock_irqsave(&parent_ctx->lock, flags);

@@ -10714,6 +10732,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	}
 
 	raw_spin_unlock_irqrestore(&parent_ctx->lock, flags);
+out_unlock:
 	mutex_unlock(&parent_ctx->mutex);
 
 	perf_unpin_context(parent_ctx);

Oleg Nesterov

2017-03-14 13:26 UTC #20 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Oleg Nesterov
inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR(). Is it correct?

Quoted 6 lines

Yes. This is all a tad tricky, but it seems to be correct.
By returning NULL, not an error, we affect the silent discard of orphaned events. This is correct, because otherwise perf_event_release_kernel() would have come by and explicitly discarded those events for us anyway.

Thanks... I'll try to understand this later.

Quoted 18 lines

kernel/events/core.c +1 -2

@@ -10608,7 +10627,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-
 		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
 		if (!child_ctx)
 			return -ENOMEM;

@@ -10670,7 +10688,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	/*

kernel/events/core.c static int perf_event_init_context(struct task_struct *child, int ctxn) · line 10707

Quoted 5 lines

kernel/events/core.c +1 -1

@@ -10686,7 +10704,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;

With this change you can also simplify inherit_task_group() a little bit, it no longer needs to nullify *inherited_all if inherit_group() fails.

Oleg.

Peter Zijlstra

maintainer

2017-03-14 13:47 UTC #21 lore raw

Quoted 32 lines · Oleg Nesterov

Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Oleg Nesterov
inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR(). Is it correct?
Quoted 6 lines
Yes. This is all a tad tricky, but it seems to be correct.
By returning NULL, not an error, we affect the silent discard of orphaned events. This is correct, because otherwise perf_event_release_kernel() would have come by and explicitly discarded those events for us anyway.
Thanks... I'll try to understand this later.
Quoted 16 lines
kernel/events/core.c +1 -2
@@ -10608,7 +10627,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-
 		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
 		if (!child_ctx)
 			return -ENOMEM;
@@ -10670,7 +10688,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 

Quoted 10 lines

kernel/events/core.c static int perf_event_init_context(struct task_struct *child, int ctxn) · line 10707
Quoted 7 lines
kernel/events/core.c +1 -1
 	/*
@@ -10686,7 +10704,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
With this change you can also simplify inherit_task_group() a little bit, it no longer needs to nullify *inherited_all if inherit_group() fails.

Ah, that last one is broken because then we forget to re-enable parent_ctx->rotate_disable.

So if we keep that a break, we still need that inherited_all thing as well.

Oleg Nesterov

2017-03-14 14:06 UTC #22 lore raw

Quoted 3 lines · Peter Zijlstra

Yes, this looks buggy. But I cannot explain how that would result in the observed use-after-free.

Yes...

Suppose that copy_process() fails after perf_event_init_task(). In this case perf_event_free_task() does put_ctx(), but if this ctx has another reference (ctx->refcount > 1) then ctx->task will point to the already freed task, copy_process() does free_task() at the end of error path. And we can't replace it with put_task_struct().

I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should use it too?

Oleg.

Peter Zijlstra

maintainer

2017-03-14 14:09 UTC #23 lore raw

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Peter Zijlstra
Yes, this looks buggy. But I cannot explain how that would result in the observed use-after-free.
Yes...

Quoted 8 lines

Suppose that copy_process() fails after perf_event_init_task(). In this case perf_event_free_task() does put_ctx(), but if this ctx has another reference (ctx->refcount > 1) then ctx->task will point to the already freed task, copy_process() does free_task() at the end of error path. And we can't replace it with put_task_struct().
I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should use it too?

The idea was that the task isn't visible when we use perf_event_free_task(). But I'll have a look.

Oleg Nesterov

2017-03-14 14:32 UTC #24 lore raw

Quoted 15 lines · Peter Zijlstra

Quoted 13 lines · Oleg Nesterov
Quoted 3 lines · Peter Zijlstra
Yes, this looks buggy. But I cannot explain how that would result in the observed use-after-free.
Yes...
Suppose that copy_process() fails after perf_event_init_task(). In this case perf_event_free_task() does put_ctx(), but if this ctx has another reference (ctx->refcount > 1) then ctx->task will point to the already freed task, copy_process() does free_task() at the end of error path. And we can't replace it with put_task_struct().

Quoted 5 lines

Quoted 2 lines
I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should use it too?
The idea was that the task isn't visible when we use perf_event_free_task(). But I'll have a look.

I can be easily wrong, I do not understans this code.

But. perf_event_init_task() adds child_event to parent_event->child_list.

If perf_event_release_kernel(parent_event) is called before copy_process() does perf_event_free_task() which (in particular) removes it from child_list, perf_event_release_kernel() can find this child_event and do get_ctx(ctx) (under the list_for_each_entry(child, &event->child_list, child_list) loop).

Then it does put_ctx(ctx), but ctx->task can be already freed by copy_process()->free_task() in this case.

No?

Oleg.

Peter Zijlstra

maintainer

2017-03-14 15:03 UTC #25 lore raw

Quoted 6 lines · Oleg Nesterov

But. perf_event_init_task() adds child_event to parent_event->child_list.
If perf_event_release_kernel(parent_event) is called before copy_process() does perf_event_free_task() which (in particular) removes it from child_list, perf_event_release_kernel() can find this child_event and do get_ctx(ctx) (under the list_for_each_entry(child, &event->child_list, child_list) loop).

Right; the child_list is the only thing that is exposed. And yes, it looks like that can interleave just right.

Quoted 2 lines

Then it does put_ctx(ctx), but ctx->task can be already freed by copy_process()->free_task() in this case.

	Task1				Task2

	fork()
	  perf_event_init_task()
	  /* ... */
	  goto bad_fork_$foo;
	  /* ... */
	  perf_event_free_task()
	    mutex_lock(ctx->lock)
	    perf_free_event(B)

					perf_event_release_kernel(A)
					  mutex_lock(A->child_mutex)
					  list_for_each_entry(child, ...) {
					    /* child == B */
					    ctx = B->ctx;
					    get_ctx(ctx);
					    mutex_unlock(A->child_mutex);

	      mutex_lock(A->child_mutex)
	      list_del_init(B->child_list)
	      mutex_unlock(A->child_mutex)

	      /* ... */

	    mutex_unlock(ctx->lock);
	    put_ctx() /* >0 */
	  free_task();
					    mutex_lock(ctx->lock);
					    mutex_lock(A->child_mutex);
					    /* ... */
					    mutex_unlock(A->child_mutex);
					    mutex_unlock(ctx->lock)
					    put_ctx() /* 0 */
					      ctx->task && !TOMBSTONE
					        put_task_struct() /* UAF */

Something like that, right?

Let me see if it makes sense to retain perf_event_free_task() at all; maybe we should always do perf_event_exit_task().

Peter Zijlstra

maintainer

2017-03-14 15:07 UTC #26 lore raw

Quoted 54 lines · Peter Zijlstra

Quoted 6 lines · Oleg Nesterov
But. perf_event_init_task() adds child_event to parent_event->child_list.
If perf_event_release_kernel(parent_event) is called before copy_process() does perf_event_free_task() which (in particular) removes it from child_list, perf_event_release_kernel() can find this child_event and do get_ctx(ctx) (under the list_for_each_entry(child, &event->child_list, child_list) loop).
Right; the child_list is the only thing that is exposed. And yes, it looks like that can interleave just right.
Quoted 2 lines
Then it does put_ctx(ctx), but ctx->task can be already freed by copy_process()->free_task() in this case.
	Task1				Task2

	fork()
	  perf_event_init_task()
	  /* ... */
	  goto bad_fork_$foo;
	  /* ... */
	  perf_event_free_task()
	    mutex_lock(ctx->lock)
	    perf_free_event(B)

					perf_event_release_kernel(A)
					  mutex_lock(A->child_mutex)
					  list_for_each_entry(child, ...) {
					    /* child == B */
					    ctx = B->ctx;
					    get_ctx(ctx);
					    mutex_unlock(A->child_mutex);

	      mutex_lock(A->child_mutex)
	      list_del_init(B->child_list)
	      mutex_unlock(A->child_mutex)

	      /* ... */

	    mutex_unlock(ctx->lock);
	    put_ctx() /* >0 */
	  free_task();
					    mutex_lock(ctx->lock);
					    mutex_lock(A->child_mutex);
					    /* ... */
					    mutex_unlock(A->child_mutex);
					    mutex_unlock(ctx->lock)
					    put_ctx() /* 0 */
					      ctx->task && !TOMBSTONE
					        put_task_struct() /* UAF */

Quoted 5 lines

Something like that, right?
Let me see if it makes sense to retain perf_event_free_task() at all; maybe we should always do perf_event_exit_task().

Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()? Because in the above scenario we're freeing it with references on.

Oleg Nesterov

2017-03-14 15:39 UTC #27 lore raw

Quoted 3 lines · Peter Zijlstra

Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()? Because in the above scenario we're freeing it with references on.

Not sure, in this case copy_process() should decrement tsk->usage before free_task(), note the atomic_set(&tsk->usage, 2) in dup_task_struct().

Perhaps we should just add WARN_ON(tsk->usage != 2) into copy_process() right before free_task() ?

On the other hand, WARN_ON(atomic_read(&tsk->usage)) looks pointless, the only caller is put_task_struct().

Oleg.

Peter Zijlstra

maintainer

2017-03-14 15:47 UTC #28 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 3 lines · Peter Zijlstra
Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()? Because in the above scenario we're freeing it with references on.
Not sure, in this case copy_process() should decrement tsk->usage before free_task(), note the atomic_set(&tsk->usage, 2) in dup_task_struct().
Perhaps we should just add WARN_ON(tsk->usage != 2) into copy_process() right before free_task() ?

Sure; that works. I'll try that once I'm back home again, to see if there's unexpected fail because other things increment it.

Oleg Nesterov

2017-03-14 15:21 UTC #29 lore raw

In reply to Oleg Nesterov

Quoted 14 lines · Peter Zijlstra

	    mutex_unlock(ctx->lock);
	    put_ctx() /* >0 */
	  free_task();
					    mutex_lock(ctx->lock);
					    mutex_lock(A->child_mutex);
					    /* ... */
					    mutex_unlock(A->child_mutex);
					    mutex_unlock(ctx->lock)
					    put_ctx() /* 0 */
					      ctx->task && !TOMBSTONE
					        put_task_struct() /* UAF */

Quoted 1 line

Something like that, right?

Yes, exactly.

Quoted 2 lines

Let me see if it makes sense to retain perf_event_free_task() at all; maybe we should always do perf_event_exit_task().

Yes, perhaps... but this needs changes too. Say, WARN_ON_ONCE(child != current) in perf_event_exit_task_context(). And even perf_event_task(new => F) does not look right in this case. In fact it would be simply buggy to do this, this task was not fully constructed yet, so even perf_event_pid(task) is not safe.

Oleg.

Peter Zijlstra

maintainer

2017-03-14 15:26 UTC #30 lore raw

Quoted 19 lines · Oleg Nesterov

Quoted 14 lines · Peter Zijlstra

	    mutex_unlock(ctx->lock);
	    put_ctx() /* >0 */
	  free_task();
					    mutex_lock(ctx->lock);
					    mutex_lock(A->child_mutex);
					    /* ... */
					    mutex_unlock(A->child_mutex);
					    mutex_unlock(ctx->lock)
					    put_ctx() /* 0 */
					      ctx->task && !TOMBSTONE
					        put_task_struct() /* UAF */

Quoted 1 line

Something like that, right?

Yes, exactly.

Quoted 7 lines

Quoted 2 lines
Let me see if it makes sense to retain perf_event_free_task() at all; maybe we should always do perf_event_exit_task().
Yes, perhaps... but this needs changes too. Say, WARN_ON_ONCE(child != current) in perf_event_exit_task_context(). And even perf_event_task(new => F) does not look right in this case. In fact it would be simply buggy to do this, this task was not fully constructed yet, so even perf_event_pid(task) is not safe.

Yeah; there's a fair amount of stuff like that. I'm afraid crafting exceptions for all that will just end up with more of a mess than we safe by merging the two :/

A well.. I'll go do the 'trivial' patch then.

Peter Zijlstra

maintainer

2017-03-14 17:37 UTC #31 lore raw

Quoted 1 line · Peter Zijlstra

A well.. I'll go do the 'trivial' patch then.

A little like so; completely untested.

---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

kernel/events/core.c +11 -0

index 110b38a58493..6576449b6029 100644
@@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);
+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);
 again:
 		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
 				group_entry)

Oleg Nesterov

2017-03-15 16:45 UTC #32 lore raw

Quoted 8 lines · Peter Zijlstra

kernel/events/core.c +1 -0

@@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);

kernel/events/core.c void perf_event_free_task(struct task_struct *task) · line 10359

Quoted 10 lines

kernel/events/core.c +10 -0

+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);

Agreed, this is what I had in mind. Although you know, I spent 3 hours looking at your patch and I still can't convince myself I am really sure it closes all races ;)

OK, I believe this is correct. And iiuc both RCU_INIT_POINTER(NULL) and put_task_struct() are not strictly necessary? At least until we add WARN_ON(tsk->usage != 2) before free_task() in copy process().

---------------------------------------------------------------------
This is off-topic, but to me list_for_each_entry(event->child_list) in perf_event_release_kernel() looks very confusing and misleading. And list_first_entry_or_null(), we do not really need NULL if list is empty, tmp == child should be F even if we use list_first_entry(). And given that we already have list_is_last(), it would be nice to add list_is_first() and cleanup perf_event_release_kernel() a bit:

	--- x/kernel/events/core.c
	+++ x/kernel/events/core.c
	@@ -4152,7 +4152,7 @@ static void put_event(struct perf_event 
	 int perf_event_release_kernel(struct perf_event *event)
	 {
		struct perf_event_context *ctx = event->ctx;
	-	struct perf_event *child, *tmp;
	+	struct perf_event *child;
	 
		/*
		 * If we got here through err_file: fput(event_file); we will not have
	@@ -4190,8 +4190,9 @@ int perf_event_release_kernel(struct per
	 
	 again:
		mutex_lock(&event->child_mutex);
	-	list_for_each_entry(child, &event->child_list, child_list) {
	-
	+	if (!list_empty(&event->child_list)) {
	+		child = list_first_entry(&event->child_list,
	+					 struct perf_event, child_list);
			/*
			 * Cannot change, child events are not migrated, see the
			 * comment with perf_event_ctx_lock_nested().
	@@ -4221,9 +4222,7 @@ again:
			 * state, if child is still the first entry, it didn't get freed
			 * and we can continue doing so.
			 */
	-		tmp = list_first_entry_or_null(&event->child_list,
	-					       struct perf_event, child_list);
	-		if (tmp == child) {
	+		if (list_is_first(child, &event->child_list)) {
				perf_remove_from_context(child, DETACH_GROUP);
				list_del(&child->child_list);
				free_event(child);

But we can't, because

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return list->prev == head;
	}

won't work, "child" can be freed so we can't dereference it, and

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return head->next == list;
	}

won't be symmetrical with list_is_last() we already have.

Oleg.

Peter Zijlstra

maintainer

2017-03-16 12:05 UTC #33 lore raw

Quoted 7 lines · Oleg Nesterov

Quoted 6 lines · Peter Zijlstra
kernel/events/core.c +0 -0
@@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 

Quoted 16 lines

kernel/events/core.c void perf_event_free_task(struct task_struct *task) · line 10359
Quoted 12 lines
kernel/events/core.c +11 -0
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);
+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);
Agreed, this is what I had in mind. Although you know, I spent 3 hours looking at your patch and I still can't convince myself I am really sure it closes all races ;)

Ha; yes I know that feeling. I used to have a few sheets of paper filled with diagrams. Sadly I could not find them again. Must've been over eager cleaning my desk at some point.

Quoted 4 lines

OK, I believe this is correct. And iiuc both RCU_INIT_POINTER(NULL) and put_task_struct() are not strictly necessary? At least until we add WARN_ON(tsk->usage != 2) before free_task() in copy process().

Right; I just kept the code similar to the other location. I even considered making a helper function to not duplicate, but in the end decided against it.

Quoted 8 lines

---------------------------------------------------------------------
This is off-topic, but to me list_for_each_entry(event->child_list) in perf_event_release_kernel() looks very confusing and misleading. And list_first_entry_or_null(), we do not really need NULL if list is empty, tmp == child should be F even if we use list_first_entry(). And given that we already have list_is_last(), it would be nice to add list_is_first() and cleanup perf_event_release_kernel() a bit:

Agreed; its a bit of a weird one.

Let me go write proper patches for the things we have so far though.

Peter Zijlstra

maintainer

2017-03-16 13:58 UTC #34 lore raw

Quoted 7 lines · Oleg Nesterov

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return head->next == list;
	}

won't be symmetrical with list_is_last() we already have.

This is the one that makes sense to me though; that is, the current list_is_last() doesn't make sense to me.

I would expect:

static inline int list_is_last(const struct list_head *list,
const struct list_head *head)

{
	return head->prev == list
}

because @head is the list argument (yes, I know, horrible naming!).

Oleg Nesterov

2017-03-16 16:43 UTC #35 lore raw

Quoted 13 lines · Peter Zijlstra

Quoted 7 lines · Oleg Nesterov
	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return head->next == list;
	}
won't be symmetrical with list_is_last() we already have.
This is the one that makes sense to me though; that is, the current list_is_last() doesn't make sense to me.

Quoted 7 lines

I would expect:
static inline int list_is_last(const struct list_head *list,
const struct list_head *head)
{
	return head->prev == list
}

Yes!

Quoted 1 line

because @head is the list argument (yes, I know, horrible naming!).

and perhaps it could have more users if we redefine it to dereference "head" which is likely more "stable", iow less likely can go away.

But after the quick grep I came to conclusion it is not possible to audit the users it already has.

Oleg.

Peter Zijlstra

maintainer

2017-03-16 13:00 UTC #36 lore raw

cover perf patches

Hi,

These 4 patches are the result of Dmitry reporting a use-after-free.

The first two are /urgent material, the last two can go in /core.

Peter Zijlstra

maintainer

2017-03-16 13:00 UTC #37 lore raw

PATCH 1/4 perf: Fix use-after-free in perf_release()

Dmitry reported syzcaller tripped a use-after-free in perf_release().

After much puzzlement Oleg spotted the below scenario:

Task1 Task2

fork()

    perf_event_init_task()
    /* ... */
    goto bad_fork_$foo;
    /* ... */
    perf_event_free_task()
      mutex_lock(ctx->lock)
      perf_free_event(B)

                                  perf_event_release_kernel(A)
                                    mutex_lock(A->child_mutex)
                                    list_for_each_entry(child, ...) {
                                      /* child == B */
                                      ctx = B->ctx;
                                      get_ctx(ctx);
                                      mutex_unlock(A->child_mutex);

        mutex_lock(A->child_mutex)
        list_del_init(B->child_list)
        mutex_unlock(A->child_mutex)

        /* ... */

      mutex_unlock(ctx->lock);
      put_ctx() /* >0 */
    free_task();
                                      mutex_lock(ctx->lock);
                                      mutex_lock(A->child_mutex);
                                      /* ... */
                                      mutex_unlock(A->child_mutex);
                                      mutex_unlock(ctx->lock)
                                      put_ctx() /* 0 */
                                        ctx->task && !TOMBSTONE
                                          put_task_struct() /* UAF */

This patch closes the hole by making perf_event_free_task() destroy the task <-> ctx relation such that perf_event_release_kernel() will no longer observe the now dead task.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Alexander Shishkin <[email protected]>

Fixes c6e5b73242d2 ("perf: Synchronously clean up child events")

Reported-by Dmitry Vyukov <[email protected]>

Link http://lkml.kernel.org/r/20170314155949.GE32474@worktop open here

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

---
 kernel/events/core.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

kernel/events/core.c +11 -0

@@ -10556,6 +10556,17 @@ void perf_event_free_task(struct task_st
 			continue;
 
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);
+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);
 again:
 		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
 				group_entry)

tip-bot for Peter Zijlstra

merge bot

2017-03-16 15:43 UTC #38 lore raw

[tip:perf/urgent] perf/core: Fix use-after-free in perf_release()

perf/core: Fix use-after-free in perf_release()

Commit-ID:  e552a8389aa409e257b7dcba74f67f128f979ccc
Gitweb:     http://git.kernel.org/tip/e552a8389aa409e257b7dcba74f67f128f979ccc
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 16 Mar 2017 13:47:48 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 16 Mar 2017 14:16:52 +0100

perf/core: Fix use-after-free in perf_release()

Dmitry reported syzcaller tripped a use-after-free in perf_release().

After much puzzlement Oleg spotted the below scenario:

Task1 Task2

fork()

    perf_event_init_task()
    /* ... */
    goto bad_fork_$foo;
    /* ... */
    perf_event_free_task()
      mutex_lock(ctx->lock)
      perf_free_event(B)

                                  perf_event_release_kernel(A)
                                    mutex_lock(A->child_mutex)
                                    list_for_each_entry(child, ...) {
                                      /* child == B */
                                      ctx = B->ctx;
                                      get_ctx(ctx);
                                      mutex_unlock(A->child_mutex);

        mutex_lock(A->child_mutex)
        list_del_init(B->child_list)
        mutex_unlock(A->child_mutex)

        /* ... */

      mutex_unlock(ctx->lock);
      put_ctx() /* >0 */
    free_task();
                                      mutex_lock(ctx->lock);
                                      mutex_lock(A->child_mutex);
                                      /* ... */
                                      mutex_unlock(A->child_mutex);
                                      mutex_unlock(ctx->lock)
                                      put_ctx() /* 0 */
                                        ctx->task && !TOMBSTONE
                                          put_task_struct() /* UAF */

This patch closes the hole by making perf_event_free_task() destroy the task <-> ctx relation such that perf_event_release_kernel() will no longer observe the now dead task.

Reported-by Dmitry Vyukov <[email protected]>

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

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/20170314155949.GE32474@worktop open here

Fixes c6e5b73242d2 ("perf: Synchronously clean up child events")

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

kernel/events/core.c +11 -0

index 1031bdf..4742909 100644
@@ -10415,6 +10415,17 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);
+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);
 again:
 		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
 				group_entry)

Peter Zijlstra

maintainer

2017-03-16 12:47 UTC #39 lore raw

PATCH 2/4 perf: Fix event inheritance on fork()

While hunting for clues to a use-after-free, Oleg spotted that perf_event_init_context() can loose an error value with the result that fork() can succeed even though we did not fully inherit the perf event context.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Fixes 889ff0150661 ("perf/core: Split context's event group list into pinned and non-pinned lists")

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

---
 kernel/events/core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/events/core.c +3 -2

@@ -10820,7 +10819,7 @@ static int perf_event_init_context(struc
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	/*

@@ -10836,7 +10835,7 @@ static int perf_event_init_context(struc
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	raw_spin_lock_irqsave(&parent_ctx->lock, flags);

@@ -10864,6 +10863,7 @@ static int perf_event_init_context(struc
 	}
 
 	raw_spin_unlock_irqrestore(&parent_ctx->lock, flags);
+out_unlock:
 	mutex_unlock(&parent_ctx->mutex);
 
 	perf_unpin_context(parent_ctx);

tip-bot for Peter Zijlstra

merge bot

2017-03-16 15:23 UTC #40 lore raw

[tip:perf/urgent] perf/core: Fix event inheritance on fork()

perf/core: Fix event inheritance on fork()

Commit-ID:  e7cc4865f0f31698ef2f7aac01a50e78968985b7
Gitweb:     http://git.kernel.org/tip/e7cc4865f0f31698ef2f7aac01a50e78968985b7
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 16 Mar 2017 13:47:49 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 16 Mar 2017 14:16:52 +0100

perf/core: Fix event inheritance on fork()

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

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Fixes 889ff0150661 ("perf/core: Split context's event group list into pinned and non-pinned lists")

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/events/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/events/core.c +3 -2

index 4742909..fc7c9a8 100644
@@ -10679,7 +10679,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	/*

@@ -10695,7 +10695,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
-			break;
+			goto out_unlock;
 	}
 
 	raw_spin_lock_irqsave(&parent_ctx->lock, flags);

@@ -10723,6 +10723,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	}
 
 	raw_spin_unlock_irqrestore(&parent_ctx->lock, flags);
+out_unlock:
 	mutex_unlock(&parent_ctx->mutex);
 
 	perf_unpin_context(parent_ctx);

Peter Zijlstra

maintainer

2017-03-16 13:01 UTC #41 lore raw

PATCH 3/4 perf: Simplify perf_event_free_task()

We have ctx->event_list that contains all events; no need to repeatedly iterate the group lists to find them all.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Oleg Nesterov <[email protected]>

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

---
 kernel/events/core.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

kernel/events/core.c +1 ~~-11~~

@@ -10567,21 +10567,11 @@ void perf_event_free_task(struct task_st
 		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
 		put_task_struct(task); /* cannot be last */
 		raw_spin_unlock_irq(&ctx->lock);
-again:
-		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
-				group_entry)
-			perf_free_event(event, ctx);
 
-		list_for_each_entry_safe(event, tmp, &ctx->flexible_groups,
-				group_entry)
+		list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry)
 			perf_free_event(event, ctx);
 
-		if (!list_empty(&ctx->pinned_groups) ||
-				!list_empty(&ctx->flexible_groups))
-			goto again;
-
 		mutex_unlock(&ctx->mutex);
-
 		put_ctx(ctx);
 	}
 }

tip-bot for Peter Zijlstra

merge bot

2017-03-16 15:22 UTC #42 lore raw

[tip:perf/urgent] perf/core: Simplify perf_event_free_task()

perf/core: Simplify perf_event_free_task()

Commit-ID:  15121c789e001168decac6483d192bdb7ea29e74
Gitweb:     http://git.kernel.org/tip/15121c789e001168decac6483d192bdb7ea29e74
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 16 Mar 2017 13:47:50 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 16 Mar 2017 14:16:53 +0100

perf/core: Simplify perf_event_free_task()

We have ctx->event_list that contains all events; no need to repeatedly iterate the group lists to find them all.

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

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/events/core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

kernel/events/core.c +1 ~~-11~~

index fc7c9a8..5f21e5e 100644
@@ -10426,21 +10426,11 @@ void perf_event_free_task(struct task_struct *task)
 		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
 		put_task_struct(task); /* cannot be last */
 		raw_spin_unlock_irq(&ctx->lock);
-again:
-		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
-				group_entry)
-			perf_free_event(event, ctx);
 
-		list_for_each_entry_safe(event, tmp, &ctx->flexible_groups,
-				group_entry)
+		list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry)
 			perf_free_event(event, ctx);
 
-		if (!list_empty(&ctx->pinned_groups) ||
-				!list_empty(&ctx->flexible_groups))
-			goto again;
-
 		mutex_unlock(&ctx->mutex);
-
 		put_ctx(ctx);
 	}
 }

Peter Zijlstra

maintainer

2017-03-16 13:00 UTC #43 lore raw

PATCH 4/4 perf: Better explain the inherit magic

While going through the event inheritance code Oleg got confused.

Add some comments to better explain the silent dissapearance of orphaned events.

So what happens is that at perf_event_release_kernel() time; when an event looses its connection to userspace (and ceases to exist from the user's perspective) we can still have an arbitrary amount of inherited copies of the event. We want to synchronously find and remove all these child events.

Since that requires a bit of lock juggling, there is the possibility that concurrent clone()s will create new child events. Therefore we first mark the parent event as DEAD, which marks all the extant child events as orphaned.

We then avoid copying orphaned events; in order to avoid getting more of them.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Oleg Nesterov <[email protected]>

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

---
 kernel/events/core.c |   36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

kernel/events/core.c +33 -3

@@ -4261,7 +4261,7 @@ int perf_event_release_kernel(struct per
 
 	raw_spin_lock_irq(&ctx->lock);
 	/*
-	 * Mark this even as STATE_DEAD, there is no external reference to it
+	 * Mark this event as STATE_DEAD, there is no external reference to it
 	 * anymore.
 	 *
 	 * Anybody acquiring event->child_mutex after the below loop _must_

@@ -10609,7 +10609,12 @@ const struct perf_event_attr *perf_event
 }
 
 /*
- * inherit a event from parent task to child task:
+ * Inherit a event from parent task to child task.
+ *
+ * Returns:
+ *  - valid pointer on success
+ *  - NULL for orphaned events
+ *  - IS_ERR() on error
  */
 static struct perf_event *
 inherit_event(struct perf_event *parent_event,

@@ -10703,6 +10708,16 @@ inherit_event(struct perf_event *parent_
 	return child_event;
 }
 
+/*
+ * Inherits an event group.
+ *
+ * This will quietly suppress orphaned events; !inherit_event() is not an error.
+ * This matches with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int inherit_group(struct perf_event *parent_event,
 	      struct task_struct *parent,
 	      struct perf_event_context *parent_ctx,

@@ -10717,6 +10732,11 @@ static int inherit_group(struct perf_eve
 				 child, NULL, child_ctx);
 	if (IS_ERR(leader))
 		return PTR_ERR(leader);
+	/*
+	 * @leader can be NULL here because of is_orphaned_event(). In this
+	 * case inherit_event() will create individual events, similar to what
+	 * perf_group_detach() would do anyway.
+	 */
 	list_for_each_entry(sub, &parent_event->sibling_list, group_entry) {
 		child_ctr = inherit_event(sub, parent, parent_ctx,
 					    child, leader, child_ctx);

@@ -10726,6 +10746,17 @@ static int inherit_group(struct perf_eve
 	return 0;
 }
 
+/*
+ * Creates the child task context and tries to inherit the event-group.
+ *
+ * Clears @inherited_all on !attr.inherited or error. Note that we'll leave
+ * inherited_all set when we 'fail' to inherit an orphaned event; this is
+ * consistent with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int
 inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		   struct perf_event_context *parent_ctx,

@@ -10748,7 +10779,6 @@ inherit_task_group(struct perf_event *ev
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-
 		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
 		if (!child_ctx)
 			return -ENOMEM;

tip-bot for Peter Zijlstra

merge bot

2017-03-16 15:23 UTC #44 lore raw

[tip:perf/urgent] perf/core: Better explain the inherit magic

perf/core: Better explain the inherit magic

Commit-ID:  d8a8cfc76919b6c830305266b23ba671623f37ff
Gitweb:     http://git.kernel.org/tip/d8a8cfc76919b6c830305266b23ba671623f37ff
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 16 Mar 2017 13:47:51 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 16 Mar 2017 14:16:53 +0100

perf/core: Better explain the inherit magic

While going through the event inheritance code Oleg got confused.

Add some comments to better explain the silent dissapearance of orphaned events.

We then avoid copying orphaned events; in order to avoid getting more of them.

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

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/events/core.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

kernel/events/core.c +33 -3

index 5f21e5e..7298e14 100644
@@ -4254,7 +4254,7 @@ int perf_event_release_kernel(struct perf_event *event)
 
 	raw_spin_lock_irq(&ctx->lock);
 	/*
-	 * Mark this even as STATE_DEAD, there is no external reference to it
+	 * Mark this event as STATE_DEAD, there is no external reference to it
 	 * anymore.
 	 *
 	 * Anybody acquiring event->child_mutex after the below loop _must_

@@ -10468,7 +10468,12 @@ const struct perf_event_attr *perf_event_attrs(struct perf_event *event)
 }
 
 /*
- * inherit a event from parent task to child task:
+ * Inherit a event from parent task to child task.
+ *
+ * Returns:
+ *  - valid pointer on success
+ *  - NULL for orphaned events
+ *  - IS_ERR() on error
  */
 static struct perf_event *
 inherit_event(struct perf_event *parent_event,

@@ -10562,6 +10567,16 @@ inherit_event(struct perf_event *parent_event,
 	return child_event;
 }
 
+/*
+ * Inherits an event group.
+ *
+ * This will quietly suppress orphaned events; !inherit_event() is not an error.
+ * This matches with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int inherit_group(struct perf_event *parent_event,
 	      struct task_struct *parent,
 	      struct perf_event_context *parent_ctx,

@@ -10576,6 +10591,11 @@ static int inherit_group(struct perf_event *parent_event,
 				 child, NULL, child_ctx);
 	if (IS_ERR(leader))
 		return PTR_ERR(leader);
+	/*
+	 * @leader can be NULL here because of is_orphaned_event(). In this
+	 * case inherit_event() will create individual events, similar to what
+	 * perf_group_detach() would do anyway.
+	 */
 	list_for_each_entry(sub, &parent_event->sibling_list, group_entry) {
 		child_ctr = inherit_event(sub, parent, parent_ctx,
 					    child, leader, child_ctx);

@@ -10585,6 +10605,17 @@ static int inherit_group(struct perf_event *parent_event,
 	return 0;
 }
 
+/*
+ * Creates the child task context and tries to inherit the event-group.
+ *
+ * Clears @inherited_all on !attr.inherited or error. Note that we'll leave
+ * inherited_all set when we 'fail' to inherit an orphaned event; this is
+ * consistent with perf_event_release_kernel() removing all child events.
+ *
+ * Returns:
+ *  - 0 on success
+ *  - <0 on error
+ */
 static int
 inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		   struct perf_event_context *parent_ctx,

@@ -10607,7 +10638,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-
 		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
 		if (!child_ctx)
 			return -ENOMEM;

Ingo Molnar

Applied

2017-03-16 13:20 UTC #45 lore raw

Re: [PATCH 0/4] perf patches