| Message ID | 20220117124432.3119132-3-german.gomez@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: arm_spe: Fix consistency of CONTEXT packets in SPE driver | expand |
On 17/01/2022 12:44, German Gomez wrote: > Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode. > This is no less permissive than the existing behavior for the following > reason: > > If perf_event_paranoid <= 0, then non perfmon_capable() users can open > a per-CPU event. With a per-CPU event, unpriviledged users are allowed > to profile _all_ processes, even ones owned by root. > > Without this change, users could see kernel addresses, root processes, > etc, but not gather the PIDs of those processes. The PID is probably the > least sensitive of all the information. > > It would be more idiomatic to check the perf_event_paranoid level with > perf_allow_cpu(), but this function is not exported so cannot be used > from a module. Looking for cpu != -1 is the indirect way of checking > the same thing as it could never get to arm_spe_pmu_event_init() without Reconsidering this bit (comment below) > perf_event_paranoid <= 0. > > Co-authored-by: James Clark <james.clark@arm.com> > Signed-off-by: German Gomez <german.gomez@arm.com> > --- > drivers/perf/arm_spe_pmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index 8515bf85c..7d9a7fa4f 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -711,7 +711,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT)) > return -EOPNOTSUPP; > > - spe_pmu->pmscr_cx = perfmon_capable(); > + spe_pmu->pmscr_cx = perfmon_capable() || (event->cpu != -1); The perf_event_open(2) manpage states: pid == -1 and cpu >= 0 This measures all processes/threads on the specified CPU. This requires CAP_PERFMON (since Linux 5.8) or CAP_SYS_ADMIN capability or a /proc/sys/kernel/perf_event_paranoid value of less than 1. So perhaps it's more accurate to (still implicitly) check the paranoid level with "pid == -1 && event->cpu > 0" ? If so, I think I have to dig deeper into perf_event. I don't immediately see the pid argument. Any hints? Thanks, German > reg = arm_spe_event_to_pmscr(event); > if (!perfmon_capable() && > (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
On Mon, Jan 17, 2022 at 12:44:32PM +0000, German Gomez wrote: > Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode. > This is no less permissive than the existing behavior for the following > reason: > > If perf_event_paranoid <= 0, then non perfmon_capable() users can open > a per-CPU event. With a per-CPU event, unpriviledged users are allowed > to profile _all_ processes, even ones owned by root. > > Without this change, users could see kernel addresses, root processes, > etc, but not gather the PIDs of those processes. The PID is probably the > least sensitive of all the information. > > It would be more idiomatic to check the perf_event_paranoid level with > perf_allow_cpu(), but this function is not exported so cannot be used > from a module. Looking for cpu != -1 is the indirect way of checking > the same thing as it could never get to arm_spe_pmu_event_init() without > perf_event_paranoid <= 0. perf_allow_cpu() is a static inline so there's no need to export it. What's missing? Will
On 18/01/2022 09:52, Will Deacon wrote: > On Mon, Jan 17, 2022 at 12:44:32PM +0000, German Gomez wrote: >> Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode. >> This is no less permissive than the existing behavior for the following >> reason: >> >> If perf_event_paranoid <= 0, then non perfmon_capable() users can open >> a per-CPU event. With a per-CPU event, unpriviledged users are allowed >> to profile _all_ processes, even ones owned by root. >> >> Without this change, users could see kernel addresses, root processes, >> etc, but not gather the PIDs of those processes. The PID is probably the >> least sensitive of all the information. >> >> It would be more idiomatic to check the perf_event_paranoid level with >> perf_allow_cpu(), but this function is not exported so cannot be used >> from a module. Looking for cpu != -1 is the indirect way of checking >> the same thing as it could never get to arm_spe_pmu_event_init() without >> perf_event_paranoid <= 0. > perf_allow_cpu() is a static inline so there's no need to export it. What's > missing? We were still running into build errors: ERROR: modpost: "security_perf_event_open" [drivers/perf/arm_spe_pmu.ko] undefined! ERROR: modpost: "sysctl_perf_event_paranoid" [drivers/perf/arm_spe_pmu.ko] undefined > > Will
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c index 8515bf85c..7d9a7fa4f 100644 --- a/drivers/perf/arm_spe_pmu.c +++ b/drivers/perf/arm_spe_pmu.c @@ -711,7 +711,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event) !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT)) return -EOPNOTSUPP; - spe_pmu->pmscr_cx = perfmon_capable(); + spe_pmu->pmscr_cx = perfmon_capable() || (event->cpu != -1); reg = arm_spe_event_to_pmscr(event); if (!perfmon_capable() && (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode. This is no less permissive than the existing behavior for the following reason: If perf_event_paranoid <= 0, then non perfmon_capable() users can open a per-CPU event. With a per-CPU event, unpriviledged users are allowed to profile _all_ processes, even ones owned by root. Without this change, users could see kernel addresses, root processes, etc, but not gather the PIDs of those processes. The PID is probably the least sensitive of all the information. It would be more idiomatic to check the perf_event_paranoid level with perf_allow_cpu(), but this function is not exported so cannot be used from a module. Looking for cpu != -1 is the indirect way of checking the same thing as it could never get to arm_spe_pmu_event_init() without perf_event_paranoid <= 0. Co-authored-by: James Clark <james.clark@arm.com> Signed-off-by: German Gomez <german.gomez@arm.com> --- drivers/perf/arm_spe_pmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)