perf: recursive locking of ctx->mutex

perf: recursive locking of ctx->mutex

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel I've stumbled on the following spew:

[  460.391146] =============================================
[  460.391146] [ INFO: possible recursive locking detected ]
[  460.391146] 4.1.0-rc1-next-20150429-sasha-00037-g3e011d3-dirty #2192 Not tainted
[  460.391146] ---------------------------------------------
[  460.391146] trinity-main/9652 is trying to acquire lock:
[  460.391146] (&ctx->mutex){+.+.+.}, at: perf_event_ctx_lock_nested (kernel/events/core.c:965)
[  460.391146] Mutex: counter: 1 owner: None
[  460.391146]
[  460.391146] but task is already holding lock:
[  460.391146] (&ctx->mutex){+.+.+.}, at: perf_event_init_task (kernel/events/core.c:8732 kernel/events/core.c:8800)
[  460.391146] Mutex: counter: 0 owner: trinity-main
[  460.391146]
[  460.391146] other info that might help us debug this:
[  460.391146]  Possible unsafe locking scenario:
[  460.391146]
[  460.391146]        CPU0
[  460.391146]        ----
[  460.391146]   lock(&ctx->mutex);
[  460.391146]   lock(&ctx->mutex);
[  460.391146]
[  460.391146]  *** DEADLOCK ***
[  460.391146]
[  460.391146]  May be due to missing lock nesting notation
[  460.391146]
[  460.391146] 2 locks held by trinity-main/9652:
[  460.391146] #0: (&ctx->mutex){+.+.+.}, at: perf_event_init_task (kernel/events/core.c:8732 kernel/events/core.c:8800)
[  460.438470] Mutex: counter: 0 owner: trinity-main
[  460.438470] #1: (&pmus_srcu){......}, at: perf_init_event (kernel/events/core.c:7385)
[  460.438470]
[  460.438470] stack backtrace:
[  460.438470] CPU: 3 PID: 9652 Comm: trinity-main Not tainted 4.1.0-rc1-next-20150429-sasha-00037-g3e011d3-dirty #2192
[  460.438470]  ffffffffaef7d2c0 00000000f948113a ffff8802f4e4f528 ffffffffa8dddba3
[  460.438470]  0000000000000000 ffffffffaef7d2c0 ffff8802f4e4f6c8 ffffffff9f302223
[  460.438470]  ffff880a70f240c8 ffff880a0000bb5a ffffed0000000547 ffff8802f61d0dc8
[  460.438470] Call Trace:
[  460.438470] dump_stack (lib/dump_stack.c:52)
[  460.438470] __lock_acquire (kernel/locking/lockdep.c:1776 kernel/locking/lockdep.c:1820 kernel/locking/lockdep.c:2152 kernel/locking/lockdep.c:3238)
[  460.438470] ? lockdep_init_map (kernel/locking/lockdep.c:3105)
[  460.438470] ? sched_clock_cpu (kernel/sched/clock.c:311)
[  460.438470] ? __lock_is_held (kernel/locking/lockdep.c:3572)
[  460.438470] lock_acquire (kernel/locking/lockdep.c:3658)
[  460.438470] ? perf_event_ctx_lock_nested (kernel/events/core.c:965)
[  460.438470] mutex_lock_nested (kernel/locking/mutex.c:526 kernel/locking/mutex.c:617)
[  460.438470] ? perf_event_ctx_lock_nested (kernel/events/core.c:965)
[  460.438470] ? get_parent_ip (kernel/sched/core.c:2556)
[  460.438470] ? get_lock_stats (kernel/locking/lockdep.c:249)
[  460.438470] ? perf_event_ctx_lock_nested (kernel/events/core.c:965)
[  460.438470] ? _mutex_lock_nest_lock (kernel/locking/mutex.c:615)
[  460.438470] ? perf_event_ctx_lock_nested (include/linux/rcupdate.h:969 kernel/events/core.c:962)
[  460.438470] perf_event_ctx_lock_nested (kernel/events/core.c:965)
[  460.438470] ? perf_event_ctx_lock_nested (include/linux/rcupdate.h:912 kernel/events/core.c:956)
[  460.438470] ? perf_init_event (include/linux/rcupdate.h:969 kernel/events/core.c:7394)
[  460.438470] perf_try_init_event (kernel/events/core.c:977 kernel/events/core.c:7368)
[  460.438470] perf_init_event (kernel/events/core.c:7404)
[  460.438470] ? perf_bp_event (kernel/events/core.c:7385)
[  460.438470] perf_event_alloc (kernel/events/core.c:7564)
[  460.438470] inherit_event.isra.57 (kernel/events/core.c:8564)
[  460.438470] inherit_task_group.isra.59 (kernel/events/core.c:8646 kernel/events/core.c:8682)
[  460.438470] perf_event_init_task (kernel/events/core.c:8735 kernel/events/core.c:8800)
[  460.438470] copy_process (kernel/fork.c:1418)
[  460.438470] do_fork (kernel/fork.c:1705)
[  460.438470] SyS_clone (kernel/fork.c:1794)
[  460.438470] system_call_fastpath (arch/x86/kernel/entry_64.S:261)


