lkml.org 
[lkml]   [2021]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V7 2/2] perf/x86: Reset the dirty counter to prevent the leak for an RDPMC task
From
Date


On 5/13/2021 11:50 PM, Rob Herring wrote:
> On Thu, May 13, 2021 at 5:14 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 5/13/2021 11:02 AM, Peter Zijlstra wrote:
>>> On Thu, May 13, 2021 at 07:23:02AM -0700, kan.liang@linux.intel.com 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 we are trying to use a lazy way to clear the counters, I think the
RDPMC should be enabled only for a user who owns the counters.

For the above case, we only perf_event_open(pid = 0, cpu = -1) an event
in the thread A. Perf should only monitor the thread A. The RDPMC should
be enabled only when the thread A is scheduled in.
The thread B doesn't open any events. The RDPMC should be disabled for
the thread B. Otherwise, it can read any counters on the CPU, including
other task-bound events, which is what the patchset intends to prevent.


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

If we don't use the lazy way, I think we can clear the counters when a
task is scheduled out. I don't think we need to worry about the above case.

Thanks,
Kan

\
 
 \ /
  Last update: 2021-05-14 15:50    [W:0.057 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site