Re: [PATCH] mm/mlock: Fix vma iterator conversion of apply_vma_lock_flags()

[Ryan Roberts]

On 11/07/2023 18:50, Liam R. Howlett wrote:
> apply_vma_lock_flags() calls mlock_fixup(), which could merge the VMA
> after where the vma iterator is located.  Although this is not an issue,
> the next iteration of the loop will check the start of the vma to be
> equal to the locally saved 'tmp' variable and cause an incorrect failure
> scenario.  Fix the error by setting tmp to the end of the vma iterator
> value before restarting the loop.
> 
> There is also a potential of the error code being overwritten when the
> loop terminates early.  Fix the return issue by directly returning when
> an error is encountered since there is nothing to undo after the loop.
> 
> Reported-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> Link: https://lore.kernel.org/linux-mm/50341ca1-d582-b33a-e3d0-acb08a65166f@xxxxxxx/
> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: 37598f5a9d8b ("mlock: convert mlock to vma iterator")
> Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>

Wow; thanks for the quick fix! I've tested this on arm64/v6.4 with the failing
mm selftests that I reported: they are now passing.

Tested-by: Ryan Roberts <ryan.roberts@xxxxxxx>

> ---
>  mm/mlock.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index d7db94519884..0a0c996c5c21 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -477,7 +477,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>  {
>  	unsigned long nstart, end, tmp;
>  	struct vm_area_struct *vma, *prev;
> -	int error;
>  	VMA_ITERATOR(vmi, current->mm, start);
>  
>  	VM_BUG_ON(offset_in_page(start));
> @@ -498,6 +497,7 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>  	nstart = start;
>  	tmp = vma->vm_start;
>  	for_each_vma_range(vmi, vma, end) {
> +		int error;
>  		vm_flags_t newflags;
>  
>  		if (vma->vm_start != tmp)
> @@ -511,14 +511,15 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>  			tmp = end;
>  		error = mlock_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
>  		if (error)
> -			break;
> +			return error;
> +		tmp = vma_iter_end(&vmi);
>  		nstart = tmp;
>  	}
>  
> -	if (vma_iter_end(&vmi) < end)
> +	if (tmp < end)
>  		return -ENOMEM;
>  
> -	return error;
> +	return 0;
>  }
>  
>  /*