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