Re: [PATCH V7 2/2] perf/x86: Reset the dirty counter to prevent the leak for an RDPMC task

[Rob Herring]

On Thu, May 13, 2021 at 5:14 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 5/13/2021 11:02 AM, Peter Zijlstra wrote:
> > On Thu, May 13, 2021 at 07:23:02AM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
> >
> >> +    if (x86_pmu.sched_task && event->hw.target) {
> >> +            atomic_inc(&event->pmu->sched_cb_usage);
> >> +            local_irq_save(flags);
> >> +            x86_pmu_clear_dirty_counters();
> >> +            local_irq_restore(flags);
> >> +    }
> >
> > So what happens if our mmap() happens after we've already created two
> > (or more) threads in the process, all of who already have a counter (or
> > more) on?
> >
> > Shouldn't this be something like?
>
> That's not enough.
>
> I implemented a test case as below:
> - The main thread A creates a new thread B.
> - Bind the thread A to CPU 0. Then the thread A opens a event, mmap,
> enable the event, and sleep.
> - Bind the thread B to CPU 1. Wait until the event in the thread A is
> enabled. Then RDPMC can read the counters on CPU 1.
>
> In the x86_pmu_event_mapped(), we do on_each_cpu_mask(mm_cpumask(mm),
> cr4_update_pce, NULL, 1);
> The RDPMC from thread B on CPU 1 is not forbidden.

You want RDPMC disabled since the counters are not cleared? If you had
a cpu bound event for CPU1, then you'd want RDPMC enabled, right?

> Since the counter is not created in thread B, the sched_task() never
> gets a chance to be invoked. The dirty counter is not cleared.
>
> To fix it, I think we have to move the cr4_update_pce() to the context
> switch, and update it only when the RDPMC task is scheduled. But it
> probably brings some overhead.

I'm trying to do a similar approach (if I understand what you mean)
using sched_task() without a switch_mm hook or IPIs. The current
branch is here[1]. I have things working for task bound events, but I
don't think cpu bound events are handled for similar reasons as above.
I'm not too sure that enabling user access for cpu bound events is
really all that useful? Maybe for Arm we should just keep user access
for cpu bound events disabled.

Note for now I'm not doing lazy clearing of counters for simplicity.

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
user-perf-event-v8