Messages in this thread | | | Date | Thu, 26 Jan 2017 15:04:01 -0800 | From | Andres Freund <> | Subject | Re: perf/jit doesn't cope well with mprotect() to jit containing pages |
| |
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 <andres@anarazel.de>
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
| |