Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752745AbdHBRjh (ORCPT ); Wed, 2 Aug 2017 13:39:37 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:58930 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752154AbdHBRjd (ORCPT ); Wed, 2 Aug 2017 13:39:33 -0400 Date: Wed, 2 Aug 2017 19:39:30 +0200 From: Peter Zijlstra To: Vince Weaver Cc: linux-kernel@vger.kernel.org, Andy Lutomirski , Ingo Molnar , Arnaldo Carvalho de Melo , Stephane Eranian Subject: Re: perf: bug in rdpmc/mmap accounting after exec Message-ID: <20170802173930.cstykcqefmqt7jau@hirez.programming.kicks-ass.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4492 Lines: 129 On Wed, Jul 26, 2017 at 03:39:01PM -0400, Vince Weaver wrote: > Hello > > so one last bug found by the PAPI testsuite. > > This one involves the rdpmc auto-disable on last unmap of an event > feature. > > Failing test case: > > fd=perf_event_open(); > addr=mmap(fd); > exec() // without closing or unmapping the event > fd=perf_event_open(); > addr=mmap(fd); > rdpmc() // GPFs due to rdpmc being disabled > > I won't pretend to be able to follow the rdpmc disabling code, but if I > add some printks it looks like > current->mm->context.perf_rdpmc_allowed > isn't properly being reset on exec? > > In fact, current->mm->context.perf_rdpmc_allowed goes negative which seems > like it shouldn't happen? > > Anyway, a test case for this can be found in the perf_event_tests, > tests/rdpmc/rdpmc_exec_papi Good find that... The below seems to fix that for me. --- diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index 8e3db8f642a7..3186f6e081c4 100644 --- a/arch/x86/events/core.c +++ b/arch/x86/events/core.c @@ -2114,11 +2114,14 @@ static void refresh_pce(void *ignored) load_mm_cr4(this_cpu_read(cpu_tlbstate.loaded_mm)); } -static void x86_pmu_event_mapped(struct perf_event *event) +static void x86_pmu_event_mapped(struct perf_event *event, struct vm_area_struct *vma) { + struct mm_struct *mm; + if (!(event->hw.flags & PERF_X86_EVENT_RDPMC_ALLOWED)) return; + mm = vma->vm_mm; /* * This function relies on not being called concurrently in two * tasks in the same mm. Otherwise one task could observe @@ -2129,22 +2132,21 @@ static void x86_pmu_event_mapped(struct perf_event *event) * For now, this can't happen because all callers hold mmap_sem * for write. If this changes, we'll need a different solution. */ - lockdep_assert_held_exclusive(¤t->mm->mmap_sem); + lockdep_assert_held_exclusive(&mm->mmap_sem); - if (atomic_inc_return(¤t->mm->context.perf_rdpmc_allowed) == 1) - on_each_cpu_mask(mm_cpumask(current->mm), refresh_pce, NULL, 1); + if (atomic_inc_return(&mm->context.perf_rdpmc_allowed) == 1) + on_each_cpu_mask(mm_cpumask(mm), refresh_pce, NULL, 1); } -static void x86_pmu_event_unmapped(struct perf_event *event) +static void x86_pmu_event_unmapped(struct perf_event *event, struct vm_area_struct *vma) { - if (!current->mm) - return; + struct mm_struct *mm = vma->vm_mm; if (!(event->hw.flags & PERF_X86_EVENT_RDPMC_ALLOWED)) return; - if (atomic_dec_and_test(¤t->mm->context.perf_rdpmc_allowed)) - on_each_cpu_mask(mm_cpumask(current->mm), refresh_pce, NULL, 1); + if (atomic_dec_and_test(&mm->context.perf_rdpmc_allowed)) + on_each_cpu_mask(mm_cpumask(mm), refresh_pce, NULL, 1); } static int x86_pmu_event_idx(struct perf_event *event) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index a3b873fc59e4..271a32a7e3e1 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -310,8 +310,8 @@ struct pmu { * Notification that the event was mapped or unmapped. Called * in the context of the mapping task. */ - void (*event_mapped) (struct perf_event *event); /*optional*/ - void (*event_unmapped) (struct perf_event *event); /*optional*/ + void (*event_mapped) (struct perf_event *event, struct vm_area_struct *vma); /*optional*/ + void (*event_unmapped) (struct perf_event *event, struct vm_area_struct *vma); /*optional*/ /* * Flags for ->add()/->del()/ ->start()/->stop(). There are diff --git a/kernel/events/core.c b/kernel/events/core.c index 426c2ffba16d..f724348bba08 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5090,7 +5105,7 @@ static void perf_mmap_open(struct vm_area_struct *vma) atomic_inc(&event->rb->aux_mmap_count); if (event->pmu->event_mapped) - event->pmu->event_mapped(event); + event->pmu->event_mapped(event, vma); } static void perf_pmu_output_stop(struct perf_event *event); @@ -5113,7 +5128,7 @@ static void perf_mmap_close(struct vm_area_struct *vma) unsigned long size = perf_data_size(rb); if (event->pmu->event_unmapped) - event->pmu->event_unmapped(event); + event->pmu->event_unmapped(event, vma); /* * rb->aux_mmap_count will always drop before rb->mmap_count and @@ -5411,7 +5426,7 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma) vma->vm_ops = &perf_mmap_vmops; if (event->pmu->event_mapped) - event->pmu->event_mapped(event); + event->pmu->event_mapped(event, vma); return ret; }