[PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Anshuman Khandual]

sysctl_perf_event_paranoid can have values from [-1, 0, 1, 2] which decides
on perf event restrictions for unprivileged users. But using them directly
makes it difficult to correlate exact restriction level they might impose.
This just adds macros for those numerical restriction values, making them
clear and improving readability.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-perf-users@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 include/linux/perf_event.h | 22 ++++++++++++++++++----
 kernel/events/core.c       |  9 +--------
 kernel/kallsyms.c          |  3 ++-
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index da759560eec5..78156b9154df 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1359,14 +1359,28 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
 #define PERF_SECURITY_KERNEL		2
 #define PERF_SECURITY_TRACEPOINT	3
 
+/*
+ * perf event paranoia level:
+ *  -1 - not paranoid at all
+ *   0 - disallow raw tracepoint access for unpriv
+ *   1 - disallow cpu events for unpriv
+ *   2 - disallow kernel profiling for unpriv
+ */
+enum {
+	PERF_EVENT_DISALLOW_NONE	= -1,
+	PERF_EVENT_DISALLOW_TRACE,
+	PERF_EVENT_DISALLOW_CPU,
+	PERF_EVENT_DISALLOW_KERNEL
+};
+
 static inline int perf_is_paranoid(void)
 {
-	return sysctl_perf_event_paranoid > -1;
+	return sysctl_perf_event_paranoid > PERF_EVENT_DISALLOW_NONE;
 }
 
 static inline int perf_allow_kernel(struct perf_event_attr *attr)
 {
-	if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
+	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_KERNEL && !perfmon_capable())
 		return -EACCES;
 
 	return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
@@ -1374,7 +1388,7 @@ static inline int perf_allow_kernel(struct perf_event_attr *attr)
 
 static inline int perf_allow_cpu(struct perf_event_attr *attr)
 {
-	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
+	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_CPU && !perfmon_capable())
 		return -EACCES;
 
 	return security_perf_event_open(attr, PERF_SECURITY_CPU);
@@ -1382,7 +1396,7 @@ static inline int perf_allow_cpu(struct perf_event_attr *attr)
 
 static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
 {
-	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
+	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_TRACE && !perfmon_capable())
 		return -EPERM;
 
 	return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 80782cddb1da..6fdfdc731bab 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -408,14 +408,7 @@ static struct srcu_struct pmus_srcu;
 static cpumask_var_t perf_online_mask;
 static struct kmem_cache *perf_event_cache;
 
-/*
- * perf event paranoia level:
- *  -1 - not paranoid at all
- *   0 - disallow raw tracepoint access for unpriv
- *   1 - disallow cpu events for unpriv
- *   2 - disallow kernel profiling for unpriv
- */
-int sysctl_perf_event_paranoid __read_mostly = 2;
+int sysctl_perf_event_paranoid __read_mostly = PERF_EVENT_DISALLOW_KERNEL;
 
 /* Minimum for 512 kiB + 1 user control page */
 int sysctl_perf_event_mlock __read_mostly = 512 + (PAGE_SIZE / 1024); /* 'free' kiB per user */
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index fbdf8d3279ac..705f7d7d81dc 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 #include <linux/kdb.h>
 #include <linux/err.h>
+#include <linux/perf_event.h>
 #include <linux/proc_fs.h>
 #include <linux/sched.h>	/* for cond_resched */
 #include <linux/ctype.h>
