On Mon, Oct 14, 2019 at 11:35:44AM +0200, Peter Zijlstra wrote:

On Fri, Oct 11, 2019 at 12:03:30PM -0400, Joel Fernandes (Google) wrote:

quoted

@@ -4761,6 +4762,7 @@ int perf_event_release_kernel(struct perf_event *event)
 	}
 
 no_ctx:
+	security_perf_event_free(event);
 	put_event(event); /* Must be the 'last' reference */
 	return 0;
 }

quoted

@@ -10553,11 +10568,16 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 		}
 	}
 
+	err = security_perf_event_alloc(event);
+	if (err)
+		goto err_security;
+
 	/* symmetric to unaccount_event() in _free_event() */
 	account_event(event);
 
 	return event;
 
+err_security:
 err_addr_filters:
 	kfree(event->addr_filter_ranges);
 

There's a bunch of problems here I think:

 - err_security is named wrong; the naming scheme is to name the label
   after the last thing that succeeded / first thing that needs to be
   undone.

 - per that, you're forgetting to undo 'get_callchain_buffers()'

Yes, you're right. Tested your fix below. Sorry to miss this.

 - perf_event_release_kernel() is not a full match to
   perf_event_alloc(), inherited events get created by
   perf_event_alloc() but never pass through
   perf_event_release_kernel().

Oh, through inherit_event(). Thanks for pointing this semantic out, did not
know that.

I'm thinking the below patch on top should ammend these issues; please
verify.

Yes, applied your diff below and verified that the events are getting freed
as they were in my initial set of tests. The diff also looks good to me.

I squashed your diff below and will resend as v3. Since you modified this
patch a lot, I will add your Co-developed-by tag as well.

thanks, Peter!

 - Joel