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

Wed, 27 Jun 2018 02:15:38 -0700 


On 26/06/18 18:25, Alexey Budankov wrote:

Hi,

On 26.06.2018 18:36, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin <[email protected]>

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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Madhavan Srinivasan <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
  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",

























					Reply via email to