@@ -803,7 +804,7 @@ static inline int kallsyms_for_perf(void)
 {
 #ifdef CONFIG_PERF_EVENTS
 	extern int sysctl_perf_event_paranoid;
-	if (sysctl_perf_event_paranoid <= 1)
+	if (sysctl_perf_event_paranoid <= PERF_EVENT_DISALLOW_CPU)
 		return 1;
 #endif
 	return 0;
-- 
2.20.1

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[James Clark]

On 01/07/2022 07:39, Anshuman Khandual wrote:
> sysctl_perf_event_paranoid can have values from [-1, 0, 1, 2] which decides
> on perf event restrictions for unprivileged users. But using them directly
> makes it difficult to correlate exact restriction level they might impose.
> This just adds macros for those numerical restriction values, making them
> clear and improving readability.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-perf-users@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  include/linux/perf_event.h | 22 ++++++++++++++++++----
>  kernel/events/core.c       |  9 +--------
>  kernel/kallsyms.c          |  3 ++-
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index da759560eec5..78156b9154df 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1359,14 +1359,28 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>  #define PERF_SECURITY_KERNEL		2
>  #define PERF_SECURITY_TRACEPOINT	3
>  
> +/*
> + * perf event paranoia level:
> + *  -1 - not paranoid at all
> + *   0 - disallow raw tracepoint access for unpriv
> + *   1 - disallow cpu events for unpriv
> + *   2 - disallow kernel profiling for unpriv
> + */
> +enum {
> +	PERF_EVENT_DISALLOW_NONE	= -1,
> +	PERF_EVENT_DISALLOW_TRACE,
> +	PERF_EVENT_DISALLOW_CPU,
> +	PERF_EVENT_DISALLOW_KERNEL
> +};
> +
>  static inline int perf_is_paranoid(void)
>  {
> -	return sysctl_perf_event_paranoid > -1;
> +	return sysctl_perf_event_paranoid > PERF_EVENT_DISALLOW_NONE;
>  }
>  

Hi Anshuman,

There are quite a few other instances of integers left in the tools code.
If you search for perf_event_paranoid_check() and perf_event_paranoid()
you will find them.

I'm also wondering if it makes sense to return your new enum from all of
the helper functions instead of an int and make it explicit that it's
an instance of this new type? Although the compiler doesn't seem to warn
about using integers so maybe it's not worth doing this.

James

>  static inline int perf_allow_kernel(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_KERNEL && !perfmon_capable())
>  		return -EACCES;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
> @@ -1374,7 +1388,7 @@ static inline int perf_allow_kernel(struct perf_event_attr *attr)
>  
>  static inline int perf_allow_cpu(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_CPU && !perfmon_capable())
>  		return -EACCES;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_CPU);
> @@ -1382,7 +1396,7 @@ static inline int perf_allow_cpu(struct perf_event_attr *attr)
>  
>  static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
>  {
> -	if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
> +	if (sysctl_perf_event_paranoid >= PERF_EVENT_DISALLOW_TRACE && !perfmon_capable())
>  		return -EPERM;
>  
>  	return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 80782cddb1da..6fdfdc731bab 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -408,14 +408,7 @@ static struct srcu_struct pmus_srcu;
>  static cpumask_var_t perf_online_mask;
>  static struct kmem_cache *perf_event_cache;
>  
> -/*
> - * perf event paranoia level:
> - *  -1 - not paranoid at all
> - *   0 - disallow raw tracepoint access for unpriv
> - *   1 - disallow cpu events for unpriv
> - *   2 - disallow kernel profiling for unpriv
> - */
> -int sysctl_perf_event_paranoid __read_mostly = 2;
> +int sysctl_perf_event_paranoid __read_mostly = PERF_EVENT_DISALLOW_KERNEL;
>  
>  /* Minimum for 512 kiB + 1 user control page */
>  int sysctl_perf_event_mlock __read_mostly = 512 + (PAGE_SIZE / 1024); /* 'free' kiB per user */
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fbdf8d3279ac..705f7d7d81dc 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -18,6 +18,7 @@
>  #include <linux/fs.h>
>  #include <linux/kdb.h>
>  #include <linux/err.h>
> +#include <linux/perf_event.h>
>  #include <linux/proc_fs.h>
>  #include <linux/sched.h>	/* for cond_resched */
>  #include <linux/ctype.h>
> @@ -803,7 +804,7 @@ static inline int kallsyms_for_perf(void)
>  {
>  #ifdef CONFIG_PERF_EVENTS
>  	extern int sysctl_perf_event_paranoid;
> -	if (sysctl_perf_event_paranoid <= 1)
> +	if (sysctl_perf_event_paranoid <= PERF_EVENT_DISALLOW_CPU)
>  		return 1;
>  #endif
>  	return 0;

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Peter Zijlstra]

On Fri, Jul 08, 2022 at 10:10:15AM +0100, James Clark wrote:
> 
> 
> On 01/07/2022 07:39, Anshuman Khandual wrote:
> > sysctl_perf_event_paranoid can have values from [-1, 0, 1, 2] which decides
> > on perf event restrictions for unprivileged users. But using them directly
> > makes it difficult to correlate exact restriction level they might impose.
> > This just adds macros for those numerical restriction values, making them
> > clear and improving readability.
> > 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: linux-perf-users@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > ---
> >  include/linux/perf_event.h | 22 ++++++++++++++++++----
> >  kernel/events/core.c       |  9 +--------
> >  kernel/kallsyms.c          |  3 ++-
> >  3 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index da759560eec5..78156b9154df 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -1359,14 +1359,28 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
> >  #define PERF_SECURITY_KERNEL		2
> >  #define PERF_SECURITY_TRACEPOINT	3
> >  
> > +/*
> > + * perf event paranoia level:
> > + *  -1 - not paranoid at all
> > + *   0 - disallow raw tracepoint access for unpriv
> > + *   1 - disallow cpu events for unpriv
> > + *   2 - disallow kernel profiling for unpriv
> > + */
> > +enum {
> > +	PERF_EVENT_DISALLOW_NONE	= -1,
> > +	PERF_EVENT_DISALLOW_TRACE,
> > +	PERF_EVENT_DISALLOW_CPU,
> > +	PERF_EVENT_DISALLOW_KERNEL
> > +};
> > +
> >  static inline int perf_is_paranoid(void)
> >  {
> > -	return sysctl_perf_event_paranoid > -1;
> > +	return sysctl_perf_event_paranoid > PERF_EVENT_DISALLOW_NONE;
> >  }
> >  
> 
> Hi Anshuman,
> 
> There are quite a few other instances of integers left in the tools code.
> If you search for perf_event_paranoid_check() and perf_event_paranoid()
> you will find them.
> 
> I'm also wondering if it makes sense to return your new enum from all of
> the helper functions instead of an int and make it explicit that it's
> an instance of this new type? Although the compiler doesn't seem to warn
> about using integers so maybe it's not worth doing this.

so I don't see the point of all this; it's already wrapped in these
helper functions that have a descriptive name, why do we need more muck
on top?

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Anshuman Khandual]

