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

On 18/01/2022 14:04, German Gomez wrote:

Hi Will,

Many thanks for your comments

On 18/01/2022 10:07, Will Deacon wrote:

quoted

On Mon, Jan 17, 2022 at 12:44:31PM +0000, German Gomez wrote:

quoted

[...]

  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?

quoted

quoted

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).

Just to expand a bit more on this (I missed grepping the other directories)

./arch/powerpc/perf/imc-pmu.c => perfmon_capable() only called on init
./kernel/events/core.c        => perfmon_capable() only called on init
./arch/x86/events/core.c      => perf_allow_*() function only called on init

As far as I see, currently spe seems to be the only PMU event that
checks capabilities in the start callback. 'perf' may not be the current
task in this callback.

quoted

quoted

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

quoted

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel