The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
packets into the traces, if the owner of the perf event runs with required
capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
The value of this bit is computed in the arm_spe_event_to_pmscr() function
but the check for capabilities happens in the pmu event init callback i.e
arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
remain consistent for the duration of the perf session.
However, the function arm_spe_event_to_pmscr() may be called later during
the event start callback i.e arm_spe_pmu_start() when the "current" process
is not the owner of the perf session, hence the CX bit setting is currently
not consistent.
One way to fix this, is by caching the required value of the CX bit during
the initialization of the PMU event, so that it remains consistent for the
duration of the session. It uses currently unused 'event->hw.flags' element
to cache perfmon_capable() value, which can be referred during event start
callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
of context packets in the trace as per event owner capabilities.
Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
disabled.
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
Reported-by: German Gomez <[email protected]>
Signed-off-by: Anshuman Khandual <[email protected]>
---
Changes in V3:
- Moved set_spe_event_has_cx() before arm_spe_event_to_pmscr()
- Reinstated perfmon_capable() back in arm_spe_pmu_event_init()
- Dropped BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init()
- Updated the commit message
Changes in V2:
https://lore.kernel.org/all/[email protected]/
- Moved CONFIG_PID_IN_CONTEXTIDR config check inside the helper per Suzuki
- Changed the comment per Suzuki
- Renamed the helpers Per Suzuki
- Added "Fixes: " tag per German
Changes in V1:
https://lore.kernel.org/all/[email protected]/
drivers/perf/arm_spe_pmu.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index db670b265897..b65a7d9640e1 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -39,6 +39,24 @@
#include <asm/mmu.h>
#include <asm/sysreg.h>
+/*
+ * Cache if the event is allowed to trace Context information.
+ * This allows us to perform the check, i.e, perfmon_capable(),
+ * in the context of the event owner, once, during the event_init().
+ */
+#define SPE_PMU_HW_FLAGS_CX BIT(0)
+
+static void set_spe_event_has_cx(struct perf_event *event)
+{
+ if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
+ event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
+}
+
+static bool get_spe_event_has_cx(struct perf_event *event)
+{
+ return !!(event->hw.flags & SPE_PMU_HW_FLAGS_CX);
+}
+
#define ARM_SPE_BUF_PAD_BYTE 0
struct arm_spe_pmu_buf {
@@ -272,7 +290,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
if (!attr->exclude_kernel)
reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
- if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
+ if (get_spe_event_has_cx(event))
reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
return reg;
@@ -709,10 +727,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
!(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
return -EOPNOTSUPP;
+ set_spe_event_has_cx(event);
reg = arm_spe_event_to_pmscr(event);
if (!perfmon_capable() &&
(reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
- BIT(SYS_PMSCR_EL1_CX_SHIFT) |
BIT(SYS_PMSCR_EL1_PCT_SHIFT))))
return -EACCES;
--
2.25.1
On 14/07/2022 07:13, Anshuman Khandual wrote:
> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
> packets into the traces, if the owner of the perf event runs with required
> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>
> The value of this bit is computed in the arm_spe_event_to_pmscr() function
> but the check for capabilities happens in the pmu event init callback i.e
> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
> remain consistent for the duration of the perf session.
>
> However, the function arm_spe_event_to_pmscr() may be called later during
> the event start callback i.e arm_spe_pmu_start() when the "current" process
> is not the owner of the perf session, hence the CX bit setting is currently
> not consistent.
>
> One way to fix this, is by caching the required value of the CX bit during
> the initialization of the PMU event, so that it remains consistent for the
> duration of the session. It uses currently unused 'event->hw.flags' element
> to cache perfmon_capable() value, which can be referred during event start
> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
> of context packets in the trace as per event owner capabilities.
>
> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
> disabled.
>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexey Budankov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
TBH, this is not sufficient. The above commit simply replaced the
capable() check with perfmon_capable() wrapper. The "incorrect check
in the wrong task context" existed since :
d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical
Profiling Extension").
So I would recommend using that for the Fixes tag. And any stable
backports without perfmon_capable() could fallback to using capable()
check, like we do in this patch, from the event_init.
Otherwise,
Reviewed-by: Suzuki K Poulose <[email protected]>
On 7/14/22 16:40, Suzuki K Poulose wrote:
> On 14/07/2022 07:13, Anshuman Khandual wrote:
>> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
>> packets into the traces, if the owner of the perf event runs with required
>> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>>
>> The value of this bit is computed in the arm_spe_event_to_pmscr() function
>> but the check for capabilities happens in the pmu event init callback i.e
>> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
>> remain consistent for the duration of the perf session.
>>
>> However, the function arm_spe_event_to_pmscr() may be called later during
>> the event start callback i.e arm_spe_pmu_start() when the "current" process
>> is not the owner of the perf session, hence the CX bit setting is currently
>> not consistent.
>>
>> One way to fix this, is by caching the required value of the CX bit during
>> the initialization of the PMU event, so that it remains consistent for the
>> duration of the session. It uses currently unused 'event->hw.flags' element
>> to cache perfmon_capable() value, which can be referred during event start
>> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
>> of context packets in the trace as per event owner capabilities.
>>
>> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
>> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
>> disabled.
>>
>> Cc: Will Deacon <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: Alexey Budankov <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
>
> TBH, this is not sufficient. The above commit simply replaced the capable() check with perfmon_capable() wrapper. The "incorrect check
> in the wrong task context" existed since :
>
> d5d9696b0380 ("drivers/perf: Add support for ARMv8.2 Statistical Profiling Extension").
>
> So I would recommend using that for the Fixes tag. And any stable
> backports without perfmon_capable() could fallback to using capable()
> check, like we do in this patch, from the event_init.
Makes sense. I will respin the patch with above "Fixes: " tag unless there
is some other feedback here.
>
> Otherwise,
>
> Reviewed-by: Suzuki K Poulose <[email protected]>
>
On 14/07/2022 07:13, Anshuman Khandual wrote:
> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
> packets into the traces, if the owner of the perf event runs with required
> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>
> The value of this bit is computed in the arm_spe_event_to_pmscr() function
> but the check for capabilities happens in the pmu event init callback i.e
> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
> remain consistent for the duration of the perf session.
>
> However, the function arm_spe_event_to_pmscr() may be called later during
> the event start callback i.e arm_spe_pmu_start() when the "current" process
> is not the owner of the perf session, hence the CX bit setting is currently
> not consistent.
>
> One way to fix this, is by caching the required value of the CX bit during
> the initialization of the PMU event, so that it remains consistent for the
> duration of the session. It uses currently unused 'event->hw.flags' element
> to cache perfmon_capable() value, which can be referred during event start
> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
> of context packets in the trace as per event owner capabilities.
>
> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
> disabled.
>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexey Budankov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
> Reported-by: German Gomez <[email protected]>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> Changes in V3:
>
> - Moved set_spe_event_has_cx() before arm_spe_event_to_pmscr()
> - Reinstated perfmon_capable() back in arm_spe_pmu_event_init()
> - Dropped BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init()
> - Updated the commit message
>
> Changes in V2:
>
> https://lore.kernel.org/all/[email protected]/
>
> - Moved CONFIG_PID_IN_CONTEXTIDR config check inside the helper per Suzuki
> - Changed the comment per Suzuki
> - Renamed the helpers Per Suzuki
> - Added "Fixes: " tag per German
>
> Changes in V1:
>
> https://lore.kernel.org/all/[email protected]/
>
>
> drivers/perf/arm_spe_pmu.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index db670b265897..b65a7d9640e1 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -39,6 +39,24 @@
> #include <asm/mmu.h>
> #include <asm/sysreg.h>
>
> +/*
> + * Cache if the event is allowed to trace Context information.
> + * This allows us to perform the check, i.e, perfmon_capable(),
> + * in the context of the event owner, once, during the event_init().
> + */
> +#define SPE_PMU_HW_FLAGS_CX BIT(0)
> +
> +static void set_spe_event_has_cx(struct perf_event *event)
> +{
> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> + event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
> +}
> +
> +static bool get_spe_event_has_cx(struct perf_event *event)
> +{
> + return !!(event->hw.flags & SPE_PMU_HW_FLAGS_CX);
> +}
> +
> #define ARM_SPE_BUF_PAD_BYTE 0
>
> struct arm_spe_pmu_buf {
> @@ -272,7 +290,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
> if (!attr->exclude_kernel)
> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
>
> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> + if (get_spe_event_has_cx(event))
> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>
> return reg;
> @@ -709,10 +727,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
> return -EOPNOTSUPP;
>
> + set_spe_event_has_cx(event);
> reg = arm_spe_event_to_pmscr(event);
> if (!perfmon_capable() &&
> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
> - BIT(SYS_PMSCR_EL1_CX_SHIFT) |
The first part of the change looks ok, but I'm not sure about this removal here.
Doesn't this mean that if you ask for context data when opening the event
without permission you don't get an error returned any more? It just silently
ignores it.
That changes the semantics of the perf event open call and I don't see why that's
needed to fix the issue about only checking the permissions of the owning process.
At least it seems like a separate unrelated change.
It's also worth noting that the value doesn't need to be cached, and another
one line solution is just to check the permissions of the owning process. This
avoids duplicating something that is already saved, will survive any future
refactors of the permissions system, and doesn't use up space in hw_flags:
if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) &&
(has_capability(event->owner, CAP_PERFMON) || has_capability(event->owner, CAP_SYS_ADMIN)))
{
reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
}
> BIT(SYS_PMSCR_EL1_PCT_SHIFT))))
> return -EACCES;
>
Hi James
On 18/07/2022 10:30, James Clark wrote:
>
>
> On 14/07/2022 07:13, Anshuman Khandual wrote:
>> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
>> packets into the traces, if the owner of the perf event runs with required
>> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>>
>> The value of this bit is computed in the arm_spe_event_to_pmscr() function
>> but the check for capabilities happens in the pmu event init callback i.e
>> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
>> remain consistent for the duration of the perf session.
>>
>> However, the function arm_spe_event_to_pmscr() may be called later during
>> the event start callback i.e arm_spe_pmu_start() when the "current" process
>> is not the owner of the perf session, hence the CX bit setting is currently
>> not consistent.
>>
>> One way to fix this, is by caching the required value of the CX bit during
>> the initialization of the PMU event, so that it remains consistent for the
>> duration of the session. It uses currently unused 'event->hw.flags' element
>> to cache perfmon_capable() value, which can be referred during event start
>> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
>> of context packets in the trace as per event owner capabilities.
>>
>> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
>> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
>> disabled.
>>
>> Cc: Will Deacon <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: Alexey Budankov <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
>> Reported-by: German Gomez <[email protected]>
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> Changes in V3:
>>
>> - Moved set_spe_event_has_cx() before arm_spe_event_to_pmscr()
>> - Reinstated perfmon_capable() back in arm_spe_pmu_event_init()
>> - Dropped BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init()
>> - Updated the commit message
>>
>> Changes in V2:
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> - Moved CONFIG_PID_IN_CONTEXTIDR config check inside the helper per Suzuki
>> - Changed the comment per Suzuki
>> - Renamed the helpers Per Suzuki
>> - Added "Fixes: " tag per German
>>
>> Changes in V1:
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>>
>> drivers/perf/arm_spe_pmu.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index db670b265897..b65a7d9640e1 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -39,6 +39,24 @@
>> #include <asm/mmu.h>
>> #include <asm/sysreg.h>
>>
>> +/*
>> + * Cache if the event is allowed to trace Context information.
>> + * This allows us to perform the check, i.e, perfmon_capable(),
>> + * in the context of the event owner, once, during the event_init().
>> + */
>> +#define SPE_PMU_HW_FLAGS_CX BIT(0)
>> +
>> +static void set_spe_event_has_cx(struct perf_event *event)
>> +{
>> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>> + event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
>> +}
>> +
>> +static bool get_spe_event_has_cx(struct perf_event *event)
>> +{
>> + return !!(event->hw.flags & SPE_PMU_HW_FLAGS_CX);
>> +}
>> +
>> #define ARM_SPE_BUF_PAD_BYTE 0
>>
>> struct arm_spe_pmu_buf {
>> @@ -272,7 +290,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
>> if (!attr->exclude_kernel)
>> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
>>
>> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>> + if (get_spe_event_has_cx(event))
>> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>>
>> return reg;
>> @@ -709,10 +727,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
>> return -EOPNOTSUPP;
>>
>> + set_spe_event_has_cx(event);
>> reg = arm_spe_event_to_pmscr(event);
>> if (!perfmon_capable() &&
>> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
>> - BIT(SYS_PMSCR_EL1_CX_SHIFT) |
>
> The first part of the change looks ok, but I'm not sure about this removal here.
>
> Doesn't this mean that if you ask for context data when opening the event
> without permission you don't get an error returned any more? It just silently
> ignores it.
How do you ask for context data with SPE ? If there was a way, we don't
need this caching. The CX bit is set unconditionally on
perfmon_capable() and is not controlled by an attribute. Ideally it is
better to switch to an attribute. But given that it was never there,
I wonder if this would be a problem for the existing perf users ?
>
> That changes the semantics of the perf event open call and I don't see why that's
> needed to fix the issue about only checking the permissions of the owning process.
> At least it seems like a separate unrelated change.
>
> It's also worth noting that the value doesn't need to be cached, and another
> one line solution is just to check the permissions of the owning process. This
> avoids duplicating something that is already saved, will survive any future
> refactors of the permissions system, and doesn't use up space in hw_flags:
>
> if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) &&
> (has_capability(event->owner, CAP_PERFMON) || has_capability(event->owner, CAP_SYS_ADMIN)))
> {
> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
> }
We don't use any bits in the hw_events for SPE. So using a bit for
storing something doesn't seem to be a wasted effort. Any future
refactors to the permission system would need to take care of the
current users. So that argument is not valid in either case.
Cheers
Suzuki
On 18/07/2022 10:42, Suzuki K Poulose wrote:
> Hi James
>
> On 18/07/2022 10:30, James Clark wrote:
>>
>>
>> On 14/07/2022 07:13, Anshuman Khandual wrote:
>>> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
>>> packets into the traces, if the owner of the perf event runs with required
>>> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>>>
>>> The value of this bit is computed in the arm_spe_event_to_pmscr() function
>>> but the check for capabilities happens in the pmu event init callback i.e
>>> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
>>> remain consistent for the duration of the perf session.
>>>
>>> However, the function arm_spe_event_to_pmscr() may be called later during
>>> the event start callback i.e arm_spe_pmu_start() when the "current" process
>>> is not the owner of the perf session, hence the CX bit setting is currently
>>> not consistent.
>>>
>>> One way to fix this, is by caching the required value of the CX bit during
>>> the initialization of the PMU event, so that it remains consistent for the
>>> duration of the session. It uses currently unused 'event->hw.flags' element
>>> to cache perfmon_capable() value, which can be referred during event start
>>> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
>>> of context packets in the trace as per event owner capabilities.
>>>
>>> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
>>> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
>>> disabled.
>>>
>>> Cc: Will Deacon <[email protected]>
>>> Cc: Mark Rutland <[email protected]>
>>> Cc: Alexey Budankov <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
>>> Reported-by: German Gomez <[email protected]>
>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>> ---
>>> Changes in V3:
>>>
>>> - Moved set_spe_event_has_cx() before arm_spe_event_to_pmscr()
>>> - Reinstated perfmon_capable() back in arm_spe_pmu_event_init()
>>> - Dropped BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init()
>>> - Updated the commit message
>>> Changes in V2:
>>>
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>> - Moved CONFIG_PID_IN_CONTEXTIDR config check inside the helper per Suzuki
>>> - Changed the comment per Suzuki
>>> - Renamed the helpers Per Suzuki
>>> - Added "Fixes: " tag per German
>>>
>>> Changes in V1:
>>>
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>>
>>> drivers/perf/arm_spe_pmu.c | 22 ++++++++++++++++++++--
>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>>> index db670b265897..b65a7d9640e1 100644
>>> --- a/drivers/perf/arm_spe_pmu.c
>>> +++ b/drivers/perf/arm_spe_pmu.c
>>> @@ -39,6 +39,24 @@
>>> #include <asm/mmu.h>
>>> #include <asm/sysreg.h>
>>> +/*
>>> + * Cache if the event is allowed to trace Context information.
>>> + * This allows us to perform the check, i.e, perfmon_capable(),
>>> + * in the context of the event owner, once, during the event_init().
>>> + */
>>> +#define SPE_PMU_HW_FLAGS_CX BIT(0)
>>> +
>>> +static void set_spe_event_has_cx(struct perf_event *event)
>>> +{
>>> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>>> + event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
>>> +}
>>> +
>>> +static bool get_spe_event_has_cx(struct perf_event *event)
>>> +{
>>> + return !!(event->hw.flags & SPE_PMU_HW_FLAGS_CX);
>>> +}
>>> +
>>> #define ARM_SPE_BUF_PAD_BYTE 0
>>> struct arm_spe_pmu_buf {
>>> @@ -272,7 +290,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
>>> if (!attr->exclude_kernel)
>>> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
>>> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>>> + if (get_spe_event_has_cx(event))
>>> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>>> return reg;
>>> @@ -709,10 +727,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>>> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
>>> return -EOPNOTSUPP;
>>> + set_spe_event_has_cx(event);
>>> reg = arm_spe_event_to_pmscr(event);
>>> if (!perfmon_capable() &&
>>> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
>>> - BIT(SYS_PMSCR_EL1_CX_SHIFT) |
>>
>> The first part of the change looks ok, but I'm not sure about this removal here.
>>
>> Doesn't this mean that if you ask for context data when opening the event
>> without permission you don't get an error returned any more? It just silently
>> ignores it.
>
> How do you ask for context data with SPE ? If there was a way, we don't
> need this caching. The CX bit is set unconditionally on perfmon_capable() and is not controlled by an attribute. Ideally it is
> better to switch to an attribute. But given that it was never there,
> I wonder if this would be a problem for the existing perf users ?
Oh yes sorry I thought one of those lines was checking the bit from the user
request, but you are right it's unconditional. So this point should be dropped.
I don't think it's actually a problem currently.
>
>
>>
>> That changes the semantics of the perf event open call and I don't see why that's
>> needed to fix the issue about only checking the permissions of the owning process.
>> At least it seems like a separate unrelated change.
>>
>> It's also worth noting that the value doesn't need to be cached, and another
>> one line solution is just to check the permissions of the owning process. This
>> avoids duplicating something that is already saved, will survive any future
>> refactors of the permissions system, and doesn't use up space in hw_flags:
>>
>> if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) &&
>> (has_capability(event->owner, CAP_PERFMON) || has_capability(event->owner, CAP_SYS_ADMIN)))
>> {
>> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>> }
>
> We don't use any bits in the hw_events for SPE. So using a bit for storing something doesn't seem to be a wasted effort. Any future
> refactors to the permission system would need to take care of the
> current users. So that argument is not valid in either case.
I'm just thinking that if you can get something from existing data without
saving something new, and do it in fewer lines, then it's more readable.
Maybe the refactor argument is less strong. Either way, with my previous
point dropped the patch is functionally the same to my suggestion so I
don't have any strong feelings about this one.
>
> Cheers
> Suzuki
On 18/07/2022 10:47, James Clark wrote:
>
>
> On 18/07/2022 10:42, Suzuki K Poulose wrote:
>> Hi James
>>
>> On 18/07/2022 10:30, James Clark wrote:
>>>
>>>
>>> On 14/07/2022 07:13, Anshuman Khandual wrote:
>>>> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
>>>> packets into the traces, if the owner of the perf event runs with required
>>>> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>>>>
>>>> The value of this bit is computed in the arm_spe_event_to_pmscr() function
>>>> but the check for capabilities happens in the pmu event init callback i.e
>>>> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
>>>> remain consistent for the duration of the perf session.
>>>>
>>>> However, the function arm_spe_event_to_pmscr() may be called later during
>>>> the event start callback i.e arm_spe_pmu_start() when the "current" process
>>>> is not the owner of the perf session, hence the CX bit setting is currently
>>>> not consistent.
>>>>
>>>> One way to fix this, is by caching the required value of the CX bit during
>>>> the initialization of the PMU event, so that it remains consistent for the
>>>> duration of the session. It uses currently unused 'event->hw.flags' element
>>>> to cache perfmon_capable() value, which can be referred during event start
>>>> callback to compute SYS_PMSCR_EL1.CX. This ensures consistent availability
>>>> of context packets in the trace as per event owner capabilities.
>>>>
>>>> Drop BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init(), because
>>>> now CX bit cannot be set in arm_spe_event_to_pmscr() with perfmon_capable()
>>>> disabled.
>>>>
>>>> Cc: Will Deacon <[email protected]>
>>>> Cc: Mark Rutland <[email protected]>
>>>> Cc: Alexey Budankov <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Fixes: cea7d0d4a59b ("drivers/perf: Open access for CAP_PERFMON privileged process")
>>>> Reported-by: German Gomez <[email protected]>
>>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>>> ---
>>>> Changes in V3:
>>>>
>>>> - Moved set_spe_event_has_cx() before arm_spe_event_to_pmscr()
>>>> - Reinstated perfmon_capable() back in arm_spe_pmu_event_init()
>>>> - Dropped BIT(SYS_PMSCR_EL1_CX_SHIFT) check in arm_spe_pmu_event_init()
>>>> - Updated the commit message
>>>> Changes in V2:
>>>>
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>> - Moved CONFIG_PID_IN_CONTEXTIDR config check inside the helper per Suzuki
>>>> - Changed the comment per Suzuki
>>>> - Renamed the helpers Per Suzuki
>>>> - Added "Fixes: " tag per German
>>>>
>>>> Changes in V1:
>>>>
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>>
>>>> drivers/perf/arm_spe_pmu.c | 22 ++++++++++++++++++++--
>>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>>>> index db670b265897..b65a7d9640e1 100644
>>>> --- a/drivers/perf/arm_spe_pmu.c
>>>> +++ b/drivers/perf/arm_spe_pmu.c
>>>> @@ -39,6 +39,24 @@
>>>> #include <asm/mmu.h>
>>>> #include <asm/sysreg.h>
>>>> +/*
>>>> + * Cache if the event is allowed to trace Context information.
>>>> + * This allows us to perform the check, i.e, perfmon_capable(),
>>>> + * in the context of the event owner, once, during the event_init().
>>>> + */
>>>> +#define SPE_PMU_HW_FLAGS_CX BIT(0)
>>>> +
>>>> +static void set_spe_event_has_cx(struct perf_event *event)
>>>> +{
>>>> + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>>>> + event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
>>>> +}
>>>> +
>>>> +static bool get_spe_event_has_cx(struct perf_event *event)
>>>> +{
>>>> + return !!(event->hw.flags & SPE_PMU_HW_FLAGS_CX);
>>>> +}
>>>> +
>>>> #define ARM_SPE_BUF_PAD_BYTE 0
>>>> struct arm_spe_pmu_buf {
>>>> @@ -272,7 +290,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
>>>> if (!attr->exclude_kernel)
>>>> reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
>>>> - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
>>>> + if (get_spe_event_has_cx(event))
>>>> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>>>> return reg;
>>>> @@ -709,10 +727,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>>>> !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
>>>> return -EOPNOTSUPP;
>>>> + set_spe_event_has_cx(event);
>>>> reg = arm_spe_event_to_pmscr(event);
>>>> if (!perfmon_capable() &&
>>>> (reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
>>>> - BIT(SYS_PMSCR_EL1_CX_SHIFT) |
>>>
>>> The first part of the change looks ok, but I'm not sure about this removal here.
>>>
>>> Doesn't this mean that if you ask for context data when opening the event
>>> without permission you don't get an error returned any more? It just silently
>>> ignores it.
>>
>> How do you ask for context data with SPE ? If there was a way, we don't
>> need this caching. The CX bit is set unconditionally on perfmon_capable() and is not controlled by an attribute. Ideally it is
>> better to switch to an attribute. But given that it was never there,
>> I wonder if this would be a problem for the existing perf users ?
>
> Oh yes sorry I thought one of those lines was checking the bit from the user
> request, but you are right it's unconditional. So this point should be dropped.
>
> I don't think it's actually a problem currently.
>>
>>
>>>
>>> That changes the semantics of the perf event open call and I don't see why that's
>>> needed to fix the issue about only checking the permissions of the owning process.
>>> At least it seems like a separate unrelated change.
>>>
>>> It's also worth noting that the value doesn't need to be cached, and another
>>> one line solution is just to check the permissions of the owning process. This
>>> avoids duplicating something that is already saved, will survive any future
>>> refactors of the permissions system, and doesn't use up space in hw_flags:
>>>
>>> if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) &&
>>> (has_capability(event->owner, CAP_PERFMON) || has_capability(event->owner, CAP_SYS_ADMIN)))
>>> {
>>> reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
>>> }
>>
>> We don't use any bits in the hw_events for SPE. So using a bit for storing something doesn't seem to be a wasted effort. Any future
>> refactors to the permission system would need to take care of the
>> current users. So that argument is not valid in either case.
>
> I'm just thinking that if you can get something from existing data without
> saving something new, and do it in fewer lines, then it's more readable.
True. On the otherside, you don't have to repeat this operation,
every single time the event is scheduled. I would also goto the
next level and cache the "PMSCR" configuration for a given event
in to the hw_event, which is what the hw_event is for. i.e, storing
hw specific configuration of the event for the PMU. But this is for a
later series.
Cheers
Suzuki
>
> Maybe the refactor argument is less strong. Either way, with my previous
> point dropped the patch is functionally the same to my suggestion so I
> don't have any strong feelings about this one.
>
>>
>> Cheers
>> Suzuki
On Thu, 14 Jul 2022 11:43:02 +0530, Anshuman Khandual wrote:
> The arm_spe_pmu driver will enable SYS_PMSCR_EL1.CX in order to add CONTEXT
> packets into the traces, if the owner of the perf event runs with required
> capabilities i.e CAP_PERFMON or CAP_SYS_ADMIN via perfmon_capable() helper.
>
> The value of this bit is computed in the arm_spe_event_to_pmscr() function
> but the check for capabilities happens in the pmu event init callback i.e
> arm_spe_pmu_event_init(). This suggests that the value of the CX bit should
> remain consistent for the duration of the perf session.
>
> [...]
Applied to will (for-next/perf), thanks!
[1/1] drivers/perf: arm_spe: Fix consistency of SYS_PMSCR_EL1.CX
https://git.kernel.org/will/c/92f2b8bafa3d
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev