[RFC 0/4] perf: Per PMU access controls (paranoid setting)

[RFC 0/4] perf: Per PMU access controls (paranoid setting)

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

For situations where sysadmins might want to allow different level of
access control for different PMUs, we start creating per-PMU
perf_event_paranoid controls in sysfs.

These work in equivalent fashion as the existing perf_event_paranoid
sysctl, which now becomes the parent control for each PMU.

On PMU registration the global/parent value will be inherited by each PMU,
as it will be propagated to all registered PMUs when the sysctl is
updated.

At any later point individual PMU access controls, located in
<sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
fine grained access control.

Discussion from previous posting:
https://lkml.org/lkml/2018/5/21/156

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org

Tvrtko Ursulin (4):
  perf: Move some access checks later in perf_event_open
  perf: Pass pmu pointer to perf_paranoid_* helpers
  perf: Allow per PMU access control
  perf Documentation: Document the per PMU perf_event_paranoid interface

 .../sysfs-bus-event_source-devices-events     |  14 +++
 arch/powerpc/perf/core-book3s.c               |   2 +-
 arch/x86/events/intel/bts.c                   |   2 +-
 arch/x86/events/intel/core.c                  |   2 +-
 arch/x86/events/intel/p4.c                    |   2 +-
 include/linux/perf_event.h                    |  18 ++-
 kernel/events/core.c                          | 104 +++++++++++++++---
 kernel/sysctl.c                               |   4 +-
 kernel/trace/trace_event_perf.c               |   6 +-
 9 files changed, 123 insertions(+), 31 deletions(-)

-- 
2.17.1

[RFC 1/4] perf: Move some access checks later in perf_event_open

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

To enable per-PMU access controls in a following patch first move all call
sites of perf_paranoid_kernel() to after the event has been created.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 kernel/events/core.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f490caca9aa4..12de95b0472e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10189,10 +10189,6 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 			 */
 			attr->branch_sample_type = mask;
 		}
