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

["Liang, Kan" <kan.liang@linux.intel.com>]

On 5/14/2021 10:44 AM, Peter Zijlstra wrote:
> On Thu, May 13, 2021 at 06:14:08PM -0400, Liang, Kan 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.
> 
> This?

Yes

> 
> 	A				B
> 
> 	clone(CLONE_THREAD) --->	
> 	set_affine(0)
> 					set_affine(1)
> 					while (!event-enabled)
> 						;
> 	event = perf_event_open()
> 	mmap(event)
> 	ioctl(event, IOC_ENABLE); --->
> 					RDPMC
> 
> 	sleep(n)
> 	  schedule(INTERRUPTIBLE)
> 	  /* idle */
> 
> 
>> 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.
>> 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.
> 
> Per-task counters from CPU1 that ran before B ran?

Yes

> 
>> 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.
> 
> We have CR4:PCE updates in the context switch path, see
> switch_mm_irqs_off() -> cr4_update_pce_mm().
> 
> Doing the clear there might actually make sense and avoids this frobbing
> of ->sched_task(). When we call cr4_update_pce_mm(), and @mm has rdpmc
> on, clear dirty or something like that.
> 
> Worth a try.
> 
> 

Looks like a good place. Will try.

Thanks,
Kan