* perf: recursive locking of ctx->mutex
@ 2015-04-30 15:12 Sasha Levin
2015-05-01 14:13 ` Peter Zijlstra
0 siblings, 1 reply; 3+ messages in thread
From: Sasha Levin @ 2015-04-30 15:12 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney, Ingo Molnar, Steven Rostedt, acme
Cc: LKML, Dave Jones
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: perf: recursive locking of ctx->mutex
2015-04-30 15:12 perf: recursive locking of ctx->mutex Sasha Levin
@ 2015-05-01 14:13 ` Peter Zijlstra
2015-05-01 14:15 ` Peter Zijlstra
0 siblings, 1 reply; 3+ messages in thread
From: Peter Zijlstra @ 2015-05-01 14:13 UTC (permalink / raw)
To: Sasha Levin
Cc: Paul E. McKenney, Ingo Molnar, Steven Rostedt, acme, LKML,
Dave Jones, jolsa, vincent.weaver
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: perf: recursive locking of ctx->mutex
2015-05-01 14:13 ` Peter Zijlstra
@ 2015-05-01 14:15 ` Peter Zijlstra
0 siblings, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2015-05-01 14:15 UTC (permalink / raw)
To: Sasha Levin
Cc: Paul E. McKenney, Ingo Molnar, Steven Rostedt, acme, LKML,
Dave Jones, jolsa, vincent.weaver
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..
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-05-01 14:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-30 15:12 perf: recursive locking of ctx->mutex Sasha Levin
2015-05-01 14:13 ` Peter Zijlstra
2015-05-01 14:15 ` Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.