-		/* privileged levels capture (kernel, hv): check permissions */
-		if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
-		    && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
-			return -EACCES;
 	}
 
 	if (attr->sample_type & PERF_SAMPLE_REGS_USER) {
@@ -10409,11 +10405,6 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (err)
 		return err;
 
-	if (!attr.exclude_kernel) {
-		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
-			return -EACCES;
-	}
-
 	if (attr.namespaces) {
 		if (!capable(CAP_SYS_ADMIN))
 			return -EACCES;
@@ -10427,11 +10418,6 @@ SYSCALL_DEFINE5(perf_event_open,
 			return -EINVAL;
 	}
 
-	/* Only privileged users can get physical addresses */
-	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
-	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
-		return -EACCES;
-
 	/*
 	 * In cgroup mode, the pid argument is used to pass the fd
 	 * opened to the cgroup directory in cgroupfs. The cpu argument
@@ -10501,6 +10487,28 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_cred;
 	}
 
+	if (!attr.exclude_kernel) {
+		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+			err = -EACCES;
+			goto err_alloc;
+		}
+	}
+
+	/* Only privileged users can get physical addresses */
+	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
+	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+		err = -EACCES;
+		goto err_alloc;
+	}
+
+	/* privileged levels capture (kernel, hv): check permissions */
+	if ((attr.sample_type & PERF_SAMPLE_BRANCH_STACK) &&
+	    (attr.branch_sample_type & PERF_SAMPLE_BRANCH_PERM_PLM) &&
+	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+		err = -EACCES;
+		goto err_alloc;
+	}
+
 	if (is_sampling_event(event)) {
 		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
 			err = -EOPNOTSUPP;
-- 
2.17.1

Re: [RFC 1/4] perf: Move some access checks later in perf_event_open

[Alexey Budankov]

Hi,

On 26.06.2018 18:36, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> To enable per-PMU access controls in a following patch first move all call
> sites of perf_paranoid_kernel() to after the event has been created.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: x86@kernel.org
> ---
>  kernel/events/core.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f490caca9aa4..12de95b0472e 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10189,10 +10189,6 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  			 */
>  			attr->branch_sample_type = mask;
>  		}
> -		/* privileged levels capture (kernel, hv): check permissions */
> -		if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
> -		    && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> -			return -EACCES;
>  	}
>  
>  	if (attr->sample_type & PERF_SAMPLE_REGS_USER) {
> @@ -10409,11 +10405,6 @@ SYSCALL_DEFINE5(perf_event_open,
>  	if (err)
>  		return err;
>  
> -	if (!attr.exclude_kernel) {
> -		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -	}
> -
>  	if (attr.namespaces) {
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EACCES;
> @@ -10427,11 +10418,6 @@ SYSCALL_DEFINE5(perf_event_open,
>  			return -EINVAL;
>  	}
>  
> -	/* Only privileged users can get physical addresses */
> -	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
> -	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
> -		return -EACCES;
> -
>  	/*
>  	 * In cgroup mode, the pid argument is used to pass the fd
>  	 * opened to the cgroup directory in cgroupfs. The cpu argument
> @@ -10501,6 +10487,28 @@ SYSCALL_DEFINE5(perf_event_open,
>  		goto err_cred;
>  	}
>  
> +	if (!attr.exclude_kernel) {
> +		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
> +			err = -EACCES;

I would separate this combined permissions check into a function e.g.
static bool perf_test_pmu_paranoid(const struct pmu *pmu, int *err) to avoid 
code duplication.

> +			goto err_alloc;
> +		}
> +	}
> +
> +	/* Only privileged users can get physical addresses */
> +	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
> +	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
> +		err = -EACCES;
> +		goto err_alloc;
> +	}
> +
> +	/* privileged levels capture (kernel, hv): check permissions */
> +	if ((attr.sample_type & PERF_SAMPLE_BRANCH_STACK) &&
> +	    (attr.branch_sample_type & PERF_SAMPLE_BRANCH_PERM_PLM) &&
> +	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
> +		err = -EACCES;
> +		goto err_alloc;
> +	}
> +
>  	if (is_sampling_event(event)) {
>  		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
>  			err = -EOPNOTSUPP;
> 

Re: [RFC 1/4] perf: Move some access checks later in perf_event_open

[Tvrtko Ursulin]

On 26/06/18 18:24, Alexey Budankov wrote:
> Hi,
> 
> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> To enable per-PMU access controls in a following patch first move all call
>> sites of perf_paranoid_kernel() to after the event has been created.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: x86@kernel.org
>> ---
>>   kernel/events/core.c | 36 ++++++++++++++++++++++--------------
>>   1 file changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index f490caca9aa4..12de95b0472e 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -10189,10 +10189,6 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>>   			 */
>>   			attr->branch_sample_type = mask;
>>   		}
>> -		/* privileged levels capture (kernel, hv): check permissions */
>> -		if ((mask & PERF_SAMPLE_BRANCH_PERM_PLM)
>> -		    && perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> -			return -EACCES;
>>   	}
>>   
>>   	if (attr->sample_type & PERF_SAMPLE_REGS_USER) {
>> @@ -10409,11 +10405,6 @@ SYSCALL_DEFINE5(perf_event_open,
>>   	if (err)
>>   		return err;
>>   
>> -	if (!attr.exclude_kernel) {
>> -		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> -			return -EACCES;
>> -	}
>> -
>>   	if (attr.namespaces) {
>>   		if (!capable(CAP_SYS_ADMIN))
>>   			return -EACCES;
>> @@ -10427,11 +10418,6 @@ SYSCALL_DEFINE5(perf_event_open,
>>   			return -EINVAL;
>>   	}
>>   
>> -	/* Only privileged users can get physical addresses */
>> -	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
>> -	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
>> -		return -EACCES;
>> -
>>   	/*
>>   	 * In cgroup mode, the pid argument is used to pass the fd
>>   	 * opened to the cgroup directory in cgroupfs. The cpu argument
>> @@ -10501,6 +10487,28 @@ SYSCALL_DEFINE5(perf_event_open,
>>   		goto err_cred;
>>   	}
>>   
>> +	if (!attr.exclude_kernel) {
>> +		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
>> +			err = -EACCES;
> 
> I would separate this combined permissions check into a function e.g.
> static bool perf_test_pmu_paranoid(const struct pmu *pmu, int *err) to avoid
> code duplication.

My thinking was for this to be as mechanical (code movement) as 
possible, but I can consider it.

Regards,

Tvrtko

>> +			goto err_alloc;
>> +		}
>> +	}
>> +
>> +	/* Only privileged users can get physical addresses */
>> +	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
>> +	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
>> +		err = -EACCES;
>> +		goto err_alloc;
>> +	}
>> +
>> +	/* privileged levels capture (kernel, hv): check permissions */
>> +	if ((attr.sample_type & PERF_SAMPLE_BRANCH_STACK) &&
>> +	    (attr.branch_sample_type & PERF_SAMPLE_BRANCH_PERM_PLM) &&
>> +	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
>> +		err = -EACCES;
>> +		goto err_alloc;
>> +	}
>> +
>>   	if (is_sampling_event(event)) {
>>   		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
>>   			err = -EOPNOTSUPP;
>>
> 

[RFC 2/4] perf: Pass pmu pointer to perf_paranoid_* helpers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

To enable per-PMU access controls in a following patch we need to start
passing in the PMU object pointer to perf_paranoid_* helpers.

This patch only changes the API across the code base without changing the
behaviour.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 arch/powerpc/perf/core-book3s.c |  2 +-
 arch/x86/events/intel/bts.c     |  2 +-
 arch/x86/events/intel/core.c    |  2 +-
 arch/x86/events/intel/p4.c      |  2 +-
 include/linux/perf_event.h      |  6 +++---
 kernel/events/core.c            | 15 ++++++++-------
 kernel/trace/trace_event_perf.c |  6 ++++--
 7 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 3f66fcf8ad99..ae6716cea308 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -199,7 +199,7 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
 	if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
 		*addrp = mfspr(SPRN_SDAR);
 
-	if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
+	if (perf_paranoid_kernel(ppmu) && !capable(CAP_SYS_ADMIN) &&
 		is_kernel_addr(mfspr(SPRN_SDAR)))
 		*addrp = 0;
 }
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 24ffa1e88cf9..e416c9e2400a 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -555,7 +555,7 @@ static int bts_event_init(struct perf_event *event)
 	 * Note that the default paranoia setting permits unprivileged
 	 * users to profile the kernel.
 	 */
-	if (event->attr.exclude_kernel && perf_paranoid_kernel() &&
+	if (event->attr.exclude_kernel && perf_paranoid_kernel(event->pmu) &&
 	    !capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 707b2a96e516..6b126bdbd16c 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3025,7 +3025,7 @@ static int intel_pmu_hw_config(struct perf_event *event)
 	if (x86_pmu.version < 3)
 		return -EINVAL;
 
-	if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+	if (perf_paranoid_cpu(event->pmu) && !capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
 	event->hw.config |= ARCH_PERFMON_EVENTSEL_ANY;
diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
index d32c0eed38ca..878451ef1ace 100644
--- a/arch/x86/events/intel/p4.c
+++ b/arch/x86/events/intel/p4.c
@@ -776,7 +776,7 @@ static int p4_validate_raw_event(struct perf_event *event)
 	 * the user needs special permissions to be able to use it
 	 */
 	if (p4_ht_active() && p4_event_bind_map[v].shared) {
-		if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_cpu(event->pmu) && !capable(CAP_SYS_ADMIN))
 			return -EACCES;
 	}
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1fa12887ec02..d7938d88c028 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1178,17 +1178,17 @@ extern int perf_cpu_time_max_percent_handler(struct ctl_table *table, int write,
 int perf_event_max_stack_handler(struct ctl_table *table, int write,
 				 void __user *buffer, size_t *lenp, loff_t *ppos);
 
-static inline bool perf_paranoid_tracepoint_raw(void)
+static inline bool perf_paranoid_tracepoint_raw(const struct pmu *pmu)
 {
 	return sysctl_perf_event_paranoid > -1;
 }
 
-static inline bool perf_paranoid_cpu(void)
+static inline bool perf_paranoid_cpu(const struct pmu *pmu)
 {
 	return sysctl_perf_event_paranoid > 0;
 }
 
-static inline bool perf_paranoid_kernel(void)
+static inline bool perf_paranoid_kernel(const struct pmu *pmu)
 {
 	return sysctl_perf_event_paranoid > 1;
 }
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 12de95b0472e..370c89e81722 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4113,7 +4113,7 @@ find_get_context(struct pmu *pmu, struct task_struct *task,
 
 	if (!task) {
 		/* Must be root to operate on a CPU event: */
-		if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_cpu(pmu) && !capable(CAP_SYS_ADMIN))
 			return ERR_PTR(-EACCES);
 
 		cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
@@ -5681,7 +5681,7 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 	lock_limit >>= PAGE_SHIFT;
 	locked = vma->vm_mm->pinned_vm + extra;
 
-	if ((locked > lock_limit) && perf_paranoid_tracepoint_raw() &&
+	if ((locked > lock_limit) && perf_paranoid_tracepoint_raw(event->pmu) &&
 		!capable(CAP_IPC_LOCK)) {
 		ret = -EPERM;
 		goto unlock;
@@ -10487,8 +10487,10 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_cred;
 	}
 
+	pmu = event->pmu;
+
 	if (!attr.exclude_kernel) {
-		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+		if (perf_paranoid_kernel(pmu) && !capable(CAP_SYS_ADMIN)) {
 			err = -EACCES;
 			goto err_alloc;
 		}
@@ -10496,7 +10498,7 @@ SYSCALL_DEFINE5(perf_event_open,
 
 	/* Only privileged users can get physical addresses */
 	if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
-	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+	    perf_paranoid_kernel(pmu) && !capable(CAP_SYS_ADMIN)) {
 		err = -EACCES;
 		goto err_alloc;
 	}
@@ -10504,13 +10506,13 @@ SYSCALL_DEFINE5(perf_event_open,
 	/* privileged levels capture (kernel, hv): check permissions */
 	if ((attr.sample_type & PERF_SAMPLE_BRANCH_STACK) &&
 	    (attr.branch_sample_type & PERF_SAMPLE_BRANCH_PERM_PLM) &&
-	    perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) {
+	    perf_paranoid_kernel(pmu) && !capable(CAP_SYS_ADMIN)) {
 		err = -EACCES;
 		goto err_alloc;
 	}
 
 	if (is_sampling_event(event)) {
-		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
+		if (pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
 			err = -EOPNOTSUPP;
 			goto err_alloc;
 		}
@@ -10520,7 +10522,6 @@ SYSCALL_DEFINE5(perf_event_open,
 	 * Special case software events and allow them to be part of
 	 * any hardware group.
 	 */
-	pmu = event->pmu;
 
 	if (attr.use_clockid) {
 		err = perf_event_set_clock(event, attr.clockid);
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index c79193e598f5..545a7ef9bfe1 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -45,7 +45,8 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 
 	/* The ftrace function trace is allowed only for root. */
 	if (ftrace_event_is_function(tp_event)) {
-		if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_tracepoint_raw(p_event->pmu) &&
+		    !capable(CAP_SYS_ADMIN))
 			return -EPERM;
 
 		if (!is_sampling_event(p_event))
@@ -81,7 +82,8 @@ static int perf_trace_event_perm(struct trace_event_call *tp_event,
 	 * ...otherwise raw tracepoint data can be a severe data leak,
 	 * only allow root to have these.
 	 */
-	if (perf_paranoid_tracepoint_raw() && !capable(CAP_SYS_ADMIN))
+	if (perf_paranoid_tracepoint_raw(p_event->pmu) &&
+	    !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
 	return 0;
-- 
2.17.1

Re: [RFC 2/4] perf: Pass pmu pointer to perf_paranoid_* helpers

[Ravi Bangoria]

Hi Tvrtko,

> @@ -199,7 +199,7 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
>  	if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
>  		*addrp = mfspr(SPRN_SDAR);
>  
> -	if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
> +	if (perf_paranoid_kernel(ppmu) && !capable(CAP_SYS_ADMIN) &&
>  		is_kernel_addr(mfspr(SPRN_SDAR)))
>  		*addrp = 0;
>  }

This patch fails for me on powerpc:

arch/powerpc/perf/core-book3s.c: In function ‘perf_get_data_addr’:
arch/powerpc/perf/core-book3s.c:202:27: error: passing argument 1 of ‘perf_paranoid_kernel’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  if (perf_paranoid_kernel(ppmu) && !capable(CAP_SYS_ADMIN) &&
                           ^~~~
In file included from arch/powerpc/perf/core-book3s.c:13:0:
./include/linux/perf_event.h:1191:20: note: expected ‘const struct pmu *’ but argument is of type ‘struct power_pmu *’
 static inline bool perf_paranoid_kernel(const struct pmu *pmu)
                    ^~~~~~~~~~~~~~~~~~~~
arch/powerpc/perf/core-book3s.c: In function ‘power_pmu_bhrb_read’:
arch/powerpc/perf/core-book3s.c:470:8: error: too few arguments to function ‘perf_paranoid_kernel’
    if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
        ^~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/perf/core-book3s.c:13:0:
./include/linux/perf_event.h:1191:20: note: declared here
 static inline bool perf_paranoid_kernel(const struct pmu *pmu)
                    ^~~~~~~~~~~~~~~~~~~~
  CC      net/ipv6/route.o

Re: [RFC 2/4] perf: Pass pmu pointer to perf_paranoid_* helpers

[Tvrtko Ursulin]

Hi Ravi,

On 03/07/18 11:24, Ravi Bangoria wrote:
> Hi Tvrtko,
> 
>> @@ -199,7 +199,7 @@ static inline void perf_get_data_addr(struct pt_regs *regs, u64 *addrp)
>>   	if (!(mmcra & MMCRA_SAMPLE_ENABLE) || sdar_valid)
>>   		*addrp = mfspr(SPRN_SDAR);
>>   
>> -	if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
>> +	if (perf_paranoid_kernel(ppmu) && !capable(CAP_SYS_ADMIN) &&
>>   		is_kernel_addr(mfspr(SPRN_SDAR)))
>>   		*addrp = 0;
>>   }
> 
> This patch fails for me on powerpc:
> 
> arch/powerpc/perf/core-book3s.c: In function ‘perf_get_data_addr’:
> arch/powerpc/perf/core-book3s.c:202:27: error: passing argument 1 of ‘perf_paranoid_kernel’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>    if (perf_paranoid_kernel(ppmu) && !capable(CAP_SYS_ADMIN) &&
>                             ^~~~
> In file included from arch/powerpc/perf/core-book3s.c:13:0:
> ./include/linux/perf_event.h:1191:20: note: expected ‘const struct pmu *’ but argument is of type ‘struct power_pmu *’
>   static inline bool perf_paranoid_kernel(const struct pmu *pmu)
>                      ^~~~~~~~~~~~~~~~~~~~
> arch/powerpc/perf/core-book3s.c: In function ‘power_pmu_bhrb_read’:
> arch/powerpc/perf/core-book3s.c:470:8: error: too few arguments to function ‘perf_paranoid_kernel’
>      if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN) &&
>          ^~~~~~~~~~~~~~~~~~~~
> In file included from arch/powerpc/perf/core-book3s.c:13:0:
> ./include/linux/perf_event.h:1191:20: note: declared here
>   static inline bool perf_paranoid_kernel(const struct pmu *pmu)
>                      ^~~~~~~~~~~~~~~~~~~~
>    CC      net/ipv6/route.o

Yep, kbuild reported this as well. I will need to re-arrange the code a 
bit to a) pass the correct pointer in, and b) I missed one call-site as 
well.

I was holding of doing that until some more general feedback arrives.

Regards,

Tvrtko

[RFC 3/4] perf: Allow per PMU access control

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

For situations where sysadmins might want to allow different level of
access control for different PMUs, we start creating per-PMU
perf_event_paranoid controls in sysfs.

These work in equivalent fashion as the existing perf_event_paranoid
sysctl, which now becomes the parent control for each PMU.

On PMU registration the global/parent value will be inherited by each PMU,
as it will be propagated to all registered PMUs when the sysctl is
updated.

At any later point individual PMU access controls, located in
<sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
fine grained access control.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 include/linux/perf_event.h | 12 ++++++--
 kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c            |  4 ++-
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d7938d88c028..22e91cc2d9e1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -271,6 +271,9 @@ struct pmu {
 	/* number of address filters this PMU can do */
 	unsigned int			nr_addr_filters;
 
+	/* per PMU access control */
+	int				perf_event_paranoid;
+
 	/*
 	 * Fully disable/enable this PMU, can be used to protect from the PMI
 	 * as well as for lazy/batch writing of the MSRs.
@@ -1168,6 +1171,9 @@ extern int sysctl_perf_cpu_time_max_percent;
 
 extern void perf_sample_event_took(u64 sample_len_ns);
 
+extern int perf_proc_paranoid_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp,
+		loff_t *ppos);
 extern int perf_proc_update_handler(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp,
 		loff_t *ppos);
@@ -1180,17 +1186,17 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
 
 static inline bool perf_paranoid_tracepoint_raw(const struct pmu *pmu)
 {
-	return sysctl_perf_event_paranoid > -1;
+	return pmu->perf_event_paranoid > -1;
 }
 
 static inline bool perf_paranoid_cpu(const struct pmu *pmu)
 {
-	return sysctl_perf_event_paranoid > 0;
+	return pmu->perf_event_paranoid > 0;
 }
 
 static inline bool perf_paranoid_kernel(const struct pmu *pmu)
 {
-	return sysctl_perf_event_paranoid > 1;
+	return pmu->perf_event_paranoid > 1;
 }
 
 extern void perf_event_init(void);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 370c89e81722..da36317dc8dc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -432,6 +432,24 @@ static void update_perf_cpu_limits(void)
 
 static bool perf_rotate_context(struct perf_cpu_context *cpuctx);
 
+int perf_proc_paranoid_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp,
+		loff_t *ppos)
+{
+	int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+	struct pmu *pmu;
+
+	if (ret || !write)
+		return ret;
+
+	mutex_lock(&pmus_lock);
+	list_for_each_entry(pmu, &pmus, entry)
+		pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
+	mutex_unlock(&pmus_lock);
+
+	return 0;
+}
+
 int perf_proc_update_handler(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp,
 		loff_t *ppos)
@@ -9425,6 +9443,41 @@ static void free_pmu_context(struct pmu *pmu)
 	mutex_unlock(&pmus_lock);
 }
 
+/*
+ * Fine-grained access control:
+ */
+static ssize_t
+perf_event_paranoid_show(struct device *dev,
+			 struct device_attribute *attr,
+			 char *page)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+
+	return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->perf_event_paranoid);
+}
+
+static ssize_t
+perf_event_paranoid_store(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+	int ret, val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val < -1 || val > 2)
+		return -EINVAL;
+
+	pmu->perf_event_paranoid = val;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(perf_event_paranoid);
+
 /*
  * Let userspace know that this PMU supports address range filtering:
  */
@@ -9539,6 +9592,11 @@ static int pmu_dev_alloc(struct pmu *pmu)
 	if (ret)
 		goto free_dev;
 
+	/* Add fine-grained access control attribute. */
+	ret = device_create_file(pmu->dev, &dev_attr_perf_event_paranoid);
+	if (ret)
+		goto del_dev;
+
 	/* For PMUs with address filters, throw in an extra attribute: */
 	if (pmu->nr_addr_filters)
 		ret = device_create_file(pmu->dev, &dev_attr_nr_addr_filters);
@@ -9570,6 +9628,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 	if (!pmu->pmu_disable_count)
 		goto unlock;
 
+	pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
 	pmu->type = -1;
 	if (!name)
 		goto skip_type;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 2d9837c0aff4..7f6fccb64a30 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1142,7 +1142,9 @@ static struct ctl_table kern_table[] = {
 		.data		= &sysctl_perf_event_paranoid,
 		.maxlen		= sizeof(sysctl_perf_event_paranoid),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler   = perf_proc_paranoid_handler,
+		.extra1         = &neg_one,
+		.extra2         = &two,
 	},
 	{
 		.procname	= "perf_event_mlock_kb",
-- 
2.17.1

Re: [RFC 3/4] perf: Allow per PMU access control

[Alexey Budankov]

Hi,

On 26.06.2018 18:36, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> For situations where sysadmins might want to allow different level of
> access control for different PMUs, we start creating per-PMU
> perf_event_paranoid controls in sysfs.
> 
> These work in equivalent fashion as the existing perf_event_paranoid
> sysctl, which now becomes the parent control for each PMU.
> 
> On PMU registration the global/parent value will be inherited by each PMU,
> as it will be propagated to all registered PMUs when the sysctl is
> updated.
> 
> At any later point individual PMU access controls, located in
> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
> fine grained access control.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: x86@kernel.org
> ---
>  include/linux/perf_event.h | 12 ++++++--
>  kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
>  kernel/sysctl.c            |  4 ++-
>  3 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d7938d88c028..22e91cc2d9e1 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -271,6 +271,9 @@ struct pmu {
>  	/* number of address filters this PMU can do */
>  	unsigned int			nr_addr_filters;
>  
> +	/* per PMU access control */
> +	int				perf_event_paranoid;

It looks like it needs to be declared as atomic and atomic_read/atomic_write 
operations need to be explicitly used below in the patch as far this 
variable may be manipulated by different threads at the same time 
without explicit locking.

> +
>  	/*
>  	 * Fully disable/enable this PMU, can be used to protect from the PMI
>  	 * as well as for lazy/batch writing of the MSRs.
> @@ -1168,6 +1171,9 @@ extern int sysctl_perf_cpu_time_max_percent;
>  
>  extern void perf_sample_event_took(u64 sample_len_ns);
>  
> +extern int perf_proc_paranoid_handler(struct ctl_table *table, int write,
> +		void __user *buffer, size_t *lenp,
> +		loff_t *ppos);
>  extern int perf_proc_update_handler(struct ctl_table *table, int write,
>  		void __user *buffer, size_t *lenp,
>  		loff_t *ppos);
> @@ -1180,17 +1186,17 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>  
>  static inline bool perf_paranoid_tracepoint_raw(const struct pmu *pmu)
>  {
> -	return sysctl_perf_event_paranoid > -1;
> +	return pmu->perf_event_paranoid > -1;
>  }
>  
>  static inline bool perf_paranoid_cpu(const struct pmu *pmu)
>  {
> -	return sysctl_perf_event_paranoid > 0;
> +	return pmu->perf_event_paranoid > 0;
>  }
>  
>  static inline bool perf_paranoid_kernel(const struct pmu *pmu)
>  {
> -	return sysctl_perf_event_paranoid > 1;
> +	return pmu->perf_event_paranoid > 1;
>  }
>  
>  extern void perf_event_init(void);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 370c89e81722..da36317dc8dc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -432,6 +432,24 @@ static void update_perf_cpu_limits(void)
>  
>  static bool perf_rotate_context(struct perf_cpu_context *cpuctx);
>  
> +int perf_proc_paranoid_handler(struct ctl_table *table, int write,
> +		void __user *buffer, size_t *lenp,
> +		loff_t *ppos)
> +{
> +	int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +	struct pmu *pmu;
> +
> +	if (ret || !write)
> +		return ret;
> +
> +	mutex_lock(&pmus_lock);
> +	list_for_each_entry(pmu, &pmus, entry)
> +		pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
> +	mutex_unlock(&pmus_lock);
> +
> +	return 0;
> +}
> +
>  int perf_proc_update_handler(struct ctl_table *table, int write,
>  		void __user *buffer, size_t *lenp,
>  		loff_t *ppos)
> @@ -9425,6 +9443,41 @@ static void free_pmu_context(struct pmu *pmu)
>  	mutex_unlock(&pmus_lock);
>  }
>  
> +/*
> + * Fine-grained access control:
> + */
> +static ssize_t
> +perf_event_paranoid_show(struct device *dev,
> +			 struct device_attribute *attr,
> +			 char *page)
> +{
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +
> +	return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->perf_event_paranoid);
> +}
> +
> +static ssize_t
> +perf_event_paranoid_store(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	int ret, val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val < -1 || val > 2)
> +		return -EINVAL;
> +
> +	pmu->perf_event_paranoid = val;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(perf_event_paranoid);
> +
>  /*
>   * Let userspace know that this PMU supports address range filtering:
>   */
> @@ -9539,6 +9592,11 @@ static int pmu_dev_alloc(struct pmu *pmu)
>  	if (ret)
>  		goto free_dev;
>  
> +	/* Add fine-grained access control attribute. */
> +	ret = device_create_file(pmu->dev, &dev_attr_perf_event_paranoid);
> +	if (ret)
> +		goto del_dev;
> +
>  	/* For PMUs with address filters, throw in an extra attribute: */
>  	if (pmu->nr_addr_filters)
>  		ret = device_create_file(pmu->dev, &dev_attr_nr_addr_filters);
> @@ -9570,6 +9628,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
>  	if (!pmu->pmu_disable_count)
>  		goto unlock;
>  
> +	pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
>  	pmu->type = -1;
>  	if (!name)
>  		goto skip_type;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 2d9837c0aff4..7f6fccb64a30 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1142,7 +1142,9 @@ static struct ctl_table kern_table[] = {
>  		.data		= &sysctl_perf_event_paranoid,
>  		.maxlen		= sizeof(sysctl_perf_event_paranoid),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler   = perf_proc_paranoid_handler,
> +		.extra1         = &neg_one,
> +		.extra2         = &two,
>  	},
>  	{
>  		.procname	= "perf_event_mlock_kb",
> 

Re: [RFC 3/4] perf: Allow per PMU access control

[Tvrtko Ursulin]

On 26/06/18 18:25, Alexey Budankov wrote:
> Hi,
> 
> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> For situations where sysadmins might want to allow different level of
>> access control for different PMUs, we start creating per-PMU
>> perf_event_paranoid controls in sysfs.
>>
>> These work in equivalent fashion as the existing perf_event_paranoid
>> sysctl, which now becomes the parent control for each PMU.
>>
>> On PMU registration the global/parent value will be inherited by each PMU,
>> as it will be propagated to all registered PMUs when the sysctl is
>> updated.
>>
>> At any later point individual PMU access controls, located in
>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>> fine grained access control.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: x86@kernel.org
>> ---
>>   include/linux/perf_event.h | 12 ++++++--
>>   kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
>>   kernel/sysctl.c            |  4 ++-
>>   3 files changed, 71 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index d7938d88c028..22e91cc2d9e1 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -271,6 +271,9 @@ struct pmu {
>>   	/* number of address filters this PMU can do */
>>   	unsigned int			nr_addr_filters;
>>   
>> +	/* per PMU access control */
>> +	int				perf_event_paranoid;
> 
> It looks like it needs to be declared as atomic and atomic_read/atomic_write
> operations need to be explicitly used below in the patch as far this
> variable may be manipulated by different threads at the same time
> without explicit locking.

It is just a write of an integer from either sysfs access or sysctl. As 
such I don't think going atomic would change anything. There is no RMW 
or increment or anything on it.

Unless there are architectures where int stores are not atomic? But then 
the existing sysctl would have the same issue. So I suspect we can 
indeed rely on int store being atomic.

Regards,

Tvrtko

> 
>> +
>>   	/*
>>   	 * Fully disable/enable this PMU, can be used to protect from the PMI
>>   	 * as well as for lazy/batch writing of the MSRs.
>> @@ -1168,6 +1171,9 @@ extern int sysctl_perf_cpu_time_max_percent;
>>   
>>   extern void perf_sample_event_took(u64 sample_len_ns);
>>   
>> +extern int perf_proc_paranoid_handler(struct ctl_table *table, int write,
>> +		void __user *buffer, size_t *lenp,
>> +		loff_t *ppos);
>>   extern int perf_proc_update_handler(struct ctl_table *table, int write,
>>   		void __user *buffer, size_t *lenp,
>>   		loff_t *ppos);
>> @@ -1180,17 +1186,17 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>>   
>>   static inline bool perf_paranoid_tracepoint_raw(const struct pmu *pmu)
>>   {
>> -	return sysctl_perf_event_paranoid > -1;
>> +	return pmu->perf_event_paranoid > -1;
>>   }
>>   
>>   static inline bool perf_paranoid_cpu(const struct pmu *pmu)
>>   {
>> -	return sysctl_perf_event_paranoid > 0;
>> +	return pmu->perf_event_paranoid > 0;
>>   }
>>   
>>   static inline bool perf_paranoid_kernel(const struct pmu *pmu)
>>   {
>> -	return sysctl_perf_event_paranoid > 1;
>> +	return pmu->perf_event_paranoid > 1;
>>   }
>>   
>>   extern void perf_event_init(void);
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 370c89e81722..da36317dc8dc 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -432,6 +432,24 @@ static void update_perf_cpu_limits(void)
>>   
>>   static bool perf_rotate_context(struct perf_cpu_context *cpuctx);
>>   
>> +int perf_proc_paranoid_handler(struct ctl_table *table, int write,
>> +		void __user *buffer, size_t *lenp,
>> +		loff_t *ppos)
>> +{
>> +	int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>> +	struct pmu *pmu;
>> +
>> +	if (ret || !write)
>> +		return ret;
>> +
>> +	mutex_lock(&pmus_lock);
>> +	list_for_each_entry(pmu, &pmus, entry)
>> +		pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
>> +	mutex_unlock(&pmus_lock);
>> +
>> +	return 0;
>> +}
>> +
>>   int perf_proc_update_handler(struct ctl_table *table, int write,
>>   		void __user *buffer, size_t *lenp,
>>   		loff_t *ppos)
>> @@ -9425,6 +9443,41 @@ static void free_pmu_context(struct pmu *pmu)
>>   	mutex_unlock(&pmus_lock);
>>   }
>>   
>> +/*
>> + * Fine-grained access control:
>> + */
>> +static ssize_t
>> +perf_event_paranoid_show(struct device *dev,
>> +			 struct device_attribute *attr,
>> +			 char *page)
>> +{
>> +	struct pmu *pmu = dev_get_drvdata(dev);
>> +
>> +	return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->perf_event_paranoid);
>> +}
>> +
>> +static ssize_t
>> +perf_event_paranoid_store(struct device *dev,
>> +			  struct device_attribute *attr,
>> +			  const char *buf, size_t count)
>> +{
>> +	struct pmu *pmu = dev_get_drvdata(dev);
>> +	int ret, val;
>> +
>> +	ret = kstrtoint(buf, 0, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val < -1 || val > 2)
>> +		return -EINVAL;
>> +
>> +	pmu->perf_event_paranoid = val;
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(perf_event_paranoid);
>> +
>>   /*
>>    * Let userspace know that this PMU supports address range filtering:
>>    */
>> @@ -9539,6 +9592,11 @@ static int pmu_dev_alloc(struct pmu *pmu)
>>   	if (ret)
>>   		goto free_dev;
>>   
>> +	/* Add fine-grained access control attribute. */
>> +	ret = device_create_file(pmu->dev, &dev_attr_perf_event_paranoid);
>> +	if (ret)
>> +		goto del_dev;
>> +
>>   	/* For PMUs with address filters, throw in an extra attribute: */
>>   	if (pmu->nr_addr_filters)
>>   		ret = device_create_file(pmu->dev, &dev_attr_nr_addr_filters);
>> @@ -9570,6 +9628,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
>>   	if (!pmu->pmu_disable_count)
>>   		goto unlock;
>>   
>> +	pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
>>   	pmu->type = -1;
>>   	if (!name)
>>   		goto skip_type;
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 2d9837c0aff4..7f6fccb64a30 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1142,7 +1142,9 @@ static struct ctl_table kern_table[] = {
>>   		.data		= &sysctl_perf_event_paranoid,
>>   		.maxlen		= sizeof(sysctl_perf_event_paranoid),
>>   		.mode		= 0644,
>> -		.proc_handler	= proc_dointvec,
>> +		.proc_handler   = perf_proc_paranoid_handler,
>> +		.extra1         = &neg_one,
>> +		.extra2         = &two,
>>   	},
>>   	{
>>   		.procname	= "perf_event_mlock_kb",
>>
> 

Re: [RFC 3/4] perf: Allow per PMU access control

[Alexey Budankov]

On 27.06.2018 12:15, Tvrtko Ursulin wrote:
> 
> On 26/06/18 18:25, Alexey Budankov wrote:
>> Hi,
>>
>> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> For situations where sysadmins might want to allow different level of
>>> access control for different PMUs, we start creating per-PMU
>>> perf_event_paranoid controls in sysfs.
>>>
>>> These work in equivalent fashion as the existing perf_event_paranoid
>>> sysctl, which now becomes the parent control for each PMU.
>>>
>>> On PMU registration the global/parent value will be inherited by each PMU,
>>> as it will be propagated to all registered PMUs when the sysctl is
>>> updated.
>>>
>>> At any later point individual PMU access controls, located in
>>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>>> fine grained access control.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>> Cc: Jiri Olsa <jolsa@redhat.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>> Cc: Andi Kleen <ak@linux.intel.com>
>>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: x86@kernel.org
>>> ---
>>>   include/linux/perf_event.h | 12 ++++++--
>>>   kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
>>>   kernel/sysctl.c            |  4 ++-
>>>   3 files changed, 71 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>> index d7938d88c028..22e91cc2d9e1 100644
>>> --- a/include/linux/perf_event.h
>>> +++ b/include/linux/perf_event.h
>>> @@ -271,6 +271,9 @@ struct pmu {
>>>       /* number of address filters this PMU can do */
>>>       unsigned int            nr_addr_filters;
>>>   +    /* per PMU access control */
>>> +    int                perf_event_paranoid;
>>
>> It looks like it needs to be declared as atomic and atomic_read/atomic_write
>> operations need to be explicitly used below in the patch as far this
>> variable may be manipulated by different threads at the same time
>> without explicit locking.
> 
> It is just a write of an integer from either sysfs access or sysctl. As such I don't think going atomic would change anything. There is no RMW or increment or anything on it.
> 
> Unless there are architectures where int stores are not atomic? But then the existing sysctl would have the same issue. So I suspect we can indeed rely on int store being atomic.

Yep, aligned word read/write is atomic on Intel and there is no runtime issue 
currently, but the implementation itself is multithread and implicitly relies 
on that atomicity so my suggestion is just explicitly code that assumption :).
Also, as you mentioned, that makes the arch independent part of code more portable.

> 
> Regards,
> 
> Tvrtko
> 
>>
>>> +
>>>       /*
>>>        * Fully disable/enable this PMU, can be used to protect from the PMI
>>>        * as well as for lazy/batch writing of the MSRs.
>>> @@ -1168,6 +1171,9 @@ extern int sysctl_perf_cpu_time_max_percent;
>>>     extern void perf_sample_event_took(u64 sample_len_ns);
>>>   +extern int perf_proc_paranoid_handler(struct ctl_table *table, int write,
>>> +        void __user *buffer, size_t *lenp,
>>> +        loff_t *ppos);
>>>   extern int perf_proc_update_handler(struct ctl_table *table, int write,
>>>           void __user *buffer, size_t *lenp,
>>>           loff_t *ppos);
>>> @@ -1180,17 +1186,17 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>>>     static inline bool perf_paranoid_tracepoint_raw(const struct pmu *pmu)
>>>   {
>>> -    return sysctl_perf_event_paranoid > -1;
>>> +    return pmu->perf_event_paranoid > -1;
>>>   }
>>>     static inline bool perf_paranoid_cpu(const struct pmu *pmu)
>>>   {
>>> -    return sysctl_perf_event_paranoid > 0;
>>> +    return pmu->perf_event_paranoid > 0;
>>>   }
>>>     static inline bool perf_paranoid_kernel(const struct pmu *pmu)
>>>   {
>>> -    return sysctl_perf_event_paranoid > 1;
>>> +    return pmu->perf_event_paranoid > 1;
>>>   }
>>>     extern void perf_event_init(void);
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index 370c89e81722..da36317dc8dc 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -432,6 +432,24 @@ static void update_perf_cpu_limits(void)
>>>     static bool perf_rotate_context(struct perf_cpu_context *cpuctx);
>>>   +int perf_proc_paranoid_handler(struct ctl_table *table, int write,
>>> +        void __user *buffer, size_t *lenp,
>>> +        loff_t *ppos)
>>> +{
>>> +    int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>>> +    struct pmu *pmu;
>>> +
>>> +    if (ret || !write)
>>> +        return ret;
>>> +
>>> +    mutex_lock(&pmus_lock);
>>> +    list_for_each_entry(pmu, &pmus, entry)
>>> +        pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
>>> +    mutex_unlock(&pmus_lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int perf_proc_update_handler(struct ctl_table *table, int write,
>>>           void __user *buffer, size_t *lenp,
>>>           loff_t *ppos)
>>> @@ -9425,6 +9443,41 @@ static void free_pmu_context(struct pmu *pmu)
>>>       mutex_unlock(&pmus_lock);
>>>   }
>>>   +/*
>>> + * Fine-grained access control:
>>> + */
>>> +static ssize_t
>>> +perf_event_paranoid_show(struct device *dev,
>>> +             struct device_attribute *attr,
>>> +             char *page)
>>> +{
>>> +    struct pmu *pmu = dev_get_drvdata(dev);
>>> +
>>> +    return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->perf_event_paranoid);
>>> +}
>>> +
>>> +static ssize_t
>>> +perf_event_paranoid_store(struct device *dev,
>>> +              struct device_attribute *attr,
>>> +              const char *buf, size_t count)
>>> +{
>>> +    struct pmu *pmu = dev_get_drvdata(dev);
>>> +    int ret, val;
>>> +
>>> +    ret = kstrtoint(buf, 0, &val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (val < -1 || val > 2)
>>> +        return -EINVAL;
>>> +
>>> +    pmu->perf_event_paranoid = val;
>>> +
>>> +    return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RW(perf_event_paranoid);
>>> +
>>>   /*
>>>    * Let userspace know that this PMU supports address range filtering:
>>>    */
>>> @@ -9539,6 +9592,11 @@ static int pmu_dev_alloc(struct pmu *pmu)
>>>       if (ret)
>>>           goto free_dev;
>>>   +    /* Add fine-grained access control attribute. */
>>> +    ret = device_create_file(pmu->dev, &dev_attr_perf_event_paranoid);
>>> +    if (ret)
>>> +        goto del_dev;
>>> +
>>>       /* For PMUs with address filters, throw in an extra attribute: */
>>>       if (pmu->nr_addr_filters)
>>>           ret = device_create_file(pmu->dev, &dev_attr_nr_addr_filters);
>>> @@ -9570,6 +9628,7 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
>>>       if (!pmu->pmu_disable_count)
>>>           goto unlock;
>>>   +    pmu->perf_event_paranoid = sysctl_perf_event_paranoid;
>>>       pmu->type = -1;
>>>       if (!name)
>>>           goto skip_type;
>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>> index 2d9837c0aff4..7f6fccb64a30 100644
>>> --- a/kernel/sysctl.c
>>> +++ b/kernel/sysctl.c
>>> @@ -1142,7 +1142,9 @@ static struct ctl_table kern_table[] = {
>>>           .data        = &sysctl_perf_event_paranoid,
>>>           .maxlen        = sizeof(sysctl_perf_event_paranoid),
>>>           .mode        = 0644,
>>> -        .proc_handler    = proc_dointvec,
>>> +        .proc_handler   = perf_proc_paranoid_handler,
>>> +        .extra1         = &neg_one,
>>> +        .extra2         = &two,
>>>       },
>>>       {
>>>           .procname    = "perf_event_mlock_kb",
>>>
>>
> 

Re: [RFC 3/4] perf: Allow per PMU access control

[Tvrtko Ursulin]

On 27/06/18 10:47, Alexey Budankov wrote:
> 
> 
> On 27.06.2018 12:15, Tvrtko Ursulin wrote:
>>
>> On 26/06/18 18:25, Alexey Budankov wrote:
>>> Hi,
>>>
>>> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> For situations where sysadmins might want to allow different level of
>>>> access control for different PMUs, we start creating per-PMU
>>>> perf_event_paranoid controls in sysfs.
>>>>
>>>> These work in equivalent fashion as the existing perf_event_paranoid
>>>> sysctl, which now becomes the parent control for each PMU.
>>>>
>>>> On PMU registration the global/parent value will be inherited by each PMU,
>>>> as it will be propagated to all registered PMUs when the sysctl is
>>>> updated.
>>>>
>>>> At any later point individual PMU access controls, located in
>>>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>>>> fine grained access control.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>> Cc: Jiri Olsa <jolsa@redhat.com>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: x86@kernel.org
>>>> ---
>>>>    include/linux/perf_event.h | 12 ++++++--
>>>>    kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
>>>>    kernel/sysctl.c            |  4 ++-
>>>>    3 files changed, 71 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>>> index d7938d88c028..22e91cc2d9e1 100644
>>>> --- a/include/linux/perf_event.h
>>>> +++ b/include/linux/perf_event.h
>>>> @@ -271,6 +271,9 @@ struct pmu {
>>>>        /* number of address filters this PMU can do */
>>>>        unsigned int            nr_addr_filters;
>>>>    +    /* per PMU access control */
>>>> +    int                perf_event_paranoid;
>>>
>>> It looks like it needs to be declared as atomic and atomic_read/atomic_write
>>> operations need to be explicitly used below in the patch as far this
>>> variable may be manipulated by different threads at the same time
>>> without explicit locking.
>>
>> It is just a write of an integer from either sysfs access or sysctl. As such I don't think going atomic would change anything. There is no RMW or increment or anything on it.
>>
>> Unless there are architectures where int stores are not atomic? But then the existing sysctl would have the same issue. So I suspect we can indeed rely on int store being atomic.
> 
> Yep, aligned word read/write is atomic on Intel and there is no runtime issue
> currently, but the implementation itself is multithread and implicitly relies
> on that atomicity so my suggestion is just explicitly code that assumption :).
> Also, as you mentioned, that makes the arch independent part of code more portable.

Perhaps we are not on the same page, but my argument was that the 
current sysctl (or sysctl via proc) has the same issue with multiple 
threads potentially writing to it. As long as the end result is a valid 
value it is not a problem. So I don't see what this patch changes in 
that respect. Different tasks writing either of the sysctl/sysfs values 
race, but end up with valid values everywhere. If we can rely on int 
stores to be atomic on all architectures I don't see an effective 
difference after changing to atomic_t, or even taking the pmu mutex over 
the per PMU sysfs writes. So I don't see value in complicating the code 
with either approach but am happy to be proved wrong if that is the case.

Regards,

Tvrtko

Re: [RFC 3/4] perf: Allow per PMU access control

[Alexey Budankov]

On 27.06.2018 13:05, Tvrtko Ursulin wrote:
> 
> On 27/06/18 10:47, Alexey Budankov wrote:
>>
>>
>> On 27.06.2018 12:15, Tvrtko Ursulin wrote:
>>>
>>> On 26/06/18 18:25, Alexey Budankov wrote:
>>>> Hi,
>>>>
>>>> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> For situations where sysadmins might want to allow different level of
>>>>> access control for different PMUs, we start creating per-PMU
>>>>> perf_event_paranoid controls in sysfs.
>>>>>
>>>>> These work in equivalent fashion as the existing perf_event_paranoid
>>>>> sysctl, which now becomes the parent control for each PMU.
>>>>>
>>>>> On PMU registration the global/parent value will be inherited by each PMU,
>>>>> as it will be propagated to all registered PMUs when the sysctl is
>>>>> updated.
>>>>>
>>>>> At any later point individual PMU access controls, located in
>>>>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>>>>> fine grained access control.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>>> Cc: Jiri Olsa <jolsa@redhat.com>
>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Cc: x86@kernel.org
>>>>> ---
>>>>>    include/linux/perf_event.h | 12 ++++++--
>>>>>    kernel/events/core.c       | 59 ++++++++++++++++++++++++++++++++++++++
>>>>>    kernel/sysctl.c            |  4 ++-
>>>>>    3 files changed, 71 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>>>> index d7938d88c028..22e91cc2d9e1 100644
>>>>> --- a/include/linux/perf_event.h
>>>>> +++ b/include/linux/perf_event.h
>>>>> @@ -271,6 +271,9 @@ struct pmu {
>>>>>        /* number of address filters this PMU can do */
>>>>>        unsigned int            nr_addr_filters;
>>>>>    +    /* per PMU access control */
>>>>> +    int                perf_event_paranoid;
>>>>
>>>> It looks like it needs to be declared as atomic and atomic_read/atomic_write
>>>> operations need to be explicitly used below in the patch as far this
>>>> variable may be manipulated by different threads at the same time
>>>> without explicit locking.
>>>
>>> It is just a write of an integer from either sysfs access or sysctl. As such I don't think going atomic would change anything. There is no RMW or increment or anything on it.
>>>
>>> Unless there are architectures where int stores are not atomic? But then the existing sysctl would have the same issue. So I suspect we can indeed rely on int store being atomic.
>>
>> Yep, aligned word read/write is atomic on Intel and there is no runtime issue
>> currently, but the implementation itself is multithread and implicitly relies
>> on that atomicity so my suggestion is just explicitly code that assumption :).
>> Also, as you mentioned, that makes the arch independent part of code more portable.
> 
> Perhaps we are not on the same page, but my argument was that the current sysctl (or sysctl via proc) has the same issue with multiple threads potentially writing to it. 

Well, yes, currently the issue exists but it could be addressed in the new design.

As long as the end result is a valid value it is not a problem. So I don't see what this patch changes in that respect. Different tasks writing either of the sysctl/sysfs values race, but end up with valid values everywhere. If we can rely on int stores to be atomic on all architectures I don't see an effective difference after changing to atomic_t, or even taking the pmu mutex over the per PMU sysfs writes. So I don't see value in complicating the code with either approach but am happy to be proved wrong if that is the case.
> 
> Regards,
> 
> Tvrtko
> 

[RFC 4/4] perf Documentation: Document the per PMU perf_event_paranoid interface

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Explain behaviour of the new control knob along side the existing perf
event documentation.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 .../testing/sysfs-bus-event_source-devices-events  | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-events b/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
index 505f080d20a1..b3096ae9be6f 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-events
@@ -92,3 +92,17 @@ Description:	Perf event scaling factors
 
 		This is provided to avoid performing floating point arithmetic
 		in the kernel.
+
+What: /sys/bus/event_source/devices/<pmu>/perf_event_paranoid
+Date: 2018/06/26
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:	Per PMU access control file
+
+		This is the per PMU version of the perf_event_paranoid sysctl
+		which allows controlling the access control level for each
+		individual PMU.
+
+		Writes to the sysctl will propagate to all PMU providers.
+
+		For explanation of supported values please consult the sysctl
+		documentation.
-- 
2.17.1

Re: [RFC 0/4] perf: Per PMU access controls (paranoid setting)

[Jiri Olsa]

On Tue, Jun 26, 2018 at 04:36:38PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> For situations where sysadmins might want to allow different level of
> access control for different PMUs, we start creating per-PMU
> perf_event_paranoid controls in sysfs.
> 
> These work in equivalent fashion as the existing perf_event_paranoid
> sysctl, which now becomes the parent control for each PMU.
> 
> On PMU registration the global/parent value will be inherited by each PMU,
> as it will be propagated to all registered PMUs when the sysctl is
> updated.
> 
> At any later point individual PMU access controls, located in
> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
> fine grained access control.
> 
> Discussion from previous posting:
> https://lkml.org/lkml/2018/5/21/156
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: x86@kernel.org
> 
> Tvrtko Ursulin (4):
>   perf: Move some access checks later in perf_event_open
>   perf: Pass pmu pointer to perf_paranoid_* helpers
>   perf: Allow per PMU access control
>   perf Documentation: Document the per PMU perf_event_paranoid interface

I think we'll need some perf tool changes for this

apart from the hint/docs, that checks/display paranoid value,
I think we need some changes in arch related code like below
(untested)

jirka


---
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 2f595cd73da6..a5437f100ab9 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -69,7 +69,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
 	struct perf_evsel *evsel, *cs_etm_evsel = NULL;
 	const struct cpu_map *cpus = evlist->cpus;
-	bool privileged = (geteuid() == 0 || perf_event_paranoid() < 0);
+	bool privileged = (geteuid() == 0 || cs_etm_pmu->paranoid < 0);
 
 	ptr->evlist = evlist;
 	ptr->snapshot_mode = opts->auxtrace_snapshot_mode;
diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index 1120e39c1b00..89d3d27ed8be 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -65,7 +65,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 			container_of(itr, struct arm_spe_recording, itr);
 	struct perf_pmu *arm_spe_pmu = sper->arm_spe_pmu;
 	struct perf_evsel *evsel, *arm_spe_evsel = NULL;
-	bool privileged = geteuid() == 0 || perf_event_paranoid() < 0;
+	bool privileged = geteuid() == 0 || arm_spe_pmu->paranoid < 0;
 	struct perf_evsel *tracking_evsel;
 	int err;
 
diff --git a/tools/perf/arch/x86/util/intel-bts.c b/tools/perf/arch/x86/util/intel-bts.c
index 781df40b2966..c97e6556c8e7 100644
--- a/tools/perf/arch/x86/util/intel-bts.c
+++ b/tools/perf/arch/x86/util/intel-bts.c
@@ -116,7 +116,7 @@ static int intel_bts_recording_options(struct auxtrace_record *itr,
 	struct perf_pmu *intel_bts_pmu = btsr->intel_bts_pmu;
 	struct perf_evsel *evsel, *intel_bts_evsel = NULL;
 	const struct cpu_map *cpus = evlist->cpus;
-	bool privileged = geteuid() == 0 || perf_event_paranoid() < 0;
+	bool privileged = geteuid() == 0 || intel_bts_pmu->paranoid < 0;
 
 	btsr->evlist = evlist;
 	btsr->snapshot_mode = opts->auxtrace_snapshot_mode;
diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index db0ba8caf5a2..ffbe5f7f1c57 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -555,7 +555,7 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
 	bool have_timing_info, need_immediate = false;
 	struct perf_evsel *evsel, *intel_pt_evsel = NULL;
 	const struct cpu_map *cpus = evlist->cpus;
-	bool privileged = geteuid() == 0 || perf_event_paranoid() < 0;
+	bool privileged = geteuid() == 0 || intel_pt_pmu->paranoid < 0;
 	u64 tsc_bit;
 	int err;
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d2fb597c9a8c..f2a91305a8b6 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -450,6 +450,20 @@ static int pmu_type(const char *name, __u32 *type)
 	return ret;
 }
 
+static int pmu_paranoid(const char *name)
+{
+	char path[PATH_MAX];
+	int paranoid;
+
+	snprintf(path, PATH_MAX,
+                 EVENT_SOURCE_DEVICE_PATH "%s/perf_event_paranoid", name);
+
+	if (!sysfs__read_int(EVENT_SOURCE_DEVICE_PATH, &paranoid))
+		return paranoid;
+
+	return perf_event_paranoid();
+}
+
 /* Add all pmus in sysfs to pmu list: */
 static void pmu_read_sysfs(void)
 {
@@ -736,6 +750,7 @@ static struct perf_pmu *pmu_lookup(const char *name)
 	pmu->name = strdup(name);
 	pmu->type = type;
 	pmu->is_uncore = pmu_is_uncore(name);
+	pmu->paranoid = pmu_paranoid(name);
 	pmu_add_cpu_aliases(&aliases, pmu);
 
 	INIT_LIST_HEAD(&pmu->format);
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 76fecec7b3f9..1f00c8bbdb90 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -30,6 +30,7 @@ struct perf_pmu {
 	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
 	struct list_head list;    /* ELEM */
 	int (*set_drv_config)	(struct perf_evsel_config_term *term);
+	int paranoid;
 };
 
 struct perf_pmu_info {

Re: [RFC 0/4] perf: Per PMU access controls (paranoid setting)

[Alexey Budankov]

Hi,

Is there any plans or may be even progress on that so far?

Thanks,
Alexey

On 26.06.2018 18:36, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> For situations where sysadmins might want to allow different level of
> access control for different PMUs, we start creating per-PMU
> perf_event_paranoid controls in sysfs.
> 
> These work in equivalent fashion as the existing perf_event_paranoid
> sysctl, which now becomes the parent control for each PMU.
> 
> On PMU registration the global/parent value will be inherited by each PMU,
> as it will be propagated to all registered PMUs when the sysctl is
> updated.
> 
> At any later point individual PMU access controls, located in
> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
> fine grained access control.
> 
> Discussion from previous posting:
> https://lkml.org/lkml/2018/5/21/156
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: x86@kernel.org
> 
> Tvrtko Ursulin (4):
>   perf: Move some access checks later in perf_event_open
>   perf: Pass pmu pointer to perf_paranoid_* helpers
>   perf: Allow per PMU access control
>   perf Documentation: Document the per PMU perf_event_paranoid interface
> 
>  .../sysfs-bus-event_source-devices-events     |  14 +++
>  arch/powerpc/perf/core-book3s.c               |   2 +-
>  arch/x86/events/intel/bts.c                   |   2 +-
>  arch/x86/events/intel/core.c                  |   2 +-
>  arch/x86/events/intel/p4.c                    |   2 +-
>  include/linux/perf_event.h                    |  18 ++-
>  kernel/events/core.c                          | 104 +++++++++++++++---
>  kernel/sysctl.c                               |   4 +-
>  kernel/trace/trace_event_perf.c               |   6 +-
>  9 files changed, 123 insertions(+), 31 deletions(-)
> 

Re: [RFC 0/4] perf: Per PMU access controls (paranoid setting)

[Tvrtko Ursulin]

On 12/09/18 07:52, Alexey Budankov wrote:
> 
> Hi,
> 
> Is there any plans or may be even progress on that so far?

It's hanging in the back of my mind. AFAIR after last round there was a 
build failure or two to fix on more exotic (to me) hardware, and Jiri 
Olsa provided a tools/perf snippet supporting the feature.

But essentially I haven't done any work on it since due not seeing the 
route to upstream. :( In other words, will someone review it and will 
that r-b make it have a chance of getting into some tree. If I had a 
clear statement from someone with authority in these aspects I would 
progress it, but otherwise it felt like it's not going anywhere.

Regards,

Tvrtko


> Thanks,
> Alexey
> 
> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> For situations where sysadmins might want to allow different level of
>> access control for different PMUs, we start creating per-PMU
>> perf_event_paranoid controls in sysfs.
>>
>> These work in equivalent fashion as the existing perf_event_paranoid
>> sysctl, which now becomes the parent control for each PMU.
>>
>> On PMU registration the global/parent value will be inherited by each PMU,
>> as it will be propagated to all registered PMUs when the sysctl is
>> updated.
>>
>> At any later point individual PMU access controls, located in
>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>> fine grained access control.
>>
>> Discussion from previous posting:
>> https://lkml.org/lkml/2018/5/21/156
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: x86@kernel.org
>>
>> Tvrtko Ursulin (4):
>>    perf: Move some access checks later in perf_event_open
>>    perf: Pass pmu pointer to perf_paranoid_* helpers
>>    perf: Allow per PMU access control
>>    perf Documentation: Document the per PMU perf_event_paranoid interface
>>
>>   .../sysfs-bus-event_source-devices-events     |  14 +++
>>   arch/powerpc/perf/core-book3s.c               |   2 +-
>>   arch/x86/events/intel/bts.c                   |   2 +-
>>   arch/x86/events/intel/core.c                  |   2 +-
>>   arch/x86/events/intel/p4.c                    |   2 +-
>>   include/linux/perf_event.h                    |  18 ++-
>>   kernel/events/core.c                          | 104 +++++++++++++++---
>>   kernel/sysctl.c                               |   4 +-
>>   kernel/trace/trace_event_perf.c               |   6 +-
>>   9 files changed, 123 insertions(+), 31 deletions(-)
>>
> 

Re: [RFC 0/4] perf: Per PMU access controls (paranoid setting)

[Alexey Budankov]

Hi,

On 12.09.2018 11:41, Tvrtko Ursulin wrote:
> 
> On 12/09/18 07:52, Alexey Budankov wrote:
>>
>> Hi,
>>
>> Is there any plans or may be even progress on that so far?
> 
> It's hanging in the back of my mind. AFAIR after last round there was a build failure or two to fix on more exotic (to me) hardware, and Jiri Olsa provided a tools/perf snippet supporting the feature.
> 
> But essentially I haven't done any work on it since due not seeing the route to upstream. :( In other words, will someone review it and will that r-b make it have a chance of getting into some tree. If I had a clear statement from someone with authority in these aspects I would progress it, but otherwise it felt like it's not going anywhere.

Got you, Tvrtko, thanks!

Folks, are there any concerns still remain? design, review, testing?

Please let us known of any required assistance to move it forward.

Thanks,
Alexey

> 
> Regards,
> 
> Tvrtko
> 
> 
>> Thanks,
>> Alexey
>>
>> On 26.06.2018 18:36, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> For situations where sysadmins might want to allow different level of
>>> access control for different PMUs, we start creating per-PMU
>>> perf_event_paranoid controls in sysfs.
>>>
>>> These work in equivalent fashion as the existing perf_event_paranoid
>>> sysctl, which now becomes the parent control for each PMU.
>>>
>>> On PMU registration the global/parent value will be inherited by each PMU,
>>> as it will be propagated to all registered PMUs when the sysctl is
>>> updated.
>>>
>>> At any later point individual PMU access controls, located in
>>> <sysfs>/device/<pmu-name>/perf_event_paranoid, can be adjusted to achieve
>>> fine grained access control.
>>>
>>> Discussion from previous posting:
>>> https://lkml.org/lkml/2018/5/21/156
>>>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>> Cc: Jiri Olsa <jolsa@redhat.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>> Cc: Andi Kleen <ak@linux.intel.com>
>>> Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: x86@kernel.org
>>>
>>> Tvrtko Ursulin (4):
>>>    perf: Move some access checks later in perf_event_open
>>>    perf: Pass pmu pointer to perf_paranoid_* helpers
>>>    perf: Allow per PMU access control
>>>    perf Documentation: Document the per PMU perf_event_paranoid interface
>>>
>>>   .../sysfs-bus-event_source-devices-events     |  14 +++
>>>   arch/powerpc/perf/core-book3s.c               |   2 +-
>>>   arch/x86/events/intel/bts.c                   |   2 +-
>>>   arch/x86/events/intel/core.c                  |   2 +-
>>>   arch/x86/events/intel/p4.c                    |   2 +-
>>>   include/linux/perf_event.h                    |  18 ++-
>>>   kernel/events/core.c                          | 104 +++++++++++++++---
>>>   kernel/sysctl.c                               |   4 +-
>>>   kernel/trace/trace_event_perf.c               |   6 +-
>>>   9 files changed, 123 insertions(+), 31 deletions(-)
>>>
>>
>