Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310060 > unrolled thread

[PATCH] perf: Cleanup user's child events

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: Cleanup user's child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 12:30 +0100
    Re: [PATCH] perf: Cleanup user's child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 14:00 +0100
      Re: [PATCH] perf: Cleanup user's child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 14:10 +0100
        Re: [PATCH] perf: Cleanup user's child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 14:10 +0100
          [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 15:10 +0100
            Re: [PATCH] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 19:00 +0100
              Re: [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-18 13:20 +0100
              Re: [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-18 13:40 +0100
                Re: [PATCH] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-18 15:50 +0100
                  [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-19 16:20 +0100
                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-19 21:10 +0100
                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-19 21:10 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-19 23:00 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-20 09:40 +0100
                          Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-21 06:00 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-20 08:10 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-20 09:10 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-22 12:40 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-22 13:20 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-22 13:40 +0100
                          Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-22 20:50 +0100
                            Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 12:50 +0100
                              Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 16:00 +0100
                                Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 22:10 +0100
                                  Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-26 06:10 +0100
                                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-26 17:20 +0100
                                      Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-26 18:30 +0100
                                        Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-27 00:40 +0100
                                          Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-27 11:00 +0100
                                            Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-27 19:00 +0100
            Re: [PATCH] perf: Synchronously cleanup child events Ingo Molnar <mingo@kernel.org> - 2016-01-19 08:50 +0100

Page 1 of 2  [1] 2  Next page →

#1310060 — [PATCH] perf: Cleanup user's child events

Events are leaking in the following scenario: user creates an event for
task A, task A forks into B (producing a child event), user closes the
original event. Both original user's event and its child will remain for
as long as task B is around. In other words, we don't clean up children
when we try to release the parent.

This patch cleans up user event's children when its file descriptor is
closed.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 kernel/events/core.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 630f53acce..867c4347ea 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3828,7 +3828,43 @@ EXPORT_SYMBOL_GPL(perf_event_release_kernel);
  */
 static int perf_release(struct inode *inode, struct file *file)
 {
-	put_event(file->private_data);
+	struct perf_event *event = file->private_data, *child, *tmp;
+	LIST_HEAD(child_list);
+
+	/*
+	 * event::child_mutex nests inside ctx::lock, so move children
+	 * to a safe place first and avoid inversion
+	 */
+	mutex_lock(&event->child_mutex);
+	list_splice_init(&event->child_list, &child_list);
+	mutex_unlock(&event->child_mutex);
+
+	list_for_each_entry_safe(child, tmp, &child_list, child_list) {
+		struct perf_event_context *ctx;
+
+		/*
+		 * This is somewhat similar to perf_free_event(),
+		 * except for these events are alive and need
+		 * proper perf_remove_from_context().
+		 */
+		ctx = perf_event_ctx_lock(child);
+		perf_remove_from_context(child, true);
+		perf_event_ctx_unlock(child, ctx);
+
+		list_del(&child->child_list);
+
+		/* Children will have exactly one reference */
+		free_event(child);
+
+		/*
+		 * This matches the refcount bump in inherit_event();
+		 * this can't be the last reference.
+		 */
+		put_event(event);
+	}
+
+	/* Must be the last reference */
+	put_event(event);
 	return 0;
 }
 
-- 
2.7.0.rc3

[toc] | [next] | [standalone]

#1310088

On Fri, Jan 15, 2016 at 01:22:15PM +0200, Alexander Shishkin wrote:
> Events are leaking in the following scenario: user creates an event for
> task A, task A forks into B (producing a child event), user closes the
> original event. Both original user's event and its child will remain for
> as long as task B is around. In other words, we don't clean up children
> when we try to release the parent.

The orphan stuff should clear those up, no?

[toc] | [prev] | [next] | [standalone]

#1310089

Peter Zijlstra <peterz@infradead.org> writes:

> On Fri, Jan 15, 2016 at 01:22:15PM +0200, Alexander Shishkin wrote:
>> Events are leaking in the following scenario: user creates an event for
>> task A, task A forks into B (producing a child event), user closes the
>> original event. Both original user's event and its child will remain for
>> as long as task B is around. In other words, we don't clean up children
>> when we try to release the parent.
>
> The orphan stuff should clear those up, no?

Not if they don't schedule after the parent's gone.

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1310094

On Fri, Jan 15, 2016 at 03:05:33PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > On Fri, Jan 15, 2016 at 01:22:15PM +0200, Alexander Shishkin wrote:
> >> Events are leaking in the following scenario: user creates an event for
> >> task A, task A forks into B (producing a child event), user closes the
> >> original event. Both original user's event and its child will remain for
> >> as long as task B is around. In other words, we don't clean up children
> >> when we try to release the parent.
> >
> > The orphan stuff should clear those up, no?
> 
> Not if they don't schedule after the parent's gone.

This is true. So when Jiri did this we tried the immediate thing and
that exploded due to lock inversions.

You mention some of that. Let me go dig out that old thread to see if
its the same.

I feel that we should not have both approaches.

[toc] | [prev] | [next] | [standalone]

#1310145 — [PATCH] perf: Synchronously cleanup child events

The orphan cleanup workqueue doesn't always catch orphans, for example,
if they never schedule after they are orphaned. IOW, the event leak is
still very real. It also wouldn't work for kernel counters.

Also, there seems to be no reason not to carry out this cleanup
procedure synchronously during parent event's destruction.

This patch replaces the workqueue approach with a simple cleanup round
in the event's destruction path. To avoid racing with clone, we still
check that parent event has an owner in the inheritance path.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h |   3 --
 kernel/events/core.c       | 121 ++++++++++++++++-----------------------------
 2 files changed, 43 insertions(+), 81 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 6612732d8f..cd9c1ace29 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -634,9 +634,6 @@ struct perf_event_context {
 	int				nr_cgroups;	 /* cgroup evts */
 	void				*task_ctx_data; /* pmu specific data */
 	struct rcu_head			rcu_head;
-
-	struct delayed_work		orphans_remove;
-	bool				orphans_remove_sched;
 };
 
 /*
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 630f53acce..8eb3fee429 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,8 +49,6 @@
 
 #include <asm/irq_regs.h>
 
-static struct workqueue_struct *perf_wq;
-
 typedef int (*remote_function_f)(void *);
 
 struct remote_function_call {
@@ -1652,40 +1650,9 @@ out:
  */
 static bool is_orphaned_event(struct perf_event *event)
 {
-	return event && !is_kernel_event(event) && !event->owner;
-}
-
-/*
- * Event has a parent but parent's task finished and it's
- * alive only because of children holding refference.
- */
-static bool is_orphaned_child(struct perf_event *event)
-{
-	return is_orphaned_event(event->parent);
-}
-
-static void orphans_remove_work(struct work_struct *work);
-
-static void schedule_orphans_remove(struct perf_event_context *ctx)
-{
-	if (!ctx->task || ctx->orphans_remove_sched || !perf_wq)
-		return;
-
-	if (queue_delayed_work(perf_wq, &ctx->orphans_remove, 1)) {
-		get_ctx(ctx);
-		ctx->orphans_remove_sched = true;
-	}
-}
-
-static int __init perf_workqueue_init(void)
-{
-	perf_wq = create_singlethread_workqueue("perf");
-	WARN(!perf_wq, "failed to create perf workqueue\n");
-	return perf_wq ? 0 : -1;
+	return event && !event->owner;
 }
 
-core_initcall(perf_workqueue_init);
-
 static inline int pmu_filter_match(struct perf_event *event)
 {
 	struct pmu *pmu = event->pmu;
@@ -1746,9 +1713,6 @@ event_sched_out(struct perf_event *event,
 	if (event->attr.exclusive || !cpuctx->active_oncpu)
 		cpuctx->exclusive = 0;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 	perf_pmu_enable(event->pmu);
 }
 
@@ -1991,9 +1955,6 @@ event_sched_in(struct perf_event *event,
 	if (event->attr.exclusive)
 		cpuctx->exclusive = 1;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 out:
 	perf_pmu_enable(event->pmu);
 
@@ -3370,7 +3331,6 @@ static void __perf_event_init_context(struct perf_event_context *ctx)
 	INIT_LIST_HEAD(&ctx->flexible_groups);
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
-	INIT_DELAYED_WORK(&ctx->orphans_remove, orphans_remove_work);
 }
 
 static struct perf_event_context *
@@ -3818,54 +3778,59 @@ static void put_event(struct perf_event *event)
 
 int perf_event_release_kernel(struct perf_event *event)
 {
-	put_event(event);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(perf_event_release_kernel);
+	struct perf_event *child, *tmp;
+	LIST_HEAD(child_list);
 
-/*
- * Called when the last reference to the file is gone.
- */
-static int perf_release(struct inode *inode, struct file *file)
-{
-	put_event(file->private_data);
-	return 0;
-}
+	if (!is_kernel_event(event))
+		perf_remove_from_owner(event);
 
-/*
- * Remove all orphanes events from the context.
- */
-static void orphans_remove_work(struct work_struct *work)
-{
-	struct perf_event_context *ctx;
-	struct perf_event *event, *tmp;
+	event->owner = NULL;
 
-	ctx = container_of(work, struct perf_event_context,
-			   orphans_remove.work);
+	/*
+	 * event::child_mutex nests inside ctx::lock, so move children
+	 * to a safe place first and avoid inversion
+	 */
+	mutex_lock(&event->child_mutex);
+	list_splice_init(&event->child_list, &child_list);
+	mutex_unlock(&event->child_mutex);
 
-	mutex_lock(&ctx->mutex);
-	list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry) {
-		struct perf_event *parent_event = event->parent;
+	list_for_each_entry_safe(child, tmp, &child_list, child_list) {
+		struct perf_event_context *ctx;
 
-		if (!is_orphaned_child(event))
-			continue;
+		/*
+		 * This is somewhat similar to perf_free_event(),
+		 * except for these events are alive and need
+		 * proper perf_remove_from_context().
+		 */
+		ctx = perf_event_ctx_lock(child);
+		perf_remove_from_context(child, true);
+		perf_event_ctx_unlock(child, ctx);
 
-		perf_remove_from_context(event, true);
+		list_del(&child->child_list);
 
-		mutex_lock(&parent_event->child_mutex);
-		list_del_init(&event->child_list);
-		mutex_unlock(&parent_event->child_mutex);
+		/* Children will have exactly one reference */
+		free_event(child);
 
-		free_event(event);
-		put_event(parent_event);
+		/*
+		 * This matches the refcount bump in inherit_event();
+		 * this can't be the last reference.
+		 */
+		put_event(event);
 	}
 
-	raw_spin_lock_irq(&ctx->lock);
-	ctx->orphans_remove_sched = false;
-	raw_spin_unlock_irq(&ctx->lock);
-	mutex_unlock(&ctx->mutex);
+	/* Must be the last reference */
+	put_event(event);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(perf_event_release_kernel);
 
-	put_ctx(ctx);
+/*
+ * Called when the last reference to the file is gone.
+ */
+static int perf_release(struct inode *inode, struct file *file)
+{
+	perf_event_release_kernel(file->private_data);
+	return 0;
 }
 
 u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1310335 — Re: [PATCH] perf: Synchronously cleanup child events

On Fri, Jan 15, 2016 at 04:07:41PM +0200, Alexander Shishkin wrote:
>  int perf_event_release_kernel(struct perf_event *event)
>  {
> +	struct perf_event *child, *tmp;
> +	LIST_HEAD(child_list);
>  
> +	if (!is_kernel_event(event))
> +		perf_remove_from_owner(event);
>  
> +	event->owner = NULL;
>  
> +	/*
> +	 * event::child_mutex nests inside ctx::lock, so move children
> +	 * to a safe place first and avoid inversion
> +	 */
> +	mutex_lock(&event->child_mutex);
> +	list_splice_init(&event->child_list, &child_list);
> +	mutex_unlock(&event->child_mutex);

I suspect this races against inherit_event(), like:

	inherit_event()			perf_event_release_kernel()

	if (is_orphaned_event(parent_event) /* false */

					event->owner = NULL

	mutex_lock(child_mutex);
	list_splice
	mutex_unlock(child_mutex);

					mutex_lock(child_mutex);
					list_add_tail
					mutex_unlock(child_mutex);


Something like this would fix that I think, not sure its the best way
though...


--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -979,8 +979,8 @@ static void put_ctx(struct perf_event_co
  * Lock order:
  *	task_struct::perf_event_mutex
  *	  perf_event_context::mutex
- *	    perf_event_context::lock
  *	    perf_event::child_mutex;
+ *	      perf_event_context::lock
  *	    perf_event::mmap_mutex
  *	    mmap_sem
  */
@@ -8956,6 +8958,16 @@ inherit_event(struct perf_event *parent_
 	if (parent_event->parent)
 		parent_event = parent_event->parent;
 
+	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
+	/*
+	 * Serialize against perf_event_kernel_release()'s orphanage..
+	 */
+	mutex_lock(&parent_event->child_mutex);
+	if (is_orphaned_event(parent_event)) {
+		mutex_unlock(&parent_event->child_mutex);
+		return NULL;
+	}
+
 	child_event = perf_event_alloc(&parent_event->attr,
 					   parent_event->cpu,
 					   child,
@@ -8964,8 +8976,8 @@ inherit_event(struct perf_event *parent_
 	if (IS_ERR(child_event))
 		return child_event;
 
-	if (is_orphaned_event(parent_event) ||
-	    !atomic_long_inc_not_zero(&parent_event->refcount)) {
+	if (!atomic_long_inc_not_zero(&parent_event->refcount)) {
+		mutex_unlock(&parent_event->child_mutex);
 		free_event(child_event);
 		return NULL;
 	}
@@ -9013,8 +9025,6 @@ inherit_event(struct perf_event *parent_
 	/*
 	 * Link this into the parent event's child list
 	 */
-	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
-	mutex_lock(&parent_event->child_mutex);
 	list_add_tail(&child_event->child_list, &parent_event->child_list);
 	mutex_unlock(&parent_event->child_mutex);
 

[toc] | [prev] | [next] | [standalone]

#1311512 — Re: [PATCH] perf: Synchronously cleanup child events

Peter Zijlstra <peterz@infradead.org> writes:

> On Fri, Jan 15, 2016 at 04:07:41PM +0200, Alexander Shishkin wrote:
>>  int perf_event_release_kernel(struct perf_event *event)
>>  {
>> +	struct perf_event *child, *tmp;
>> +	LIST_HEAD(child_list);
>>  
>> +	if (!is_kernel_event(event))
>> +		perf_remove_from_owner(event);
>>  
>> +	event->owner = NULL;
>>  
>> +	/*
>> +	 * event::child_mutex nests inside ctx::lock, so move children
>> +	 * to a safe place first and avoid inversion
>> +	 */
>> +	mutex_lock(&event->child_mutex);
>> +	list_splice_init(&event->child_list, &child_list);
>> +	mutex_unlock(&event->child_mutex);
>
> I suspect this races against inherit_event(), like:
>
> 	inherit_event()			perf_event_release_kernel()
>
> 	if (is_orphaned_event(parent_event) /* false */
>
> 					event->owner = NULL
>
> 	mutex_lock(child_mutex);
> 	list_splice
> 	mutex_unlock(child_mutex);
>
> 					mutex_lock(child_mutex);
> 					list_add_tail
> 					mutex_unlock(child_mutex);

Indeed, this is possible.

>
> Something like this would fix that I think, not sure its the best way
> though...
>
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -979,8 +979,8 @@ static void put_ctx(struct perf_event_co
>   * Lock order:
>   *	task_struct::perf_event_mutex
>   *	  perf_event_context::mutex
> - *	    perf_event_context::lock
>   *	    perf_event::child_mutex;
> + *	      perf_event_context::lock
>   *	    perf_event::mmap_mutex
>   *	    mmap_sem
>   */

This is, actually, the order that we have already:

perf_ioctl():                ctx::mutex
-> perf_event_for_each():    event::child_mutex
   -> _perf_event_enable():  ctx::lock

that is, ctx::lock already nests inside event::child_mutex. So what
you're suggesting is an ok solution.

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1311518 — Re: [PATCH] perf: Synchronously cleanup child events

Peter Zijlstra <peterz@infradead.org> writes:

> I suspect this races against inherit_event(), like:
>
> 	inherit_event()			perf_event_release_kernel()
>
> 	if (is_orphaned_event(parent_event) /* false */
>
> 					event->owner = NULL
>
> 	mutex_lock(child_mutex);
> 	list_splice
> 	mutex_unlock(child_mutex);
>
> 					mutex_lock(child_mutex);
> 					list_add_tail
> 					mutex_unlock(child_mutex);

Or how about this instead:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8eb3fee429..cd9f1ac537 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3786,8 +3786,9 @@ int perf_event_release_kernel(struct perf_event *event)
 
 	event->owner = NULL;
 
+retry:
 	/*
-	 * event::child_mutex nests inside ctx::lock, so move children
+	 * event::child_mutex nests inside ctx::mutex, so move children
 	 * to a safe place first and avoid inversion
 	 */
 	mutex_lock(&event->child_mutex);
@@ -3818,8 +3819,13 @@ int perf_event_release_kernel(struct perf_event *event)
 		put_event(event);
 	}
 
-	/* Must be the last reference */
+	/* Must be the last reference, .. */
 	put_event(event);
+
+	/* .. unless we raced with inherit_event(), in which case, repeat */
+	if (atomic_long_inc_not_zero(&event->refcount))
+		goto retry;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(perf_event_release_kernel);

[toc] | [prev] | [next] | [standalone]

#1311591 — Re: [PATCH] perf: Synchronously cleanup child events

On Mon, Jan 18, 2016 at 02:37:06PM +0200, Alexander Shishkin wrote:

> Or how about this instead:

In principle better since it doesn't increase the lock hold times etc.,
but buggy I think:

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 8eb3fee429..cd9f1ac537 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3786,8 +3786,9 @@ int perf_event_release_kernel(struct perf_event *event)
>  
>  	event->owner = NULL;
>  
> +retry:
>  	/*
> -	 * event::child_mutex nests inside ctx::lock, so move children
> +	 * event::child_mutex nests inside ctx::mutex, so move children
>  	 * to a safe place first and avoid inversion
>  	 */
>  	mutex_lock(&event->child_mutex);
> @@ -3818,8 +3819,13 @@ int perf_event_release_kernel(struct perf_event *event)
>  		put_event(event);
>  	}
>  
> -	/* Must be the last reference */
> +	/* Must be the last reference, .. */
>  	put_event(event);
> +
> +	/* .. unless we raced with inherit_event(), in which case, repeat */

Nothing prevents @event from being freed here, which would make:

> +	if (atomic_long_inc_not_zero(&event->refcount))

a use-after-free.

You'll have to do something like:

static bool put_event_last(struct perf_event *event, long value)
{
	if (atomic_long_cmpxchg(&event->refcount, 1, 0)) {
		__put_event(event); /* normal put_event() body */
		return true;
	}
	return false;
}

with which you can do:

	if (!put_event_last(event))
		goto retry;

[toc] | [prev] | [next] | [standalone]

#1312322 — [PATCH v2] perf: Synchronously cleanup child events

The orphan cleanup workqueue doesn't always catch orphans, for example,
if they never schedule after they are orphaned. IOW, the event leak is
still very real. It also wouldn't work for kernel counters.

Also, there seems to be no reason not to carry out this cleanup
procedure synchronously during parent event's destruction.

This patch replaces the workqueue approach with a simple cleanup round
in the event's destruction path. To avoid racing with clone, we still
check that parent event has an owner in the inheritance path.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h |   3 -
 kernel/events/core.c       | 142 ++++++++++++++++++++-------------------------
 2 files changed, 63 insertions(+), 82 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 6612732d8f..cd9c1ace29 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -634,9 +634,6 @@ struct perf_event_context {
 	int				nr_cgroups;	 /* cgroup evts */
 	void				*task_ctx_data; /* pmu specific data */
 	struct rcu_head			rcu_head;
-
-	struct delayed_work		orphans_remove;
-	bool				orphans_remove_sched;
 };
 
 /*
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 630f53acce..33083ed5a6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,8 +49,6 @@
 
 #include <asm/irq_regs.h>
 
-static struct workqueue_struct *perf_wq;
-
 typedef int (*remote_function_f)(void *);
 
 struct remote_function_call {
@@ -1652,40 +1650,9 @@ out:
  */
 static bool is_orphaned_event(struct perf_event *event)
 {
-	return event && !is_kernel_event(event) && !event->owner;
-}
-
-/*
- * Event has a parent but parent's task finished and it's
- * alive only because of children holding refference.
- */
-static bool is_orphaned_child(struct perf_event *event)
-{
-	return is_orphaned_event(event->parent);
+	return event && !event->owner;
 }
 
-static void orphans_remove_work(struct work_struct *work);
-
-static void schedule_orphans_remove(struct perf_event_context *ctx)
-{
-	if (!ctx->task || ctx->orphans_remove_sched || !perf_wq)
-		return;
-
-	if (queue_delayed_work(perf_wq, &ctx->orphans_remove, 1)) {
-		get_ctx(ctx);
-		ctx->orphans_remove_sched = true;
-	}
-}
-
-static int __init perf_workqueue_init(void)
-{
-	perf_wq = create_singlethread_workqueue("perf");
-	WARN(!perf_wq, "failed to create perf workqueue\n");
-	return perf_wq ? 0 : -1;
-}
-
-core_initcall(perf_workqueue_init);
-
 static inline int pmu_filter_match(struct perf_event *event)
 {
 	struct pmu *pmu = event->pmu;
@@ -1746,9 +1713,6 @@ event_sched_out(struct perf_event *event,
 	if (event->attr.exclusive || !cpuctx->active_oncpu)
 		cpuctx->exclusive = 0;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 	perf_pmu_enable(event->pmu);
 }
 
@@ -1991,9 +1955,6 @@ event_sched_in(struct perf_event *event,
 	if (event->attr.exclusive)
 		cpuctx->exclusive = 1;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 out:
 	perf_pmu_enable(event->pmu);
 
@@ -3370,7 +3331,6 @@ static void __perf_event_init_context(struct perf_event_context *ctx)
 	INIT_LIST_HEAD(&ctx->flexible_groups);
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
-	INIT_DELAYED_WORK(&ctx->orphans_remove, orphans_remove_work);
 }
 
 static struct perf_event_context *
@@ -3786,13 +3746,10 @@ static void perf_remove_from_owner(struct perf_event *event)
 	}
 }
 
-static void put_event(struct perf_event *event)
+static void __put_event(struct perf_event *event)
 {
 	struct perf_event_context *ctx;
 
-	if (!atomic_long_dec_and_test(&event->refcount))
-		return;
-
 	if (!is_kernel_event(event))
 		perf_remove_from_owner(event);
 
@@ -3816,56 +3773,83 @@ static void put_event(struct perf_event *event)
 	_free_event(event);
 }
 
-int perf_event_release_kernel(struct perf_event *event)
+static void put_event(struct perf_event *event)
 {
-	put_event(event);
-	return 0;
+	if (atomic_long_dec_and_test(&event->refcount))
+		__put_event(event);
 }
-EXPORT_SYMBOL_GPL(perf_event_release_kernel);
 
-/*
- * Called when the last reference to the file is gone.
- */
-static int perf_release(struct inode *inode, struct file *file)
+static bool put_event_last(struct perf_event *event)
 {
-	put_event(file->private_data);
-	return 0;
+	if (atomic_long_cmpxchg(&event->refcount, 1, 0)) {
+		__put_event(event);
+		return true;
+	}
+
+	return false;
 }
 
-/*
- * Remove all orphanes events from the context.
- */
-static void orphans_remove_work(struct work_struct *work)
+int perf_event_release_kernel(struct perf_event *event)
 {
-	struct perf_event_context *ctx;
-	struct perf_event *event, *tmp;
+	struct perf_event *child, *tmp;
+	LIST_HEAD(child_list);
 
-	ctx = container_of(work, struct perf_event_context,
-			   orphans_remove.work);
+	if (!is_kernel_event(event))
+		perf_remove_from_owner(event);
 
-	mutex_lock(&ctx->mutex);
-	list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry) {
-		struct perf_event *parent_event = event->parent;
+	event->owner = NULL;
 
-		if (!is_orphaned_child(event))
-			continue;
+retry:
+	/*
+	 * event::child_mutex nests inside ctx::mutex, so move children
+	 * to a safe place first and avoid inversion
+	 */
+	mutex_lock(&event->child_mutex);
+	list_splice_init(&event->child_list, &child_list);
+	mutex_unlock(&event->child_mutex);
 
-		perf_remove_from_context(event, true);
+	list_for_each_entry_safe(child, tmp, &child_list, child_list) {
+		struct perf_event_context *ctx;
 
-		mutex_lock(&parent_event->child_mutex);
-		list_del_init(&event->child_list);
-		mutex_unlock(&parent_event->child_mutex);
+		/*
+		 * This is somewhat similar to perf_free_event(),
+		 * except for these events are alive and need
+		 * proper perf_remove_from_context().
+		 */
+		ctx = perf_event_ctx_lock(child);
+		perf_remove_from_context(child, true);
+		perf_event_ctx_unlock(child, ctx);
+
+		list_del(&child->child_list);
 
-		free_event(event);
-		put_event(parent_event);
+		/* Children will have exactly one reference */
+		free_event(child);
+
+		/*
+		 * This matches the refcount bump in inherit_event();
+		 * this can't be the last reference.
+		 */
+		put_event(event);
 	}
 
-	raw_spin_lock_irq(&ctx->lock);
-	ctx->orphans_remove_sched = false;
-	raw_spin_unlock_irq(&ctx->lock);
-	mutex_unlock(&ctx->mutex);
+	/*
+	 * If this is the last reference, we're done here, otherwise
+	 * we must have raced with inherit_event(), in which case, repeat
+	 */
+	if (!put_event_last(event))
+		goto retry;
 
-	put_ctx(ctx);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(perf_event_release_kernel);
+
+/*
+ * Called when the last reference to the file is gone.
+ */
+static int perf_release(struct inode *inode, struct file *file)
+{
+	perf_event_release_kernel(file->private_data);
+	return 0;
 }
 
 u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
-- 
2.7.0.rc3

[toc] | [prev] | [next] | [standalone]

#1312479 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 19, 2016 at 05:12:34PM +0200, Alexander Shishkin wrote:
> +static bool put_event_last(struct perf_event *event)
>  {
> +	if (atomic_long_cmpxchg(&event->refcount, 1, 0)) {

	if (atomic_long_cmpxchg(&event->refcount, 1, 0) == 1) {

> +		__put_event(event);
> +		return true;
> +	}
> +
> +	return false;
>  }

[toc] | [prev] | [next] | [standalone]

#1312480 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 19, 2016 at 05:12:34PM +0200, Alexander Shishkin wrote:

> +static void __put_event(struct perf_event *event)
>  {
>  	struct perf_event_context *ctx;
>  
>  	if (!is_kernel_event(event))
>  		perf_remove_from_owner(event);
>  

> +int perf_event_release_kernel(struct perf_event *event)
>  {
> +	struct perf_event *child, *tmp;
> +	LIST_HEAD(child_list);
>  
> +	if (!is_kernel_event(event))
> +		perf_remove_from_owner(event);
>  
> +	event->owner = NULL;
>  
> +retry:

	<snip>

> +	/*
> +	 * If this is the last reference, we're done here, otherwise
> +	 * we must have raced with inherit_event(), in which case, repeat
> +	 */
> +	if (!put_event_last(event))
> +		goto retry;
>  
> +	return 0;
> +}

So I think there's a number of problems still :-(

I all starts with having two perf_remove_from_owner() calls (as I
mentioned on IRC), this doesn't make sense.

I think the moment you close the file and userspace looses control over
it, we should drop the owner bit, which is exactly the one
remove_from_owner in perf_release().

If, for some magical reason, the event lives on after that (and we'll
get to that), it should live on owner-less.

Now, assume someone has such a magical reference, then our
put_event_last() goto again loop will never terminate, this seems like a
bad thing.

The most obvious place that generates such magical references would be
the bpf arraymap doing perf_event_get() on things. There are a few other
places that take temp references (perf_mmap_close), but those are
'short' lived and while ugly will not cause massive grief. The BPF one
OTOH is a real problem here.

And looking at the BPF stuff, that code seems to assume
perf_event_kernel_release() := put_event(), so this patch breaks that
too.


Alexei, is there a reason the arraymap stuff needs a perf event ref as
opposed to a file ref? I'm forever a little confused on how perf<->bpf
works.

[toc] | [prev] | [next] | [standalone]

#1312538 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 19, 2016 at 09:05:58PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 05:12:34PM +0200, Alexander Shishkin wrote:
> 
> > +static void __put_event(struct perf_event *event)
> >  {
> >  	struct perf_event_context *ctx;
> >  
> >  	if (!is_kernel_event(event))
> >  		perf_remove_from_owner(event);
> >  
> 
> > +int perf_event_release_kernel(struct perf_event *event)
> >  {
> > +	struct perf_event *child, *tmp;
> > +	LIST_HEAD(child_list);
> >  
> > +	if (!is_kernel_event(event))
> > +		perf_remove_from_owner(event);
> >  
> > +	event->owner = NULL;
> >  
> > +retry:
> 
> 	<snip>
> 
> > +	/*
> > +	 * If this is the last reference, we're done here, otherwise
> > +	 * we must have raced with inherit_event(), in which case, repeat
> > +	 */
> > +	if (!put_event_last(event))
> > +		goto retry;
> >  
> > +	return 0;
> > +}
> 
> So I think there's a number of problems still :-(
> 
> I all starts with having two perf_remove_from_owner() calls (as I
> mentioned on IRC), this doesn't make sense.
> 
> I think the moment you close the file and userspace looses control over
> it, we should drop the owner bit, which is exactly the one
> remove_from_owner in perf_release().
> 
> If, for some magical reason, the event lives on after that (and we'll
> get to that), it should live on owner-less.
> 
> Now, assume someone has such a magical reference, then our
> put_event_last() goto again loop will never terminate, this seems like a
> bad thing.
> 
> The most obvious place that generates such magical references would be
> the bpf arraymap doing perf_event_get() on things. There are a few other
> places that take temp references (perf_mmap_close), but those are
> 'short' lived and while ugly will not cause massive grief. The BPF one
> OTOH is a real problem here.
> 
> And looking at the BPF stuff, that code seems to assume
> perf_event_kernel_release() := put_event(), so this patch breaks that
> too.
> 
> 
> Alexei, is there a reason the arraymap stuff needs a perf event ref as
> opposed to a file ref? I'm forever a little confused on how perf<->bpf
> works.

A file ref will not work, since user space could have closed that
perf_event file to avoid unnecessary FDs.
Program only need the stable pointer to 'struct perf_event' which
it will use while running.
At the end it will call perf_event_kernel_release() which
is == put_event().
It was the case that 'perf_events' were normal refcnt-ed structures
and the last guy frees it.
This put_event_last() logic definitely looks problematic.
There are no ordering guarantees.
User space may close FD, while struct perf_event is still alive.
The loop around perf_event_last() looks buggy.
I'm obviously missing the main goal of this patch.

[toc] | [prev] | [next] | [standalone]

#1312994 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 19, 2016 at 01:58:19PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 19, 2016 at 09:05:58PM +0100, Peter Zijlstra wrote:

> > The most obvious place that generates such magical references would be
> > the bpf arraymap doing perf_event_get() on things. There are a few other
> > places that take temp references (perf_mmap_close), but those are
> > 'short' lived and while ugly will not cause massive grief. The BPF one
> > OTOH is a real problem here.
> > 
> > And looking at the BPF stuff, that code seems to assume
> > perf_event_kernel_release() := put_event(), so this patch breaks that
> > too.
> > 
> > 
> > Alexei, is there a reason the arraymap stuff needs a perf event ref as
> > opposed to a file ref? I'm forever a little confused on how perf<->bpf
> > works.
> 
> A file ref will not work, since user space could have closed that
> perf_event file to avoid unnecessary FDs.

So I'm (possibly again) confused on how BPF works.

I thought the reason you handed in perf events from userspace; as
opposed to creating your own with perf_event_create_kernel_counter();
was because userspace was interested in the output.

Also, BPF should not be a way to get around the filedesc resource limit.

> Program only need the stable pointer to 'struct perf_event' which
> it will use while running.
> At the end it will call perf_event_kernel_release() which
> is == put_event().
> It was the case that 'perf_events' were normal refcnt-ed structures
> and the last guy frees it.

Sort-of, but user events are (or should be, rather) tied to the filedesc
to account the resources used.

There is also the event->owner field, we track the task that created the
event, with your current scheme that is left dangling once userspace
closes the last filedesc and you still have a ref open.

> This put_event_last() logic definitely looks problematic.
> There are no ordering guarantees.
> User space may close FD, while struct perf_event is still alive.
> The loop around perf_event_last() looks buggy.
> I'm obviously missing the main goal of this patch.

Right, so the patch in question tries to synchronously clean up
everything related to the counter when we close the file. Such that the
file better reflects the actual resource usage.

Currently we do this async (and with holes).


In short, user created event really should be filedesc based, yes we
have event references, but those 'should' be short lived.

[toc] | [prev] | [next] | [standalone]

#1313860 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Wed, Jan 20, 2016 at 09:32:22AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 01:58:19PM -0800, Alexei Starovoitov wrote:
> > On Tue, Jan 19, 2016 at 09:05:58PM +0100, Peter Zijlstra wrote:
> 
> > > The most obvious place that generates such magical references would be
> > > the bpf arraymap doing perf_event_get() on things. There are a few other
> > > places that take temp references (perf_mmap_close), but those are
> > > 'short' lived and while ugly will not cause massive grief. The BPF one
> > > OTOH is a real problem here.
> > > 
> > > And looking at the BPF stuff, that code seems to assume
> > > perf_event_kernel_release() := put_event(), so this patch breaks that
> > > too.
> > > 
> > > 
> > > Alexei, is there a reason the arraymap stuff needs a perf event ref as
> > > opposed to a file ref? I'm forever a little confused on how perf<->bpf
> > > works.
> > 
> > A file ref will not work, since user space could have closed that
> > perf_event file to avoid unnecessary FDs.
> 
> So I'm (possibly again) confused on how BPF works.
> 
> I thought the reason you handed in perf events from userspace; as
> opposed to creating your own with perf_event_create_kernel_counter();
> was because userspace was interested in the output.

yes. There are two use cases of perf_events from bpf:
1. sw_bpf_output event is used by bpf to push samples into it and
   user spaces reads it as normal via mmap
2. PERF_TYPE_HARDWARE event is used by bpf program to read
   counters to measure things like number of cycles or tlb misses
   in a given function.
   In this case user space typically leaves FDs around, but it doesn't
   use them for anything.

> Also, BPF should not be a way to get around the filedesc resource limit.

all bpf tracing stuff is root only and maps are charged for every element.

> > Program only need the stable pointer to 'struct perf_event' which
> > it will use while running.
> > At the end it will call perf_event_kernel_release() which
> > is == put_event().
> > It was the case that 'perf_events' were normal refcnt-ed structures
> > and the last guy frees it.
> 
> Sort-of, but user events are (or should be, rather) tied to the filedesc
> to account the resources used.
> 
> There is also the event->owner field, we track the task that created the
> event, with your current scheme that is left dangling once userspace
> closes the last filedesc and you still have a ref open.
> 
> > This put_event_last() logic definitely looks problematic.
> > There are no ordering guarantees.
> > User space may close FD, while struct perf_event is still alive.
> > The loop around perf_event_last() looks buggy.
> > I'm obviously missing the main goal of this patch.
> 
> Right, so the patch in question tries to synchronously clean up
> everything related to the counter when we close the file. Such that the
> file better reflects the actual resource usage.
> 
> Currently we do this async (and with holes).
> 
> In short, user created event really should be filedesc based, yes we
> have event references, but those 'should' be short lived.

I'm still missing why it's the problem.
Which counter do you want to bump as part of perf_event_get() ?
still event->refcount, right?
but the same perf_event can be stored in multiple bpf maps
and many bpf programs can be using it, while nothing can
possibly prevent the user space to do close(perf_event_fd)
while programs are still running and collecting tlb miss data
from the counters.
So what do you propose?

[toc] | [prev] | [next] | [standalone]

#1312946 — Re: [PATCH v2] perf: Synchronously cleanup child events

Peter Zijlstra <peterz@infradead.org> writes:

> So I think there's a number of problems still :-(
>
> I all starts with having two perf_remove_from_owner() calls (as I
> mentioned on IRC), this doesn't make sense.
>
> I think the moment you close the file and userspace looses control over
> it, we should drop the owner bit, which is exactly the one
> remove_from_owner in perf_release().

Fair enough.

> If, for some magical reason, the event lives on after that (and we'll
> get to that), it should live on owner-less.
>
> Now, assume someone has such a magical reference, then our
> put_event_last() goto again loop will never terminate, this seems like a
> bad thing.
>
> The most obvious place that generates such magical references would be
> the bpf arraymap doing perf_event_get() on things. There are a few other
> places that take temp references (perf_mmap_close), but those are
> 'short' lived and while ugly will not cause massive grief.

We won't get to perf_release() before we're done with perf_mmap_close(),
so that one's not really a problem.

> The BPF one OTOH is a real problem here.
>
> And looking at the BPF stuff, that code seems to assume
> perf_event_kernel_release() := put_event(), so this patch breaks that
> too.

Yes, that one's very much an api abuse, it should clearly be using
get_file()/fput() instead. Now that the code is there already, there's a
slight chance that changing this will have userspace running into the fd
limit and cause a regression. As a workaround we can probably introduce
yet another magial owner to allow a userspace event to be
'stolen'. Since bpf is the only user of perf_event_get(), this can be
somewhat easily arranged.

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1312977 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Wed, Jan 20, 2016 at 09:04:28AM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:

> > The most obvious place that generates such magical references would be
> > the bpf arraymap doing perf_event_get() on things. There are a few other
> > places that take temp references (perf_mmap_close), but those are
> > 'short' lived and while ugly will not cause massive grief.
> 
> We won't get to perf_release() before we're done with perf_mmap_close(),
> so that one's not really a problem.

Only for the file we mmap()'ed, the events we've attached through
IOC_SET_OUTPUT will not have a file reference from the mmap().

[toc] | [prev] | [next] | [standalone]

#1314921 — Re: [PATCH v2] perf: Synchronously cleanup child events

Peter Zijlstra <peterz@infradead.org> writes:

> So I think there's a number of problems still :-(

Also, it does indeed race with
__perf_event_exit_task()/sync_child_event(), but that one I'd fix by
simply wrapping the sync_child_event()/free_event() in

mutex_lock(&parent_event->child_mutex);
if (!is_orphan_event(parent_event)) {
   sync_child_event(child_event);
   free_event(child_event);
}
mutex_unlock(&parent_event->child_event);

At some later point in time the code there could use a bit of
reshuffling, I guess.

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1314936 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > So I think there's a number of problems still :-(
> 
> Also, it does indeed race with
> __perf_event_exit_task()/sync_child_event(), but that one I'd fix by
> simply wrapping the sync_child_event()/free_event() in
> 
> mutex_lock(&parent_event->child_mutex);
> if (!is_orphan_event(parent_event)) {
>    sync_child_event(child_event);
>    free_event(child_event);
> }
> mutex_unlock(&parent_event->child_event);

So I've been staring at exactly that code for a while because Ingo
managed to trigger that race (and I could reproduce with his
'workload').

But I'm not seeing how; both sites hold ctx->mutex and remove the event
from the ctx->event_list.

So the way I'm seeing it, either the orphan_work find and frees it, or
the __perf_event_exit_task() one does, but I'm a bit stumped on how they
can both do.

Sure, the sync stuff is pointless if we're orphan, but I don't see how
it can harm.

> At some later point in time the code there could use a bit of
> reshuffling, I guess.

Yes.

[toc] | [prev] | [next] | [standalone]

#1314951 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > So I think there's a number of problems still :-(
> 
> Also, it does indeed race with
> __perf_event_exit_task()/sync_child_event(), but that one I'd fix by
> simply wrapping the sync_child_event()/free_event() in
> 
> mutex_lock(&parent_event->child_mutex);
> if (!is_orphan_event(parent_event)) {
>    sync_child_event(child_event);
>    free_event(child_event);
> }
> mutex_unlock(&parent_event->child_event);

Also, note the comment with _perf_event_disable(), that relies on
sync_child_event() taking event->parent->child_mutex.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web