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

Groups > linux.kernel > #1632919 > unrolled thread

[RFC PATCH] trace/perf: cure locking issue in perf_event_open() error path

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH] trace/perf: cure locking issue in perf_event_open()  error path Sebastian Siewior <bigeasy@linutronix.de> - 2017-04-28 16:30 +0200
    Re: [RFC PATCH] trace/perf: cure locking issue in perf_event_open()  error path Sebastian Siewior <bigeasy@linutronix.de> - 2017-04-28 16:40 +0200
    [tip:smp/hotplug] perf: Reorder cpu hotplug rwsem against  cred_guard_mutex tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-05-01 15:10 +0200
    [tip:smp/hotplug] perf: Push hotplug protection down to callers tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-05-01 15:10 +0200

#1632919 — [RFC PATCH] trace/perf: cure locking issue in perf_event_open() error path

As trinity figured out, there is a recursive get_online_cpus() in
perf_event_open()'s error path:
| Call Trace:
|  dump_stack+0x86/0xce
|  __lock_acquire+0x2520/0x2cd0
|  lock_acquire+0x27c/0x2f0
|  get_online_cpus+0x3d/0x80
|  static_key_slow_dec+0x5a/0x70
|  sw_perf_event_destroy+0x8e/0x100
|  _free_event+0x61b/0x800
|  free_event+0x68/0x70
|  SyS_perf_event_open+0x19db/0x1d80

In order to cure, I am moving free_event() after the put_online_cpus()
block.
Besides that one, there also the error path in perf_event_alloc() which
also invokes event->destory. Here I delayed the destory work to
schedule_work().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

I am not quite happy with the schedule_work() part.

 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 52 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 24a635887f28..d6a874dbbd21 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -718,6 +718,7 @@ struct perf_event {
 #endif
 
 	struct list_head		sb_list;
+	struct work_struct		destroy_work;
 #endif /* CONFIG_PERF_EVENTS */
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7aed78b516fc..3358889609f8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9320,6 +9320,16 @@ static void account_event(struct perf_event *event)
 	account_pmu_sb_event(event);
 }
 
+static void perf_alloc_destroy_ev(struct work_struct *work)
+{
+	struct perf_event *event;
+
+	event = container_of(work, struct perf_event, destroy_work);
+	event->destroy(event);
+	module_put(event->pmu->module);
+	kfree(event);
+}
+
 /*
  * Allocate and initialize a event structure
  */
@@ -9334,6 +9344,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	struct pmu *pmu;
 	struct perf_event *event;
 	struct hw_perf_event *hwc;
+	bool delay_destroy = false;
 	long err = -EINVAL;
 
 	if ((unsigned)cpu >= nr_cpu_ids) {
@@ -9497,15 +9508,22 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	exclusive_event_destroy(event);
 
 err_pmu:
-	if (event->destroy)
-		event->destroy(event);
-	module_put(pmu->module);
+	if (event->destroy) {
+		/* delay ->destroy due to nested get_online_cpus() */
+		INIT_WORK(&event->destroy_work, perf_alloc_destroy_ev);
+		delay_destroy = true;
+	} else {
+		module_put(pmu->module);
+	}
 err_ns:
 	if (is_cgroup_event(event))
 		perf_detach_cgroup(event);
 	if (event->ns)
 		put_pid_ns(event->ns);
-	kfree(event);
+	if (delay_destroy)
+		schedule_work(&event->destroy_work);
+	else
+		kfree(event);
 
 	return ERR_PTR(err);
 }
@@ -9798,7 +9816,7 @@ SYSCALL_DEFINE5(perf_event_open,
 		pid_t, pid, int, cpu, int, group_fd, unsigned long, flags)
 {
 	struct perf_event *group_leader = NULL, *output_event = NULL;
-	struct perf_event *event, *sibling;
+	struct perf_event *event = NULL, *sibling;
 	struct perf_event_attr attr;
 	struct perf_event_context *ctx, *uninitialized_var(gctx);
 	struct file *event_file = NULL;
@@ -9908,13 +9926,14 @@ SYSCALL_DEFINE5(perf_event_open,
 				 NULL, NULL, cgroup_fd);
 	if (IS_ERR(event)) {
 		err = PTR_ERR(event);
+		event = NULL;
 		goto err_cred;
 	}
 
 	if (is_sampling_event(event)) {
 		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
 			err = -EOPNOTSUPP;
-			goto err_alloc;
+			goto err_cred;
 		}
 	}
 
@@ -9927,7 +9946,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (attr.use_clockid) {
 		err = perf_event_set_clock(event, attr.clockid);
 		if (err)
-			goto err_alloc;
+			goto err_cred;
 	}
 
 	if (pmu->task_ctx_nr == perf_sw_context)
