[PATCH] perf: Use css_tryget to avoid propping up css refcount.

[PATCH] perf: Use css_tryget to avoid propping up css refcount.

[Salman Qazi]

An rmdir pushes css's ref count to zero.  However, if the associated
directory is open at the time, the dentry ref count is non-zero.  If
the fd for this directory is then passed into perf_event_open, it
does a css_get().  This bounces the ref count back up from zero.  This
is a problem by itself.  But what makes it turn into a crash is the
fact that we end up doing an extra dput, since we perform a dput
when css_put sees the ref count go down to zero.

css_tryget does not fall into that trap. So, we use that instead.

Reproduction case for the bug:

#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <linux/unistd.h>
#include <linux/perf_event.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>

#define PERF_FLAG_PID_CGROUP    (1U << 2)

int perf_event_open(struct perf_event_attr *hw_event_uptr,
                    pid_t pid, int cpu, int group_fd, unsigned long flags) {
  return syscall(__NR_perf_event_open,hw_event_uptr, pid, cpu,
                 group_fd, flags);
}

/* Directly poke at the perf_event bug, since it's proving hard to repro
 * depending on where in the kernel tree.  what moved?
 */
int main(int argc, char **argv)
{
        int fd;
        struct perf_event_attr attr;
        memset(&attr, 0, sizeof(attr));
        attr.exclude_kernel = 1;
        attr.size = sizeof(attr);
        mkdir("/dev/cgroup/perf_event/blah", 0777);
        fd = open("/dev/cgroup/perf_event/blah", O_RDONLY);
        perror("open");
        rmdir("/dev/cgroup/perf_event/blah");
        sleep(2);
        perf_event_open(&attr, fd, 0, -1,  PERF_FLAG_PID_CGROUP);
        perror("perf_event_open");
        close(fd);
        return 0;
}

Signed-off-by: Salman Qazi <sqazi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 kernel/events/core.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f85c015..d7d71d6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -253,9 +253,9 @@ perf_cgroup_match(struct perf_event *event)
 	return !event->cgrp || event->cgrp == cpuctx->cgrp;
 }
 
-static inline void perf_get_cgroup(struct perf_event *event)
+static inline bool perf_tryget_cgroup(struct perf_event *event)
 {
-	css_get(&event->cgrp->css);
+	return css_tryget(&event->cgrp->css);
 }
 
 static inline void perf_put_cgroup(struct perf_event *event)
@@ -484,7 +484,11 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
 	event->cgrp = cgrp;
 
 	/* must be done before we fput() the file */
-	perf_get_cgroup(event);
+	if (!perf_tryget_cgroup(event)) {
+		event->cgrp = NULL;
+		ret = -ENOENT;
+		goto out;
+	}
 
 	/*
 	 * all events in a group must monitor

Re: [PATCH] perf: Use css_tryget to avoid propping up css refcount.

[Tejun Heo]

On Thu, Jun 14, 2012 at 03:31:09PM -0700, Salman Qazi wrote:
> An rmdir pushes css's ref count to zero.  However, if the associated
> directory is open at the time, the dentry ref count is non-zero.  If
> the fd for this directory is then passed into perf_event_open, it
> does a css_get().  This bounces the ref count back up from zero.  This
> is a problem by itself.  But what makes it turn into a crash is the
> fact that we end up doing an extra dput, since we perform a dput
> when css_put sees the ref count go down to zero.
> 
> css_tryget does not fall into that trap. So, we use that instead.
> 
...
> Signed-off-by: Salman Qazi <sqazi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Peter, can you please pick up this one?

Thanks.

-- 
tejun

Re: [PATCH] perf: Use css_tryget to avoid propping up css refcount.

[Peter Zijlstra]

On Fri, 2012-06-15 at 10:40 +0900, Tejun Heo wrote:
> Peter, can you please pick up this one?

Done, thanks!