Re: [PATCH 1/7] mm/mremap: correctly handle partial mremap() of VMA starting at 0

[Harry Yoo]

On Mon, Mar 03, 2025 at 11:08:31AM +0000, Lorenzo Stoakes wrote:
> Consider the case of a a partial mremap() (that results in a VMA split) of
> an accountable VMA (i.e. which has the VM_ACCOUNT flag set) whose start
> address is zero, with the MREMAP_MAYMOVE flag specified and a scenario
> where a move does in fact occur:
> 
>        addr  end
>         |     |
>         v     v
>     |-------------|
>     |     vma     |
>     |-------------|
>     0
> 
> This move is affected by unmapping the range [addr, end). In order to
> prevent an incorrect decrement of accounted memory which has already been
> determined, the mremap() code in move_vma() clears VM_ACCOUNT from the VMA
> prior to doing so, before reestablishing it in each of the VMAs post-split:
> 
>     addr  end
>      |     |
>      v     v
>  |---|     |---|
>  | A |     | B |
>  |---|     |---|
> 
> Commit 6b73cff239e5 ("mm: change munmap splitting order and move_vma()")
> changed this logic such as to determine whether there is a need to do so by
> establishing account_start and account_end and, in the instance where such
> an operation is required, assigning them to vma->vm_start and vma->vm_end.
> 
> Later the code checks if the operation is required for 'A' referenced above
> thusly:
> 
> 	if (account_start) {
> 		...
> 	}
> 
> However, if the VMA described above has vma->vm_start == 0, which is now
> assigned to account_start, this branch will not be executed.
> 
> As a result, the VMA 'A' above will remain stripped of its VM_ACCOUNT flag,
> incorrectly.
> 
> The fix is to simply convert these variables to booleans and set them as
> required.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> Fixes: 6b73cff239e5 ("mm: change munmap splitting order and move_vma()")
> Cc: stable@xxxxxxxxxxxxxxx
> ---

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@xxxxxxxxxx>

-- 
Cheers,
Harry

>  mm/mremap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cff7f552f909..c3e4c86d0b8d 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -705,8 +705,8 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>  	unsigned long vm_flags = vma->vm_flags;
>  	unsigned long new_pgoff;
>  	unsigned long moved_len;
> -	unsigned long account_start = 0;
> -	unsigned long account_end = 0;
> +	bool account_start = false;
> +	bool account_end = false;
>  	unsigned long hiwater_vm;
>  	int err = 0;
>  	bool need_rmap_locks;
> @@ -790,9 +790,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>  	if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP)) {
>  		vm_flags_clear(vma, VM_ACCOUNT);
>  		if (vma->vm_start < old_addr)
> -			account_start = vma->vm_start;
> +			account_start = true;
>  		if (vma->vm_end > old_addr + old_len)
> -			account_end = vma->vm_end;
> +			account_end = true;
>  	}
>  
>  	/*
> @@ -832,7 +832,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>  		/* OOM: unable to split vma, just get accounts right */
>  		if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP))
>  			vm_acct_memory(old_len >> PAGE_SHIFT);
> -		account_start = account_end = 0;
> +		account_start = account_end = false;
>  	}
>  
>  	if (vm_flags & VM_LOCKED) {
> -- 
> 2.48.1
> 
>