@@ -9962,7 +9981,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	ctx = find_get_context(pmu, task, event);
 	if (IS_ERR(ctx)) {
 		err = PTR_ERR(ctx);
-		goto err_alloc;
+		goto err_cred;
 	}
 
 	if ((pmu->capabilities & PERF_PMU_CAP_EXCLUSIVE) && group_leader) {
@@ -10186,18 +10205,21 @@ SYSCALL_DEFINE5(perf_event_open,
 err_context:
 	perf_unpin_context(ctx);
 	put_ctx(ctx);
-err_alloc:
-	/*
-	 * If event_file is set, the fput() above will have called ->release()
-	 * and that will take care of freeing the event.
-	 */
-	if (!event_file)
-		free_event(event);
 err_cred:
 	if (task)
 		mutex_unlock(&task->signal->cred_guard_mutex);
 err_cpus:
 	put_online_cpus();
+	/*
+	 * The event cleanup should happen earlier (as per cleanup in reverse
+	 * allocation order). It is delayed after the put_online_cpus() section
+	 * so we don't invoke event->destroy in it and risk recursive invocation
+	 * of it via static_key_slow_dec().
+	 * If event_file is set, the fput() above will have called ->release()
+	 * and that will take care of freeing the event.
+	 */
+	if (event && !event_file)
+		free_event(event);
 err_task:
 	if (task)
 		put_task_struct(task);
-- 
2.11.0

[toc] | [next] | [standalone]

#1632932

With the last patch on-top I trigger this now and then:

 ======================================================
 WARNING: possible circular locking dependency detected
 4.11.0-rc8-00894-g8bd462ee4aac-dirty #84 Not tainted
 ------------------------------------------------------
 trinity-subchil/4966 is trying to acquire lock:
  (cpu_hotplug_lock.rw_sem){++++++}, at: [<ffffffff812fcdad>] tp_perf_event_destroy+0xd/0x20

 but task is already holding lock:
  (&ctx->mutex){+.+.+.}, at: [<ffffffff81316ede>] perf_event_exit_task+0x2ae/0x8d0

 which lock already depends on the new lock.


 the existing dependency chain (in reverse order) is:

 -> #2 (&ctx->mutex){+.+.+.}:
        __lock_acquire+0x2534/0x2cd0
        lock_acquire+0x27c/0x2f0
        __mutex_lock+0xef/0x1280
        mutex_lock_nested+0x16/0x20
        SyS_perf_event_open+0x11ab/0x1e80
        entry_SYSCALL_64_fastpath+0x23/0xc2

 -> #1 (&sig->cred_guard_mutex){+.+.+.}:
        __lock_acquire+0x2534/0x2cd0
        lock_acquire+0x27c/0x2f0
        __mutex_lock+0xef/0x1280
        mutex_lock_interruptible_nested+0x16/0x20
        SyS_perf_event_open+0x1bd6/0x1e80
        entry_SYSCALL_64_fastpath+0x23/0xc2

 -> #0 (cpu_hotplug_lock.rw_sem){++++++}:
        check_prevs_add+0x544/0x18f0
        __lock_acquire+0x2534/0x2cd0
        lock_acquire+0x27c/0x2f0
        get_online_cpus+0x3d/0x80
        tp_perf_event_destroy+0xd/0x20
        _free_event+0x61b/0x800
        free_event+0x68/0x70
        perf_event_exit_task+0x816/0x8d0
        do_exit+0xc90/0x2a90
        do_group_exit+0x1aa/0x2b0
        SyS_exit_group+0x18/0x20
        entry_SYSCALL_64_fastpath+0x23/0xc2

 other info that might help us debug this:

 Chain exists of:
   cpu_hotplug_lock.rw_sem --> &sig->cred_guard_mutex --> &ctx->mutex

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&ctx->mutex);
                                lock(&sig->cred_guard_mutex);
                                lock(&ctx->mutex);
   lock(cpu_hotplug_lock.rw_sem);

  *** DEADLOCK ***

 1 lock held by trinity-subchil/4966:
  #0:  (&ctx->mutex){+.+.+.}, at: [<ffffffff81316ede>] perf_event_exit_task+0x2ae/0x8d0

 stack backtrace:
 CPU: 3 PID: 4966 Comm: trinity-subchil Not tainted 4.11.0-rc8-00894-g8bd462ee4aac-dirty #84
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1 04/01/2014
 Call Trace:
  dump_stack+0x86/0xce
  print_circular_bug+0x5c3/0x620
  ? lockdep_on+0x50/0x50
  check_prevs_add+0x544/0x18f0
  ? check_irq_usage+0x150/0x150
  ? lock_acquire+0x27c/0x2f0
  __lock_acquire+0x2534/0x2cd0
  ? __lock_acquire+0x2534/0x2cd0
  ? find_held_lock+0x36/0x1c0
  lock_acquire+0x27c/0x2f0
  ? tp_perf_event_destroy+0xd/0x20
  get_online_cpus+0x3d/0x80
  ? tp_perf_event_destroy+0xd/0x20
  tp_perf_event_destroy+0xd/0x20
  _free_event+0x61b/0x800
  free_event+0x68/0x70
  perf_event_exit_task+0x816/0x8d0
  do_exit+0xc90/0x2a90
  ? mm_update_next_owner+0x550/0x550
  ? getname_flags+0xde/0x370
  ? getname_flags+0xde/0x370
  ? rcu_read_lock_sched_held+0x14a/0x180
  ? prepare_bprm_creds+0xf0/0xf0
  ? kmem_cache_free+0x250/0x2c0
  ? getname_flags+0xde/0x370
  ? entry_SYSCALL_64_fastpath+0x5/0xc2
  do_group_exit+0x1aa/0x2b0
  SyS_exit_group+0x18/0x20
  entry_SYSCALL_64_fastpath+0x23/0xc2

and I am not sure what to do here…

Sebastian

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

#1633657 — [tip:smp/hotplug] perf: Reorder cpu hotplug rwsem against cred_guard_mutex

Commit-ID:  1526eee294dd52b70804aa377579682cc4dcd9ad
Gitweb:     http://git.kernel.org/tip/1526eee294dd52b70804aa377579682cc4dcd9ad
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 1 May 2017 14:35:45 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 1 May 2017 14:54:40 +0200

perf: Reorder cpu hotplug rwsem against cred_guard_mutex

sys_perf_event_open() takes the hotplug rwsem before the
cred_guard_mutex. The exit() path has the reverse lock order.

The hotplug protection in sys_perf_event_open() is not required before
taking the cred_guard_mutex, so it can be reordered there.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Link: http://lkml.kernel.org/r/20170428142456.5xh44ef3fv7w2kkh@linutronix.de
---
 kernel/events/core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 997123c..71d8c74 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9742,12 +9742,10 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_task;
 	}
 
