[perf] 3215929f8f: n f[d 29 [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86

[perf] 3215929f8f: n f[d 29 [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

FYI, we noticed the below changes on

git://internal_merge_and_test_tree devel-catchup-201509131855
commit 3215929f8ff55da910c26971d4286abff53ea6e2 ("perf: Restructure perf syscall point of no return")


 [m ai2n]1 S.089628] BUG: unable to handle kernel NULL pointer dereference at 00000198
et[so ck op t(21011 .1 090827] IP: [<c2b2e592>] SyS_perf_event_open+0x757/0xb86
] Setso[ck op t( 21.092021] CPU: 0 PID: 668 Comm: trinity-main Not tainted 4.2.0-02761-g3215929 #2
[   21.092021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
1 [7  8b 60 0020 14). o092021] task: cfc523c0 ti: d039c000 task.ti: d039c000
[   21.092021] EIP: 0060:[<c2b2e592>] EFLAGS: 00010246 CPU: 0
n f[d  29  [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86
[   21.092021] EAX: c38e33c4 EBX: 00000000 ECX: 00000000 EDX: 00000000
[   21.092021] ESI: cfc523c0 EDI: 00000000 EBP: d039dfac ESP: d039df00
[ ma in ] 2Se1ts.oc092021]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[   21.092021] CR0: 8005003b CR2: 00000198 CR3: 109cb140 CR4: 000006b0
kop[t( 1  21  82b6100.0092021] Stack:
[0ma0in0] 0Se0ts0oc0k0 0000001do pt0(10070 80 80b6000000
d 34 [2[6: 2: 39 21.092021] Call Trace:
 [m ai n]2 S1et.so092021]  [<c33451c1>] syscall_call+0x7/0x7
[   21.092021]  [<c3340000>] ? preempt_schedule_irq+0x4d/0xea
sockop[t( 10 c  21.092021] EIP: [<c2b2e592>] SyS_perf_event_open+0x757/0xb86 SS:ESP 0068:d039df00
9 [8b 60 00 0 24)1 o.n 092021] CR2: 0000000000000198
fd 44 [26:1:232]
[   21.114479] ---[ end trace c29b51e2a14544eb ]---
[ ma in ] 2Se1ts.oc115072] Kernel panic - not syncing: Fatal exception


Thanks,
Ying Huang

[-- Attachment #2: dmesg.xz --]
[-- Type: application/x-xz, Size: 15408 bytes --]

Re: [perf] 3215929f8f: n f[d 29 [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Thu, Sep 17, 2015 at 01:28:18PM +0800, kernel test robot wrote:
> FYI, we noticed the below changes on
> 
> git://internal_merge_and_test_tree devel-catchup-201509131855
> commit 3215929f8ff55da910c26971d4286abff53ea6e2 ("perf: Restructure perf syscall point of no return")

How can I get the actual patch? I don't have this commit. (It could be a
previous version, I had a bug like that, but not having the actual patch
I cannot check if its the same or not).

Re: [perf] 3215929f8f: n f[d 29 [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86

[Fengguang Wu]

[-- Attachment #1: Type: text/plain, Size: 4061 bytes --]

Hi Peter,

On Thu, Sep 17, 2015 at 10:53:36AM +0200, Peter Zijlstra wrote:
> On Thu, Sep 17, 2015 at 01:28:18PM +0800, kernel test robot wrote:
> > FYI, we noticed the below changes on
> > 
> > git://internal_merge_and_test_tree devel-catchup-201509131855
> > commit 3215929f8ff55da910c26971d4286abff53ea6e2 ("perf: Restructure perf syscall point of no return")
> 
> How can I get the actual patch? I don't have this commit. (It could be a
> previous version, I had a bug like that, but not having the actual patch
> I cannot check if its the same or not).

Here is the patch:

>From 3215929f8ff55da910c26971d4286abff53ea6e2 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed, 9 Sep 2015 19:06:33 +0200
Subject: [PATCH] perf: Restructure perf syscall point of no return

The exclusive_event_installable() stuff only works because its
exclusive with the grouping bits.

Rework the code such that there is a sane place to error out before we
go do things we cannot undo.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel(a)vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index ae16867..cc06c7c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8242,15 +8242,31 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
-	if (move_group) {
-		gctx = group_leader->ctx;
+	gctx = group_leader->ctx;
+	if (move_group)
+		mutex_lock_double(&gctx->mutex, &ctx->mutex);
+	else
+		mutex_lock(&ctx->mutex);
+
+	/*
+	 * Must be under the same ctx::mutex as perf_install_in_context(),
+	 * because we need to serialize with concurrent event creation.
+	 */
+	if (!exclusive_event_installable(event, ctx)) {
+		/* exclusive and group stuff are assumed mutually exclusive */
+		WARN_ON_ONCE(move_group);
+
+		err = -EBUSY;
+		goto err_locked;
+	}
 
+	WARN_ON_ONCE(ctx->parent_ctx);
+
+	if (move_group) {
 		/*
 		 * See perf_event_ctx_lock() for comments on the details
 		 * of swizzling perf_event::ctx.
 		 */
-		mutex_lock_double(&gctx->mutex, &ctx->mutex);
-
 		perf_remove_from_context(group_leader, false);
 
 		list_for_each_entry(sibling, &group_leader->sibling_list,
@@ -8258,13 +8274,7 @@ SYSCALL_DEFINE5(perf_event_open,
 			perf_remove_from_context(sibling, false);
 			put_ctx(gctx);
 		}
-	} else {
-		mutex_lock(&ctx->mutex);
-	}
-
-	WARN_ON_ONCE(ctx->parent_ctx);
 
-	if (move_group) {
 		/*
 		 * Wait for everybody to stop referencing the events through
 		 * the old lists, before installing it on new lists.
@@ -8296,22 +8306,20 @@ SYSCALL_DEFINE5(perf_event_open,
 		perf_event__state_init(group_leader);
 		perf_install_in_context(ctx, group_leader, group_leader->cpu);
 		get_ctx(ctx);
-	}
 
-	if (!exclusive_event_installable(event, ctx)) {
-		err = -EBUSY;
-		mutex_unlock(&ctx->mutex);
-		fput(event_file);
-		goto err_context;
+		/*
+		 * Now that all events are installed in @ctx, nothing
+		 * references @gctx anymore, so drop the last reference we have
+		 * on it.
+		 */
+		put_ctx(gctx);
 	}
 
 	perf_install_in_context(ctx, event, event->cpu);
 	perf_unpin_context(ctx);
 
-	if (move_group) {
+	if (move_group)
 		mutex_unlock(&gctx->mutex);
-		put_ctx(gctx);
-	}
 	mutex_unlock(&ctx->mutex);
 
 	put_online_cpus();
@@ -8338,6 +8346,12 @@ SYSCALL_DEFINE5(perf_event_open,
 	fd_install(event_fd, event_file);
 	return event_fd;
 
+err_locked:
+	if (move_group)
+		mutex_unlock(&gctx->mutex);
+	mutex_unlock(&ctx->mutex);
+/* err_file: */
+	fput(event_file);
 err_context:
 	perf_unpin_context(ctx);
 	put_ctx(ctx);
-- 
2.1.4

Re: [perf] 3215929f8f: n f[d 29 [ 4:22:177.]092021] EIP is at SyS_perf_event_open+0x757/0xb86

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Thu, Sep 17, 2015 at 05:06:39PM +0800, Fengguang Wu wrote:
> > How can I get the actual patch? I don't have this commit. (It could be a
> > previous version, I had a bug like that, but not having the actual patch
> > I cannot check if its the same or not).
> 
> Here is the patch:

Thanks!

> +	gctx = group_leader->ctx;
> +	if (move_group)
> +		mutex_lock_double(&gctx->mutex, &ctx->mutex);
> +	else
> +		mutex_lock(&ctx->mutex);

Yep, this is the buggy one. The current one looks like:

        if (move_group) {
                gctx = group_leader->ctx;
+               mutex_lock_double(&gctx->mutex, &ctx->mutex);
+       } else {
+               mutex_lock(&ctx->mutex);
+       }