| Message ID | 20240807155153.2714025-1-james.clark@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drivers/perf: arm_spe: Use perf_allow_kernel() for permissions | expand |
On Wed, Aug 07, 2024 at 04:51:53PM +0100, James Clark wrote: > For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel() > rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form > of physical address, make it consistent and use perf_allow_kernel() for > SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change. > > This improves consistency and indirectly fixes the following error > message which is misleading because perf_event_paranoid is not taken > into account by perfmon_capable(): > > $ perf record -e arm_spe/pa_enable/ > > Error: > Access to performance monitoring and observability operations is > limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid > setting ... > > Suggested-by: Al Grant <al.grant@arm.com> > Signed-off-by: James Clark <james.clark@linaro.org> > --- > Changes since v1: > > * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid > > drivers/perf/arm_spe_pmu.c | 9 ++++----- > include/linux/perf_event.h | 8 +------- > kernel/events/core.c | 9 +++++++++ > 3 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index 9100d82bfabc..3569050f9cf3 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -41,7 +41,7 @@ > > /* > * Cache if the event is allowed to trace Context information. > - * This allows us to perform the check, i.e, perfmon_capable(), > + * This allows us to perform the check, i.e, perf_allow_kernel(), > * in the context of the event owner, once, during the event_init(). > */ > #define SPE_PMU_HW_FLAGS_CX 0x00001 > @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C > > static void set_spe_event_has_cx(struct perf_event *event) > { > - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) > + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr)) > event->hw.flags |= SPE_PMU_HW_FLAGS_CX; The rationale for this change in the commit message is because other drivers gate PERF_SAMPLE_PHYS_ADDR on perf_allow_kernel(). However, putting the PID in contextidr doesn't seem to have anything to do with that... > } > > @@ -745,9 +745,8 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > > set_spe_event_has_cx(event); > reg = arm_spe_event_to_pmscr(event); > - if (!perfmon_capable() && > - (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))) > - return -EACCES; > + if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)) Similarly here. What does the physical counter have to do with physical address sampling other than sharing the word "physical"? Will
On 16/08/2024 1:45 pm, Will Deacon wrote: > On Wed, Aug 07, 2024 at 04:51:53PM +0100, James Clark wrote: >> For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel() >> rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form >> of physical address, make it consistent and use perf_allow_kernel() for >> SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change. >> >> This improves consistency and indirectly fixes the following error >> message which is misleading because perf_event_paranoid is not taken >> into account by perfmon_capable(): >> >> $ perf record -e arm_spe/pa_enable/ >> >> Error: >> Access to performance monitoring and observability operations is >> limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid >> setting ... >> >> Suggested-by: Al Grant <al.grant@arm.com> >> Signed-off-by: James Clark <james.clark@linaro.org> >> --- >> Changes since v1: >> >> * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid >> >> drivers/perf/arm_spe_pmu.c | 9 ++++----- >> include/linux/perf_event.h | 8 +------- >> kernel/events/core.c | 9 +++++++++ >> 3 files changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c >> index 9100d82bfabc..3569050f9cf3 100644 >> --- a/drivers/perf/arm_spe_pmu.c >> +++ b/drivers/perf/arm_spe_pmu.c >> @@ -41,7 +41,7 @@ >> >> /* >> * Cache if the event is allowed to trace Context information. >> - * This allows us to perform the check, i.e, perfmon_capable(), >> + * This allows us to perform the check, i.e, perf_allow_kernel(), >> * in the context of the event owner, once, during the event_init(). >> */ >> #define SPE_PMU_HW_FLAGS_CX 0x00001 >> @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C >> >> static void set_spe_event_has_cx(struct perf_event *event) >> { >> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) >> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr)) >> event->hw.flags |= SPE_PMU_HW_FLAGS_CX; > > The rationale for this change in the commit message is because other > drivers gate PERF_SAMPLE_PHYS_ADDR on perf_allow_kernel(). However, > putting the PID in contextidr doesn't seem to have anything to do with > that... > That is true, I suppose I was thinking of two reasons to do it this way that I didn't really elaborate on: #1 because context IDs and physical timestamps didn't seem to be any more sensitive than physical addresses, so it wouldn't make sense for them to have a stricter permissions model than addresses. #2 (although this is indirect and not really related to the driver) but Perf will still print the misleading warning when physical timestamps are requested. So some other fix would eventually have to be made for that. I'm not sure if you are objecting to the permissions change for the other two things, or it's just a lack of reasoning in the commit message? IMO if we think the other two can't be changed, I would actually rather drop the change than only target PERF_SAMPLE_PHYS_ADDR. Because that seems like it unnecessarily complicates the permissions and might be quite surprising to a user. And then maybe some attempt of a fix could be made in Perf instead. Although that could be difficult because of the lack of a specific error code from the driver. >> } >> >> @@ -745,9 +745,8 @@ static int arm_spe_pmu_event_init(struct perf_event *event) >> >> set_spe_event_has_cx(event); >> reg = arm_spe_event_to_pmscr(event); >> - if (!perfmon_capable() && >> - (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))) >> - return -EACCES; >> + if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)) > > Similarly here. What does the physical counter have to do with physical > address sampling other than sharing the word "physical"? > > Will
On Fri, Aug 16, 2024 at 02:27:19PM +0100, James Clark wrote: > > > On 16/08/2024 1:45 pm, Will Deacon wrote: > > On Wed, Aug 07, 2024 at 04:51:53PM +0100, James Clark wrote: > > > For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel() > > > rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form > > > of physical address, make it consistent and use perf_allow_kernel() for > > > SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change. > > > > > > This improves consistency and indirectly fixes the following error > > > message which is misleading because perf_event_paranoid is not taken > > > into account by perfmon_capable(): > > > > > > $ perf record -e arm_spe/pa_enable/ > > > > > > Error: > > > Access to performance monitoring and observability operations is > > > limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid > > > setting ... > > > > > > Suggested-by: Al Grant <al.grant@arm.com> > > > Signed-off-by: James Clark <james.clark@linaro.org> > > > --- > > > Changes since v1: > > > > > > * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid > > > > > > drivers/perf/arm_spe_pmu.c | 9 ++++----- > > > include/linux/perf_event.h | 8 +------- > > > kernel/events/core.c | 9 +++++++++ > > > 3 files changed, 14 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > > > index 9100d82bfabc..3569050f9cf3 100644 > > > --- a/drivers/perf/arm_spe_pmu.c > > > +++ b/drivers/perf/arm_spe_pmu.c > > > @@ -41,7 +41,7 @@ > > > /* > > > * Cache if the event is allowed to trace Context information. > > > - * This allows us to perform the check, i.e, perfmon_capable(), > > > + * This allows us to perform the check, i.e, perf_allow_kernel(), > > > * in the context of the event owner, once, during the event_init(). > > > */ > > > #define SPE_PMU_HW_FLAGS_CX 0x00001 > > > @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C > > > static void set_spe_event_has_cx(struct perf_event *event) > > > { > > > - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) > > > + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr)) > > > event->hw.flags |= SPE_PMU_HW_FLAGS_CX; > > > > The rationale for this change in the commit message is because other > > drivers gate PERF_SAMPLE_PHYS_ADDR on perf_allow_kernel(). However, > > putting the PID in contextidr doesn't seem to have anything to do with > > that... > > > > That is true, I suppose I was thinking of two reasons to do it this way that > I didn't really elaborate on: > > #1 because context IDs and physical timestamps didn't seem to be any more > sensitive than physical addresses, so it wouldn't make sense for them to > have a stricter permissions model than addresses. > > #2 (although this is indirect and not really related to the driver) but Perf > will still print the misleading warning when physical timestamps are > requested. So some other fix would eventually have to be made for that. > > I'm not sure if you are objecting to the permissions change for the other > two things, or it's just a lack of reasoning in the commit message? I'm just objecting to the rationale in the commit message not being applicable to some of the changes made in the code. A much better rationale to use perf_allow_kernel() is because of its integration with the LSM hooks. Will
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c index 9100d82bfabc..3569050f9cf3 100644 --- a/drivers/perf/arm_spe_pmu.c +++ b/drivers/perf/arm_spe_pmu.c @@ -41,7 +41,7 @@ /* * Cache if the event is allowed to trace Context information. - * This allows us to perform the check, i.e, perfmon_capable(), + * This allows us to perform the check, i.e, perf_allow_kernel(), * in the context of the event owner, once, during the event_init(). */ #define SPE_PMU_HW_FLAGS_CX 0x00001 @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C static void set_spe_event_has_cx(struct perf_event *event) { - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable()) + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr)) event->hw.flags |= SPE_PMU_HW_FLAGS_CX; } @@ -745,9 +745,8 @@ static int arm_spe_pmu_event_init(struct perf_event *event) set_spe_event_has_cx(event); reg = arm_spe_event_to_pmscr(event); - if (!perfmon_capable() && - (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))) - return -EACCES; + if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)) + return perf_allow_kernel(&event->attr); return 0; } diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 1a8942277dda..e336306b8c08 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1602,13 +1602,7 @@ static inline int perf_is_paranoid(void) return sysctl_perf_event_paranoid > -1; } -static inline int perf_allow_kernel(struct perf_event_attr *attr) -{ - if (sysctl_perf_event_paranoid > 1 && !perfmon_capable()) - return -EACCES; - - return security_perf_event_open(attr, PERF_SECURITY_KERNEL); -} +int perf_allow_kernel(struct perf_event_attr *attr); static inline int perf_allow_cpu(struct perf_event_attr *attr) { diff --git a/kernel/events/core.c b/kernel/events/core.c index aa3450bdc227..ae7d63c0c593 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -13351,6 +13351,15 @@ const struct perf_event_attr *perf_event_attrs(struct perf_event *event) return &event->attr; } +int perf_allow_kernel(struct perf_event_attr *attr) +{ + if (sysctl_perf_event_paranoid > 1 && !perfmon_capable()) + return -EACCES; + + return security_perf_event_open(attr, PERF_SECURITY_KERNEL); +} +EXPORT_SYMBOL_GPL(perf_allow_kernel); + /* * Inherit an event from parent task to child task. *
For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel() rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form of physical address, make it consistent and use perf_allow_kernel() for SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change. This improves consistency and indirectly fixes the following error message which is misleading because perf_event_paranoid is not taken into account by perfmon_capable(): $ perf record -e arm_spe/pa_enable/ Error: Access to performance monitoring and observability operations is limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid setting ... Suggested-by: Al Grant <al.grant@arm.com> Signed-off-by: James Clark <james.clark@linaro.org> --- Changes since v1: * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid drivers/perf/arm_spe_pmu.c | 9 ++++----- include/linux/perf_event.h | 8 +------- kernel/events/core.c | 9 +++++++++ 3 files changed, 14 insertions(+), 12 deletions(-)