-	get_online_cpus();
-
 	if (task) {
 		err = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
 		if (err)
-			goto err_cpus;
+			goto err_task;
 
 		/*
 		 * Reuse ptrace permission checks for now.
@@ -9765,11 +9763,13 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (flags & PERF_FLAG_PID_CGROUP)
 		cgroup_fd = pid;
 
+	get_online_cpus();
+
 	event = perf_event_alloc(&attr, cpu, task, group_leader, NULL,
 				 NULL, NULL, cgroup_fd);
 	if (IS_ERR(event)) {
 		err = PTR_ERR(event);
-		goto err_cred;
+		goto err_cpus;
 	}
 
 	if (is_sampling_event(event)) {
@@ -10017,13 +10017,13 @@ SYSCALL_DEFINE5(perf_event_open,
 		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 
+	put_online_cpus();
+
 	if (task) {
 		mutex_unlock(&task->signal->cred_guard_mutex);
 		put_task_struct(task);
 	}
 
-	put_online_cpus();
-
 	mutex_lock(&current->perf_event_mutex);
 	list_add_tail(&event->owner_entry, &current->perf_event_list);
 	mutex_unlock(&current->perf_event_mutex);
@@ -10054,11 +10054,11 @@ err_alloc:
 	 */
 	if (!event_file)
 		free_event(event);
+err_cpus:
+	put_online_cpus();
 err_cred:
 	if (task)
 		mutex_unlock(&task->signal->cred_guard_mutex);
-err_cpus:
-	put_online_cpus();
 err_task:
 	if (task)
 		put_task_struct(task);

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

#1633658 — [tip:smp/hotplug] perf: Push hotplug protection down to callers

Commit-ID:  74b3980ba87e6bdb78694600e234f91fef592dbd
Gitweb:     http://git.kernel.org/tip/74b3980ba87e6bdb78694600e234f91fef592dbd
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 1 May 2017 09:57:10 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 1 May 2017 14:54:41 +0200

perf: Push hotplug protection down to callers

There are various code pathes invoked from event init/release which take
the hotplug rwsem. That's either nesting in a region which holds the
hotplug rwsem already or creates reverse lock ordering.

Push the hotplug protection down to the core call sites and remove the
hotplug locking in the various init/destroy functions.

There is a subtle problem with the error exit path in sys_perf_event_open()
where fput() calls the release function from the hotplug protected
region. Solve this by manually releasing the event and preventing the
release function from doing so.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Link: http://lkml.kernel.org/r/20170428142456.5xh44ef3fv7w2kkh@linutronix.de
---
 arch/x86/events/intel/ds.c |  7 ++-----
 kernel/events/core.c       | 29 +++++++++++++++++++++++------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 9dfeeec..f42db0c 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -391,7 +391,7 @@ void release_ds_buffers(void)
 	if (!x86_pmu.bts && !x86_pmu.pebs)
 		return;
 
-	get_online_cpus();
+	lockdep_assert_hotplug_held();
 	for_each_online_cpu(cpu)
 		fini_debug_store_on_cpu(cpu);
 
@@ -400,7 +400,6 @@ void release_ds_buffers(void)
 		release_bts_buffer(cpu);
 		release_ds_buffer(cpu);
 	}
