From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752796AbdAZXEG (ORCPT ); Thu, 26 Jan 2017 18:04:06 -0500 Received: from out3-smtp.messagingengine.com ([66.111.4.27]:42342 "EHLO out3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752229AbdAZXEE (ORCPT ); Thu, 26 Jan 2017 18:04:04 -0500 X-ME-Sender: X-Sasl-enc: 4kSsyrX7Ry483YHsXnYBqOwsep0u6IdFYlz8Yfb7qzHA 1485471842 Date: Thu, 26 Jan 2017 15:04:01 -0800 From: Andres Freund To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, Stephane Eranian , acme@kernel.org, jolsa@redhat.com, mingo@elte.hu, anton@ozlabs.org, namhyung@kernel.org, Stephane Eranian Subject: Re: perf/jit doesn't cope well with mprotect() to jit containing pages Message-ID: <20170126230401.mbehdzy2sclbb73b@alap3.anarazel.de> References: <20161210050218.jw4bak5jf766iqpb@alap3.anarazel.de> <20161212084903.GZ3124@twins.programming.kicks-ass.net> <20170126221508.GF6536@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170126221508.GF6536@twins.programming.kicks-ass.net> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017-01-26 23:15:08 +0100, Peter Zijlstra wrote: > On Mon, Dec 12, 2016 at 09:49:03AM +0100, Peter Zijlstra wrote: > > > > BTW, it's also a bit weird that those MMAP2 records triggered by > > > mprotect/mmap, have prot set to 0... > > > > Yes, mprotect() does: vma->vm_flags = newflags; before calling > > perf_event_mmap(vma); which then looks at VM_{READ,WRITE,EXEC} bits in > > that word to generate the prot value. > > > > So that being 0 is a bit weird. > > OK, that was a silly bug :/ Heh, that explains that part. Thanks for looking into it! > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 896aa53..e67c5ba 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -6660,6 +6683,27 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event) > char *buf = NULL; > char *name; > > + if (vma->vm_flags & VM_READ) > + prot |= PROT_READ; > + if (vma->vm_flags & VM_WRITE) > + prot |= PROT_WRITE; > + if (vma->vm_flags & VM_EXEC) > + prot |= PROT_EXEC; > + > + if (vma->vm_flags & VM_MAYSHARE) > + flags = MAP_SHARED; > + else > + flags = MAP_PRIVATE; > + > + if (vma->vm_flags & VM_DENYWRITE) > + flags |= MAP_DENYWRITE; > + if (vma->vm_flags & VM_MAYEXEC) > + flags |= MAP_EXECUTABLE; > + if (vma->vm_flags & VM_LOCKED) > + flags |= MAP_LOCKED; > + if (vma->vm_flags & VM_HUGETLB) > + flags |= MAP_HUGETLB; > + > if (file) { > struct inode *inode; > dev_t dev; > @@ -6686,27 +6730,6 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event) > maj = MAJOR(dev); > min = MINOR(dev); > > - if (vma->vm_flags & VM_READ) > - prot |= PROT_READ; > - if (vma->vm_flags & VM_WRITE) > - prot |= PROT_WRITE; > - if (vma->vm_flags & VM_EXEC) > - prot |= PROT_EXEC; > - > - if (vma->vm_flags & VM_MAYSHARE) > - flags = MAP_SHARED; > - else > - flags = MAP_PRIVATE; > - > - if (vma->vm_flags & VM_DENYWRITE) > - flags |= MAP_DENYWRITE; > - if (vma->vm_flags & VM_MAYEXEC) > - flags |= MAP_EXECUTABLE; > - if (vma->vm_flags & VM_LOCKED) > - flags |= MAP_LOCKED; > - if (vma->vm_flags & VM_HUGETLB) > - flags |= MAP_HUGETLB; > - > goto got_name; > } else { > if (vma->vm_ops && vma->vm_ops->name) { I'm not a mm/perf (or kernel in general) expert. But this looks obviously buggy to me. And the fix makes it look correct. So if that's helpful (I'm not actually sure): Signed-off-by: Andres Freund Not sure if this on its own has enough consequences to be worth being put into stable? Not that it's this fix's responsibility, but I did note that that piece of code doesn't maintain MAP_GROWSDOWN, nor set the bits controlling the size of the HUGETLB mapping. Seems harmless enough, but given that I already looked I thought it'd bring it up. Greetings, Andres Freund