Messages in this thread | | | From | Rob Herring <> | Date | Thu, 8 Apr 2021 13:38:17 -0500 | Subject | Re: [PATCH v6 02/10] arm64: perf: Enable PMU counter direct access for perf event |
| |
On Thu, Apr 8, 2021 at 6:08 AM Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Apr 07, 2021 at 01:44:37PM +0100, Will Deacon wrote: > > [Moving Mark to To: since I'd like his view on this] > > > > On Thu, Apr 01, 2021 at 02:45:21PM -0500, Rob Herring wrote: > > > On Wed, Mar 31, 2021 at 11:01 AM Will Deacon <will@kernel.org> wrote: > > > > > > > > On Tue, Mar 30, 2021 at 12:09:38PM -0500, Rob Herring wrote: > > > > > On Tue, Mar 30, 2021 at 10:31 AM Will Deacon <will@kernel.org> wrote: > > > > > > > > > > > > On Wed, Mar 10, 2021 at 05:08:29PM -0700, Rob Herring wrote: > > > > > > > From: Raphael Gault <raphael.gault@arm.com> > > > > > > > > +static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm) > > > > > > > +{ > > > > > > > + struct arm_pmu *armpmu = to_arm_pmu(event->pmu); > > > > > > > + > > > > > > > + if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR)) > > > > > > > + return; > > > > > > > + > > > > > > > + if (atomic_dec_and_test(&mm->context.pmu_direct_access)) > > > > > > > + on_each_cpu_mask(&armpmu->supported_cpus, refresh_pmuserenr, mm, 1); > > > > > > > > > > > > Given that the pmu_direct_access field is global per-mm, won't this go > > > > > > wrong if multiple PMUs are opened by the same process but only a subset > > > > > > are exposed to EL0? Perhaps pmu_direct_access should be treated as a mask > > > > > > rather than a counter, so that we can 'and' it with the supported_cpus for > > > > > > the PMU we're dealing with. > > > > > > > > > > It needs to be a count to support multiple events on the same PMU. If > > > > > the event is not enabled for EL0, then we'd exit out on the > > > > > ARMPMU_EL0_RD_CNTR check. So I think we're fine. > > > > > > > > I'm still not convinced; pmu_direct_access is shared between PMUs, so > > > > testing the result of atomic_dec_and_test() just doesn't make sense to > > > > me, as another PMU could be playing with the count. > > > > > > How is that a problem? Let's make a concrete example: > > > > > > map PMU1:event2 -> pmu_direct_access = 1 -> enable access > > > map PMU2:event3 -> pmu_direct_access = 2 > > > map PMU1:event4 -> pmu_direct_access = 3 > > > unmap PMU2:event3 -> pmu_direct_access = 2 > > > unmap PMU1:event2 -> pmu_direct_access = 1 > > > unmap PMU1:event4 -> pmu_direct_access = 0 -> disable access > > > > > > The only issue I can see is PMU2 remains enabled for user access until > > > the last unmap. But we're sharing the mm, so who cares? Also, in this > > > scenario it is the user's problem to pin themselves to cores sharing a > > > PMU. If the user doesn't do that, they get to keep the pieces. > > > > I guess I'm just worried about exposing the counters to userspace after > > the PMU driver (or perf core?) thinks that they're no longer exposed in > > case we leak other events. > > IMO that's not practically different from the single-PMU case (i.e. > multi-PMU isn't material, either we have a concern with leaking or we > don't); more on that below. > > While it looks odd to place this on the mm, I don't think it's the end > of the world. > > > However, I'm not sure how this is supposed to work normally: what > > happens if e.g. a privileged user has a per-cpu counter for a kernel > > event while a task has a counter with direct access -- can that task > > read the kernel event out of the PMU registers from userspace? > > Yes -- userspace could go read any counters even though it isn't > supposed to, and could potentially infer information from those. It > won't have access to the config registers or kernel data structures, so > it isn't guaranteed to know what the even is or when it is > context-switched/reprogrammed/etc. > > If we believe that's a problem, then it's difficult to do anything > robust other than denying userspace access entirely, since disabling > userspace access while in use would surprise applications, and denying > privileged events would need some global state that we consult at event > creation time (in addition to being an inversion of privilege). > > IIRC there was some fuss about this a while back on x86; I'll go dig and > see what I can find, unless Peter has a memory...
Maybe this one[1].
Rob
[1] https://lore.kernel.org/lkml/20200730123815.18518-1-kan.liang@linux.intel.com/
| |