On 7/8/22 19:01, Peter Zijlstra wrote:
> On Fri, Jul 08, 2022 at 10:10:15AM +0100, James Clark wrote:
>>
>>
>> On 01/07/2022 07:39, Anshuman Khandual wrote:
>>> sysctl_perf_event_paranoid can have values from [-1, 0, 1, 2] which decides
>>> on perf event restrictions for unprivileged users. But using them directly
>>> makes it difficult to correlate exact restriction level they might impose.
>>> This just adds macros for those numerical restriction values, making them
>>> clear and improving readability.
>>>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: linux-perf-users@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>  include/linux/perf_event.h | 22 ++++++++++++++++++----
>>>  kernel/events/core.c       |  9 +--------
>>>  kernel/kallsyms.c          |  3 ++-
>>>  3 files changed, 21 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>>> index da759560eec5..78156b9154df 100644
>>> --- a/include/linux/perf_event.h
>>> +++ b/include/linux/perf_event.h
>>> @@ -1359,14 +1359,28 @@ int perf_event_max_stack_handler(struct ctl_table *table, int write,
>>>  #define PERF_SECURITY_KERNEL		2
>>>  #define PERF_SECURITY_TRACEPOINT	3
>>>  
>>> +/*
>>> + * perf event paranoia level:
>>> + *  -1 - not paranoid at all
>>> + *   0 - disallow raw tracepoint access for unpriv
>>> + *   1 - disallow cpu events for unpriv
>>> + *   2 - disallow kernel profiling for unpriv
>>> + */
>>> +enum {
>>> +	PERF_EVENT_DISALLOW_NONE	= -1,
>>> +	PERF_EVENT_DISALLOW_TRACE,
>>> +	PERF_EVENT_DISALLOW_CPU,
>>> +	PERF_EVENT_DISALLOW_KERNEL
>>> +};
>>> +
>>>  static inline int perf_is_paranoid(void)
>>>  {
>>> -	return sysctl_perf_event_paranoid > -1;
>>> +	return sysctl_perf_event_paranoid > PERF_EVENT_DISALLOW_NONE;
>>>  }
>>>  
>>
>> Hi Anshuman,
>>
>> There are quite a few other instances of integers left in the tools code.
>> If you search for perf_event_paranoid_check() and perf_event_paranoid()
>> you will find them.
>>
>> I'm also wondering if it makes sense to return your new enum from all of
>> the helper functions instead of an int and make it explicit that it's
>> an instance of this new type? Although the compiler doesn't seem to warn
>> about using integers so maybe it's not worth doing this.
> 
> so I don't see the point of all this; it's already wrapped in these
> helper functions that have a descriptive name, why do we need more muck
> on top?

Enumerating [-1, 0, 1, 2] paranoid range values in kernel too, does not add
much value as well ?

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Peter Zijlstra]

On Mon, Jul 11, 2022 at 02:55:12PM +0530, Anshuman Khandual wrote:
> Enumerating [-1, 0, 1, 2] paranoid range values in kernel too, does not add
> much value as well ?

That's what the user-interface requires as well. How is obscuring the
values the user has to explicitly poke in help things?

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Ian Rogers]

On Mon, Jul 11, 2022 at 5:19 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Jul 11, 2022 at 02:55:12PM +0530, Anshuman Khandual wrote:
> > Enumerating [-1, 0, 1, 2] paranoid range values in kernel too, does not add
> > much value as well ?
>
> That's what the user-interface requires as well. How is obscuring the
> values the user has to explicitly poke in help things?

Perhaps a helper function like ParanoidAndNotRoot that takes the
failing paranoia level such as here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/shell/stat+csv_output.sh?h=perf/core#n48
would help clean this up. Perhaps something that explicitly calls out
this is a permissions check. Cleaning up permission checking seems
like useful value add. We've been trying to do something similar in
tests to make them skip rather than fail due to permission issues -
this should increase the signal perf test gives.

Thanks,
Ian

Re: [PATCH] perf/core: Add macros for possible sysctl_perf_event_paranoid values

[Anshuman Khandual]

On 7/11/22 17:49, Peter Zijlstra wrote:
> On Mon, Jul 11, 2022 at 02:55:12PM +0530, Anshuman Khandual wrote:
>> Enumerating [-1, 0, 1, 2] paranoid range values in kernel too, does not add
>> much value as well ?
> 
> That's what the user-interface requires as well. How is obscuring the
> values the user has to explicitly poke in help things?

Right, users still have the write raw numbers into the sysfs file.