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

[Liang, Kan]

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

What do you think?

Thanks,
Kan

> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2474,7 +2474,7 @@ static int x86_pmu_event_init(struct per
>   	return err;
>   }
>   
> -static void x86_pmu_clear_dirty_counters(void)
> +static void x86_pmu_clear_dirty_counters(void *unused)
>   {
>   	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>   	int i;
> @@ -2512,9 +2512,9 @@ static void x86_pmu_event_mapped(struct
>   	 */
>   	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);
> +		on_each_cpu_mask(mm_cpumask(mm),
> +				 x86_pmu_clear_dirty_counters,
> +				 NULL, true);
>   	}
>   
>   	/*
> @@ -2653,7 +2653,7 @@ static void x86_pmu_sched_task(struct pe
>   	 */
>   	if (sched_in && ctx && READ_ONCE(x86_pmu.attr_rdpmc) &&
>   	    current->mm && atomic_read(&current->mm->context.perf_rdpmc_allowed))
> -		x86_pmu_clear_dirty_counters();
> +		x86_pmu_clear_dirty_counters(NULL);
>   }
>   
>   static void x86_pmu_swap_task_ctx(struct perf_event_context *prev,