[PATCH 1/4] perf cgroup: Fix leak of file reference count

[PATCH 1/4] perf cgroup: Fix leak of file reference count

[Li Zefan]

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

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/perf_event.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 64a018e..4a955fd 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -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

[PATCH 2/4] perf cgroup: Fix unmatched call to perf_detach_cgroup()

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 kernel/perf_event.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 4a955fd..dca92b2 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -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

[PATCH 3/4] perf cgroup: Clean up perf_cgroup_create()

[Li Zefan]

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

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/perf_event.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index dca92b2..d6b3d16 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -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

[PATCH 4/4] perf cgroup: Fix a typo in kernel config

[Li Zefan]

s/specificied/specified

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 init/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 20d6bd9..4c4edf2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -688,7 +688,7 @@ config CGROUP_PERF
 	depends on PERF_EVENTS && CGROUPS
 	help
 	  This option extends the per-cpu mode to restrict monitoring to
-	  threads which belong to the cgroup specificied and run on the
+	  threads which belong to the cgroup specified and run on the
 	  designated cpu.
 
 	  Say N if unsure.
-- 
1.6.3

Re: [PATCH 1/4] perf cgroup: Fix leak of file reference count

[Stephane Eranian]

On Thu, Mar 3, 2011 at 7:25 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> In perf_cgroup_connect(), fput_light() is missing in a failure
> path.
>
Yes, you're right.

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/perf_event.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 64a018e..4a955fd 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -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
>

Re: [PATCH 2/4] perf cgroup: Fix unmatched call to perf_detach_cgroup()

[Stephane Eranian]

On Thu, Mar 3, 2011 at 7:25 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> In the failure path, we call perf_detach_cgroup(), but we didn't
> call perf_get_cgroup() prio to it.
>
the funny thing is that I had this change at some point. But apparently
it got lost in the many revisions of the patch. Thanks for fixing it.

Acked-by: Stephane Eranian <eranian@google.com>

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/perf_event.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 4a955fd..dca92b2 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -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
>

Re: [PATCH 3/4] perf cgroup: Clean up perf_cgroup_create()

[Stephane Eranian]

On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> - Use kzalloc() to replace kmalloc() + memset().
> - Remove redundant initialization, since alloc_percpu() returns
>  zero-filled percpu memory.
>
Acked-by: Stephane Eranian <eranian@google.com>

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/perf_event.c |   11 +----------
>  1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index dca92b2..d6b3d16 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -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
>

Re: [PATCH 3/4] perf cgroup: Clean up perf_cgroup_create()

[Stephane Eranian]

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.


On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> - Use kzalloc() to replace kmalloc() + memset().
> - Remove redundant initialization, since alloc_percpu() returns
>  zero-filled percpu memory.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/perf_event.c |   11 +----------
>  1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index dca92b2..d6b3d16 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -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
>

Re: [PATCH 3/4] perf cgroup: Clean up perf_cgroup_create()

[Stephane Eranian]

Ok,


I checked and yes, alloc_percpu() does return zeroed out memory.
So the patch looks good to me.
Thanks.


On Thu, Mar 3, 2011 at 9:37 AM, Stephane Eranian <eranian@google.com> wrote:
> 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.
>
>
> On Thu, Mar 3, 2011 at 7:26 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> - Use kzalloc() to replace kmalloc() + memset().
>> - Remove redundant initialization, since alloc_percpu() returns
>>  zero-filled percpu memory.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/perf_event.c |   11 +----------
>>  1 files changed, 1 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>> index dca92b2..d6b3d16 100644
>> --- a/kernel/perf_event.c
>> +++ b/kernel/perf_event.c
>> @@ -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
>>
>

Re: [PATCH 1/4] perf cgroup: Fix leak of file reference count

[Peter Zijlstra]

Applied all 4, 

Thanks Li!

[tip:perf/core] perf cgroup: Fix leak of file reference count

[tip-bot for Li Zefan]

Commit-ID:  3db272c0494900fcb905a201180a78cae3addd6e
Gitweb:     http://git.kernel.org/tip/3db272c0494900fcb905a201180a78cae3addd6e
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 3 Mar 2011 14:25:37 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 4 Mar 2011 11:32:50 +0100

perf cgroup: Fix leak of file reference count

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

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <4D6F3461.6060406@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 821ce82..7c999e8 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -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;
 }

[tip:perf/core] perf cgroup: Fix unmatched call to perf_detach_cgroup()

[tip-bot for Li Zefan]

Commit-ID:  f75e18cb9627b1d3d752b83a0b5563da0042c50a
Gitweb:     http://git.kernel.org/tip/f75e18cb9627b1d3d752b83a0b5563da0042c50a
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 3 Mar 2011 14:25:50 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100

perf cgroup: Fix unmatched call to perf_detach_cgroup()

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 <lizf@cn.fujitsu.com>
Acked-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <4D6F346E.9070606@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 7c999e8..b002095 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -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);

[tip:perf/core] perf cgroup: Clean up perf_cgroup_create()

[tip-bot for Li Zefan]

Commit-ID:  1b15d0558e82df9b3659804ceb44187b98eda354
Gitweb:     http://git.kernel.org/tip/1b15d0558e82df9b3659804ceb44187b98eda354
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 3 Mar 2011 14:26:06 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100

perf cgroup: Clean up perf_cgroup_create()

- Use kzalloc() to replace kmalloc() + memset().

- Remove redundant initialization, since alloc_percpu() returns
  zero-filled percpu memory.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <4D6F347E.2010806@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index b002095..193b190 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -7346,26 +7346,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;
 }
 

[tip:perf/core] perf cgroup: Fix a typo in kernel config

[tip-bot for Li Zefan]

Commit-ID:  2d0f25201ee210a0666ec9c41538ba05a07f8bc6
Gitweb:     http://git.kernel.org/tip/2d0f25201ee210a0666ec9c41538ba05a07f8bc6
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 3 Mar 2011 14:26:20 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 4 Mar 2011 11:32:51 +0100

perf cgroup: Fix a typo in kernel config

s/specificied/specified

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <4D6F348C.2050804@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 init/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 20d6bd9..4c4edf2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -688,7 +688,7 @@ config CGROUP_PERF
 	depends on PERF_EVENTS && CGROUPS
 	help
 	  This option extends the per-cpu mode to restrict monitoring to
-	  threads which belong to the cgroup specificied and run on the
+	  threads which belong to the cgroup specified and run on the
 	  designated cpu.
 
 	  Say N if unsure.