On Thu, Oct 10, 2019 at 11:13:33AM -0400, Joel Fernandes wrote:

On Thu, Oct 10, 2019 at 10:12:51AM +0200, Peter Zijlstra wrote:

quoted

+static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
 {
-	return sysctl_perf_event_paranoid > 1;
+	if (sysctl_perf_event_paranoid > -1 && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+

Here the sysctl check of > -1 also is now coupled with a CAP_SYS_ADMIN check.
However..

quoted

+	return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);

quoted

 }
 

--- a/kernel/events/core.c
+++ b/kernel/events/core.c

quoted

@@ -5862,14 +5859,8 @@ static int perf_mmap(struct file *file,
 	lock_limit >>= PAGE_SHIFT;
 	locked = atomic64_read(&vma->vm_mm->pinned_vm) + extra;
 
-	if (locked > lock_limit) {
-		if (perf_paranoid_tracepoint_raw() && !capable(CAP_IPC_LOCK)) {
-			ret = -EPERM;
-			goto unlock;
-		}
-
-		ret = security_perf_event_open(&event->attr,
-					       PERF_SECURITY_TRACEPOINT);
+	if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
+		ret = perf_allow_tracepoint(&event->attr);

In previous code, this check did not involve a check for CAP_SYS_ADMIN.

I am Ok with adding the CAP_SYS_ADMIN check as well which does make sense to
me for tracepoint access. But it is still a change in the logic so I wanted
to bring it up.

Let me know any other thoughts and then I'll post a new patch.

Yes, I did notice, I found it weird.

If you have CAP_IPC_LIMIT you should be able to bust mlock memory
limits, so I don't see why we should further relate that to paranoid.

The way I wrote it, we also allow to bust the limit if we have disabled
all paranoid checks. Which makes some sense I suppose.

The original commit is this:

  459ec28ab404 ("perf_counter: Allow mmap if paranoid checks are turned off")