-	put_online_cpus();
 }
 
 void reserve_ds_buffers(void)
@@ -420,7 +419,7 @@ void reserve_ds_buffers(void)
 	if (!x86_pmu.pebs)
 		pebs_err = 1;
 
-	get_online_cpus();
+	lockdep_assert_hotplug_held();
 
 	for_each_possible_cpu(cpu) {
 		if (alloc_ds_buffer(cpu)) {
@@ -461,8 +460,6 @@ void reserve_ds_buffers(void)
 		for_each_online_cpu(cpu)
 			init_debug_store_on_cpu(cpu);
 	}
-
-	put_online_cpus();
 }
 
 /*
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 71d8c74..fc39c22 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4337,7 +4337,15 @@ EXPORT_SYMBOL_GPL(perf_event_release_kernel);
  */
 static int perf_release(struct inode *inode, struct file *file)
 {
-	perf_event_release_kernel(file->private_data);
+	/*
+	 * The error exit path of sys_perf_event_open() might have released
+	 * the event already and cleared file->private_data.
+	 */
+	if (file->private_data) {
+		get_online_cpus();
+		perf_event_release_kernel(file->private_data);
+		put_online_cpus();
+	}
 	return 0;
 }
 
@@ -7619,7 +7627,7 @@ static void sw_perf_event_destroy(struct perf_event *event)
 
 	WARN_ON(event->parent);
 
-	static_key_slow_dec(&perf_swevent_enabled[event_id]);
+	static_key_slow_dec_cpuslocked(&perf_swevent_enabled[event_id]);
 	swevent_hlist_put();
 }
 
@@ -7783,9 +7791,7 @@ EXPORT_SYMBOL_GPL(perf_tp_event);
 
 static void tp_perf_event_destroy(struct perf_event *event)
 {
-	get_online_cpus();
 	perf_trace_destroy(event);
-	put_online_cpus();
 }
 
 static int perf_tp_event_init(struct perf_event *event)
@@ -10043,6 +10049,12 @@ err_locked:
 		perf_event_ctx_unlock(group_leader, gctx);
 	mutex_unlock(&ctx->mutex);
 /* err_file: */
+	/*
+	 * Release the event manually to avoid hotplug lock recursion in
+	 * perf_release().
+	 */
+	event_file->private_data = NULL;
+	perf_event_release_kernel(event);
 	fput(event_file);
 err_context:
 	perf_unpin_context(ctx);
@@ -10086,10 +10098,10 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 	struct perf_event *event;
 	int err;
 
+	get_online_cpus();
 	/*
 	 * Get the target context (task or percpu):
 	 */
-
 	event = perf_event_alloc(attr, cpu, task, NULL, NULL,
 				 overflow_handler, context, -1);
 	if (IS_ERR(event)) {
@@ -10121,7 +10133,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 	perf_install_in_context(ctx, event, cpu);
 	perf_unpin_context(ctx);
 	mutex_unlock(&ctx->mutex);
-
+	put_online_cpus();
 	return event;
 
 err_unlock:
@@ -10131,6 +10143,7 @@ err_unlock:
 err_free:
 	free_event(event);
 err:
+	put_online_cpus();
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(perf_event_create_kernel_counter);
@@ -10364,8 +10377,10 @@ void perf_event_exit_task(struct task_struct *child)
 	}
 	mutex_unlock(&child->perf_event_mutex);
 
+	get_online_cpus();
 	for_each_task_context_nr(ctxn)
 		perf_event_exit_task_context(child, ctxn);
+	put_online_cpus();
 
 	/*
 	 * The perf_event_exit_task_context calls perf_event_task
@@ -10410,6 +10425,7 @@ void perf_event_free_task(struct task_struct *task)
 	struct perf_event *event, *tmp;
 	int ctxn;
 
+	get_online_cpus();
 	for_each_task_context_nr(ctxn) {
 		ctx = task->perf_event_ctxp[ctxn];
 		if (!ctx)
@@ -10434,6 +10450,7 @@ void perf_event_free_task(struct task_struct *task)
 		mutex_unlock(&ctx->mutex);
 		put_ctx(ctx);
 	}
+	put_online_cpus();
 }
 
 void perf_event_delayed_put(struct task_struct *task)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web