Messages in this thread | | | Subject | Re: [PATCH V7 2/2] perf/x86: Reset the dirty counter to prevent the leak for an RDPMC task | From | "Liang, Kan" <> | Date | Fri, 14 May 2021 11:30:12 -0400 |
| |
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
| |