From 12bf2ab391d33a49d74b123b66ef87c4ffa14e2e Mon Sep 17 00:00:00 2001 From: John Dias Date: Mon, 10 Oct 2016 14:44:30 -0700 Subject: perf: protect group_leader from races that cause ctx double-free When moving a group_leader perf event from a software-context to a hardware-context, there's a race in checking and updating that context. The existing locking solution doesn't work; note that it tries to grab a lock inside the group_leader's context object, which you can only get at by going through a pointer that should be protected from these races. To avoid that problem, and to produce a simple solution, we can just use a lock per group_leader to protect all checks on the group_leader's context. The new lock is grabbed and released when no context locks are held. RM-290 Bug: 30955111 Bug: 31095224 Change-Id: If37124c100ca6f4aa962559fba3bd5dbbec8e052 --- include/linux/perf_event.h | 6 ++++++ kernel/events/core.c | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 42dc246..82660a6 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -757,6 +757,12 @@ struct perf_event { int nr_siblings; int group_flags; struct perf_event *group_leader; + + /* + * Protect the pmu, attributes and context of a group leader. + * Note: does not protect the pointer to the group_leader. + */ + struct mutex group_leader_mutex; struct pmu *pmu; enum perf_event_active_state state; diff --git a/kernel/events/core.c b/kernel/events/core.c index 5dba2cb..466213f 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6362,6 +6362,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu, if (!group_leader) group_leader = event; + mutex_init(&event->group_leader_mutex); mutex_init(&event->child_mutex); INIT_LIST_HEAD(&event->child_list); @@ -6668,6 +6669,16 @@ SYSCALL_DEFINE5(perf_event_open, group_leader = NULL; } + /* + * Take the group_leader's group_leader_mutex before observing + * anything in the group leader that leads to changes in ctx, + * many of which may be changing on another thread. + * In particular, we want to take this lock before deciding + * whether we need to move_group. + */ + if (group_leader) + mutex_lock(&group_leader->group_leader_mutex); + if (pid != -1 && !(flags & PERF_FLAG_PID_CGROUP)) { task = find_lively_task_by_vpid(pid); if (IS_ERR(task)) { @@ -6836,6 +6847,9 @@ SYSCALL_DEFINE5(perf_event_open, } mutex_unlock(&ctx->mutex); + if (group_leader) + mutex_unlock(&group_leader->group_leader_mutex); + event->owner = current; mutex_lock(¤t->perf_event_mutex); @@ -6867,6 +6881,8 @@ err_task: if (task) put_task_struct(task); err_group_fd: + if (group_leader) + mutex_unlock(&group_leader->group_leader_mutex); fput_light(group_file, fput_needed); err_fd: put_unused_fd(event_fd); -- cgit v1.1