Thanks,
Sasha

Re: perf: recursive locking of ctx->mutex

[Peter Zijlstra]

A little something like so should cure that me thinks.

I would much appreciate other people reviewing this with care though; my
snot addled brain isn't too bright.


---
Subject: perf: Annotate inherited event ctx->mutex recursion
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri May  1 16:08:46 CEST 2015

While fuzzing Sasha tripped over another ctx->mutex recursion lockdep
splat. Annotate this.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -914,10 +914,30 @@ static void put_ctx(struct perf_event_co
  * Those places that change perf_event::ctx will hold both
  * perf_event_ctx::mutex of the 'old' and 'new' ctx value.
  *
- * Lock ordering is by mutex address. There is one other site where
- * perf_event_context::mutex nests and that is put_event(). But remember that
- * that is a parent<->child context relation, and migration does not affect
- * children, therefore these two orderings should not interact.
+ * Lock ordering is by mutex address. There are two other sites where
+ * perf_event_context::mutex nests and those are:
+ *
+ *  - perf_event_exit_task_context()	[ child , 0 ]
+ *      __perf_event_exit_task()
+ *        sync_child_event()
+ *          put_event()			[ parent, 1 ]
+ *
+ *  - perf_event_init_context()		[ parent, 0 ]
+ *      inherit_task_group()
+ *        inherit_group()
+ *          inherit_event()
+ *            perf_event_alloc()
+ *              perf_init_event()
+ *                perf_try_init_event()	[ child , 1 ]
+ *
+ * While it appears there is an obvious deadlock here -- the parent and child
+ * nesting levels are inverted between the two. This is in fact safe because
+ * life-time rules separate them. That is an exiting task cannot fork, and a
+ * spawning task cannot (yet) exit.
+ *
+ * But remember that that these are parent<->child context relations, and
+ * migration does not affect children, therefore these two orderings should not
+ * interact.
  *
  * The change in perf_event::ctx does not affect children (as claimed above)
  * because the sys_perf_event_open() case will install a new event and break
@@ -3658,9 +3678,6 @@ static void perf_remove_from_owner(struc
 	}
 }
 
-/*
- * Called when the last reference to the file is gone.
- */
 static void put_event(struct perf_event *event)
 {
 	struct perf_event_context *ctx;
@@ -3698,6 +3715,9 @@ int perf_event_release_kernel(struct per
 }
 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)
 {
 	put_event(file->private_data);
@@ -7370,7 +7390,12 @@ static int perf_try_init_event(struct pm
 		return -ENODEV;
 
 	if (event->group_leader != event) {
-		ctx = perf_event_ctx_lock(event->group_leader);
+		/*
+		 * This ctx->mutex can nest when we're called through
+		 * inheritance. See the perf_event_ctx_lock_nested() comment.
+		 */
+		ctx = perf_event_ctx_lock_nested(event->group_leader,
+						 SINGLE_DEPTH_NESTING);
 		BUG_ON(!ctx);
 	}
 
@@ -8728,7 +8753,7 @@ static int perf_event_init_context(struc
 	 * Lock the parent list. No need to lock the child - not PID
 	 * hashed yet and not running, so nobody can access it.
 	 */
-	mutex_lock(&parent_ctx->mutex);
+	mutex_lock_nested(&parent_ctx->mutex);
 
 	/*
 	 * We dont have to disable NMIs - we are only looking at

Re: perf: recursive locking of ctx->mutex

[Peter Zijlstra]

On Fri, May 01, 2015 at 04:13:37PM +0200, Peter Zijlstra wrote:
> 
> A little something like so should cure that me thinks.
> 
> I would much appreciate other people reviewing this with care though; my
> snot addled brain isn't too bright.
> 
> 
> @@ -8728,7 +8753,7 @@ static int perf_event_init_context(struc
>  	 * Lock the parent list. No need to lock the child - not PID
>  	 * hashed yet and not running, so nobody can access it.
>  	 */
> -	mutex_lock(&parent_ctx->mutex);
> +	mutex_lock_nested(&parent_ctx->mutex);
>  
>  	/*
>  	 * We dont have to disable NMIs - we are only looking at

It will help with building to not apply this last hunk..