From: German Gomez <german.gomez@arm.com>
To: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, mark.rutland@arm.com,
james.clark@arm.com, leo.yan@linaro.org
Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX
Date: Wed, 19 Jan 2022 11:27:50 +0000 [thread overview]
Message-ID: <b417d710-2b45-791b-1707-175dd4398701@arm.com> (raw)
In-Reply-To: <2cb1baab-fd9c-ea20-2a09-4cd60d9d5531@arm.com>
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:
>> 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).
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.
>
>>> 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
next prev parent reply other threads:[~2022-01-19 11:28 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-17 12:44 [RFC PATCH 0/2] perf: arm_spe: Fix consistency of CONTEXT packets in SPE driver German Gomez
2022-01-17 12:44 ` [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX German Gomez
2022-01-18 10:07 ` Will Deacon
2022-01-18 14:04 ` German Gomez
2022-01-19 11:27 ` German Gomez [this message]
2022-01-18 16:28 ` James Clark
2022-02-05 15:39 ` Leo Yan
2022-02-07 12:06 ` German Gomez
2022-02-08 13:00 ` Leo Yan
2022-02-10 17:23 ` German Gomez
2022-02-11 10:45 ` Leo Yan
2022-02-15 14:29 ` German Gomez
2022-02-16 13:22 ` Leo Yan
2022-02-16 15:16 ` German Gomez
2022-01-17 12:44 ` [RFC PATCH 2/2] perf: arm_spe: Enable CONTEXT packets in SPE traces if the profiler runs in CPU mode German Gomez
2022-01-17 14:04 ` German Gomez
2022-01-18 9:52 ` Will Deacon
2022-01-18 14:13 ` German Gomez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b417d710-2b45-791b-1707-175dd4398701@arm.com \
--to=german.gomez@arm.com \
--cc=james.clark@arm.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).