Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX

[German Gomez <german.gomez@arm.com>]

Hi Will,

Many thanks for your comments

On 18/01/2022 10:07, Will Deacon wrote:
> On Mon, Jan 17, 2022 at 12:44:31PM +0000, German Gomez wrote:
>> [...]
>>
>>   1. Run a process in the background with capability CAP_SYS_ADMIN in CPU0.
>>
>>     $ taskset --cpu-list 0 sudo dd if=/dev/random of=/dev/null &
>>     [3] 3806
>>
>>   2. Begin a perf session _without_ capabilities (we shouldn't see CONTEXT packets).
>>
>>     $ perf record -e arm_spe_0// -C0 -- sleep 1
>>     $ perf report -D | grep CONTEXT
>>     .  0000000e:  65 df 0e 00 00                                  CONTEXT 0xedf el2
>>     .  0000004e:  65 df 0e 00 00                                  CONTEXT 0xedf el2
>>     .  0000008e:  65 df 0e 00 00                                  CONTEXT 0xedf el2
>>     [...]
>>
>> As can be seen, the traces begin showing CONTEXT packets when the pid is
>> 0xedf (3807).
> So to be clear: we shouldn't be reporting these packets because 'perf'
> doesn't have the right capabilities, but we evaluate that in the context of
> 'dd' (running as root) and so incorrectly grant the permission. Correct?

Yes, correct. My guess was that "perfmon_capable()" was being called
under the assumption that it would always be evaluated in the context of
'perf'. Is that correct?

>
>> This happens because the pmu start callback is run when
>> the current process is not the owner of the perf session, so the CX
>> register bit is set.
> This doesn't really seem SPE-specific to me -- the perf_allow_*() helpers
> also operate implicitly on the current task. How do other PMU drivers avoid
> falling into this trap?

I'm not as familiar with the other PMU drivers. I quickly tried grepping
something related in the cs_etm drivers as they use CONTEXTIDR as well,
but couldn't find references to perfmon_capable() or similar checks.

Grepping for "perf_allow_" inside of drivers doesn't yield results.
There's some gpu driver that has similar perfmon_capable() checks but
unlike spe, they error out if they don't pass (drivers/gpu/drm/i915/i915_perf.c).

>
>> One way to fix this is by caching the value of the CX bit during the
>> initialization of the PMU event, so that it remains consistent for the
>> duration of the session.
> It doesn't feel right to stash this in 'struct arm_spe_pmu' during event
> initialisation -- wouldn't that allow perf to continue creating new events
> with CX set, even if the paranoid sysctl was changed dynamically? Instead,
> I think it would be better if the capabilities were stash in the event
> itself somehow at initialisation time.

I hadn't considered this. Makes more sense to store in the perf_event
or via some type of mapping in the struct spe_pmu if not possible. Do
you have any idea for the former? Or an idiomatic structure from the
kernel for the later?

Thanks,
German

>
> Will