Re: perf/jit doesn't cope well with mprotect() to jit containing pages

[Andres Freund]

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@xxxxxxxxxxx>

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