perf cgroup: Fix leak of file reference count

In the failure path, we call perf_detach_cgroup(), but we didn't call perf_get_cgroup() prio to it.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/perf_event.c |    6 +++---

 1 files changed, 3 insertions(+), 3 deletions(-)

kernel/perf_event.c +3 -3

index 4a955fd..dca92b2 100644
@@ -412,6 +412,9 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
        cgrp = container_of(css, struct perf_cgroup, css);
        event->cgrp = cgrp;
 
+       /* must be done before we fput() the file */
+       perf_get_cgroup(event);
+
        /*
         * all events in a group must monitor
         * the same cgroup because a task belongs

@@ -420,9 +423,6 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
        if (group_leader && group_leader->cgrp != cgrp) {
                perf_detach_cgroup(event);
                ret = -EINVAL;
-       } else {
-               /* must be done before we fput() the file */
-               perf_get_cgroup(event);
        }

 out:
       fput_light(file, fput_needed);
--
1.6.3

- Use kzalloc() to replace kmalloc() + memset().
- Remove redundant initialization, since alloc_percpu() returns zero-filled percpu memory.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/perf_event.c |   11 +----------

 1 files changed, 1 insertions(+), 10 deletions(-)

kernel/perf_event.c +1 -11

index dca92b2..d6b3d16 100644
@@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
        struct cgroup_subsys *ss, struct cgroup *cont)
  {
        struct perf_cgroup *jc;
-       struct perf_cgroup_info *t;
-       int c;
 
-       jc = kmalloc(sizeof(*jc), GFP_KERNEL);
+       jc = kzalloc(sizeof(*jc), GFP_KERNEL);
        if (!jc)
                return ERR_PTR(-ENOMEM);
 
-       memset(jc, 0, sizeof(*jc));
-
        jc->info = alloc_percpu(struct perf_cgroup_info);
        if (!jc->info) {
                kfree(jc);
                return ERR_PTR(-ENOMEM);
        }
 
-       for_each_possible_cpu(c) {
-               t = per_cpu_ptr(jc->info, c);
-               t->time = 0;
-               t->timestamp = 0;
-       }
        return &jc->css;
  }
 
--

1.6.3

- Use kzalloc() to replace kmalloc() + memset().
- Remove redundant initialization, since alloc_percpu() returns zero-filled percpu memory.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/perf_event.c |   11 +----------

 1 files changed, 1 insertions(+), 10 deletions(-)

kernel/perf_event.c +1 -11

index dca92b2..d6b3d16 100644
@@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
        struct cgroup_subsys *ss, struct cgroup *cont)
  {
        struct perf_cgroup *jc;
-       struct perf_cgroup_info *t;
-       int c;
 
-       jc = kmalloc(sizeof(*jc), GFP_KERNEL);
+       jc = kzalloc(sizeof(*jc), GFP_KERNEL);
        if (!jc)
                return ERR_PTR(-ENOMEM);
 
-       memset(jc, 0, sizeof(*jc));
-
        jc->info = alloc_percpu(struct perf_cgroup_info);
        if (!jc->info) {
                kfree(jc);
                return ERR_PTR(-ENOMEM);
        }
 
-       for_each_possible_cpu(c) {
-               t = per_cpu_ptr(jc->info, c);
-               t->time = 0;
-               t->timestamp = 0;
-       }
        return &jc->css;
  }
 
--

1.6.3

Li,

I have to NACK this patch or at least request more information.

The for_each_possible_cpu() is initializing what's allocated via alloc_percpu(), jc->info and NOT jc.

I don't think this gets zeroed by this allocator. But I could be wrong.

Quoted 44 lines · Li Zefan

- Use kzalloc() to replace kmalloc() + memset().
- Remove redundant initialization, since alloc_percpu() returns zero-filled percpu memory.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/perf_event.c |   11 +----------

 1 files changed, 1 insertions(+), 10 deletions(-)

kernel/perf_event.c +1 -11

index dca92b2..d6b3d16 100644
@@ -7341,26 +7341,17 @@ static struct cgroup_subsys_state *perf_cgroup_create(
        struct cgroup_subsys *ss, struct cgroup *cont)
  {
        struct perf_cgroup *jc;
-       struct perf_cgroup_info *t;
-       int c;
 
-       jc = kmalloc(sizeof(*jc), GFP_KERNEL);
+       jc = kzalloc(sizeof(*jc), GFP_KERNEL);
        if (!jc)
                return ERR_PTR(-ENOMEM);
 
-       memset(jc, 0, sizeof(*jc));
-
        jc->info = alloc_percpu(struct perf_cgroup_info);
        if (!jc->info) {
                kfree(jc);
                return ERR_PTR(-ENOMEM);
        }
 
-       for_each_possible_cpu(c) {
-               t = per_cpu_ptr(jc->info, c);
-               t->time = 0;
-               t->timestamp = 0;
-       }
        return &jc->css;
  }
 
--

1.6.3

In perf_cgroup_connect(), fput_light() is missing in a failure path.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/perf_event.c |    7 +++++--

 1 files changed, 5 insertions(+), 2 deletions(-)

kernel/perf_event.c +5 -3

index 64a018e..4a955fd 100644
@@ -404,8 +404,10 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
                return -EBADF;
 
        css = cgroup_css_from_dir(file, perf_subsys_id);
-       if (IS_ERR(css))
-               return PTR_ERR(css);
+       if (IS_ERR(css)) {
+               ret = PTR_ERR(css);
+               goto out;
+       }
 
        cgrp = container_of(css, struct perf_cgroup, css);
        event->cgrp = cgrp;

@@ -422,6 +424,7 @@ static inline int perf_cgroup_connect(int fd, struct perf_event *event,
                /* must be done before we fput() the file */
                perf_get_cgroup(event);
        }
+out:
        fput_light(file, fput_needed);
        return ret;
  }
--

1.6.3