perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

PATCH In review Stephane Eranian 2 participants 3 messages Mar 21, 2011 – Mar 23, 2011

Share & tools

Stephane Eranian

author

2011-03-21 15:12 UTC #0 lore raw

PATCH 1/1 perf_events: fix cgrp stale pointer in update_cgrp_time_from_cpuctx()

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 <[email protected]>

---

kernel/perf_event.c +11 -1

index 3472bb1..0c71422 100644
@@ -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)

Ingo Molnar

2011-03-23 11:36 UTC #1 lore raw

kernel/perf_event.c list_del_event(struct perf_event *event, struct perf_event_context *ctx) · line 963

Quoted 12 lines · Stephane Eranian

kernel/perf_event.c +10 -1

-	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

Stephane Eranian

author

2011-03-23 12:48 UTC #2 lore raw

Quoted 16 lines · Ingo Molnar

Quoted 4 lines · Stephane Eranian
- if (is_cgroup_event(event))
+ if (is_cgroup_event(event)) {
ctx->nr_cgroups--;
+ cpuctx = __get_cpu_context(ctx);
Quoted 8 lines
+ /*
+ * 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;
+ }

Quoted 4 lines

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.

Quoted 4 lines

Thanks,
Ingo