[PATCH] perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

[PATCH] perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

[Stephane Eranian]

This patch solves a stale pointer problem in
update_cgrp_time_from_cpuctx(). The cpuctx->cgrp
was not cleared on all possible event exit paths,
including:
   close()
     perf_release()
       perf_release_kernel()
         list_del_event()

This patch fixes list_del_event() to clear cpuctx->cgrp
when there are no cgroup events left in the context.

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

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 3472bb1..0c71422 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -941,6 +941,7 @@ static void perf_group_attach(struct perf_event *event)
 static void
 list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 {
+	struct perf_cpu_context *cpuctx;
 	/*
 	 * We can have double detach due to exit/hot-unplug + close.
 	 */
@@ -949,8 +950,17 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 
 	event->attach_state &= ~PERF_ATTACH_CONTEXT;
 
-	if (is_cgroup_event(event))
+	if (is_cgroup_event(event)) {
 		ctx->nr_cgroups--;
+		cpuctx = __get_cpu_context(ctx);
+		/*
+		 * if there are no more cgroup events
+		 * then clear cgrp to avoid stale pointer
+		 * in update_cgrp_time_from_cpuctx()
+		 */
+		if (!ctx->nr_cgroups)
+			cpuctx->cgrp = NULL;
+	}
 
 	ctx->nr_events--;
 	if (event->attr.inherit_stat)

Re: [PATCH] perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

[Ingo Molnar]

* Stephane Eranian <eranian@google.com> wrote:

> -	if (is_cgroup_event(event))
> +	if (is_cgroup_event(event)) {
>  		ctx->nr_cgroups--;
> +		cpuctx = __get_cpu_context(ctx);
> +		/*
> +		 * if there are no more cgroup events
> +		 * then clear cgrp to avoid stale pointer
> +		 * in update_cgrp_time_from_cpuctx()
> +		 */
> +		if (!ctx->nr_cgroups)
> +			cpuctx->cgrp = NULL;
> +	}

The ->cgrp pointer does not exist on !CGROUPS kernels. I suspect the cleanest 
approach would be to make those two cgrp fields available unconditionally in 
struct perf_event.

Thanks,

	Ingo

Re: [PATCH] perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

[Stephane Eranian]

On Wed, Mar 23, 2011 at 12:36 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Stephane Eranian <eranian@google.com> wrote:
>
>> -     if (is_cgroup_event(event))
>> +     if (is_cgroup_event(event)) {
>>               ctx->nr_cgroups--;
>> +             cpuctx = __get_cpu_context(ctx);
>> +             /*
>> +              * if there are no more cgroup events
>> +              * then clear cgrp to avoid stale pointer
>> +              * in update_cgrp_time_from_cpuctx()
>> +              */
>> +             if (!ctx->nr_cgroups)
>> +                     cpuctx->cgrp = NULL;
>> +     }
>
> The ->cgrp pointer does not exist on !CGROUPS kernels. I suspect the cleanest
> approach would be to make those two cgrp fields available unconditionally in
> struct perf_event.
>
Argh, I forgot to test !CGROUP.
I tend to agree with you on making those fields unconditional. We
don't save that
much by not doing it.

> Thanks,
>
>        Ingo
>