Re: [PATCH] perf: Synchronously cleanup child events

[Peter Zijlstra]

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);