Re: [PATCH] perf: Fix the mlock accounting, again

[Thomas Richter]

On 11/15/19 5:08 PM, Alexander Shishkin wrote:
> Commit
> 
>   5e6c3c7b1ec2 ("perf/aux: Fix tracking of auxiliary trace buffer allocation")
> 
> tried to guess the correct combination of arithmetic operations that would
> undo the AUX buffer's mlock accounting, and failed, leaking the bottom part
> when an allocation needs to be changed partially to both user->locked_vm
> and mm->pinned_vm, eventually leaving the user with no locked bonus:
> 
> $ perf record -e intel_pt//u -m1,128 uname
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.061 MB perf.data ]
> $ perf record -e intel_pt//u -m1,128 uname
> Permission error mapping pages.
> Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
> or try again with a smaller value of -m/--mmap_pages.
> (current value: 1,128)
> 
> Fix this by subtracting both locked and pinned counts when AUX buffer is
> unmapped.
> 
> Signefdoff-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> ---
>  kernel/events/core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 16d80ad8d6d7..522438887206 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5664,10 +5664,8 @@ static void perf_mmap_close(struct vm_area_struct *vma)
>  		perf_pmu_output_stop(event);
>  
>  		/* now it's safe to free the pages */
> -		if (!rb->aux_mmap_locked)
> -			atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm);
> -		else
> -			atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
> +		atomic_long_sub(rb->aux_nr_pages - rb->aux_mmap_locked, &mmap_user->locked_vm);
> +		atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
>  
>  		/* this has to be the last one */
>  		rb_free_aux(rb);
> 

I have tested your patch on our test suite which discovered the original issue.
Your patch works nicely, thanks for fixing this.

-- 
Thomas Richter, Dept 3252, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Matthias Hartmann
GeschÃftsfÃhrung: Dirk Wittkopp
Sitz der Gesellschaft: BÃblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294