Re: [PATCH v2 13/13] mm/mremap: convert mremap code to use vma_flags_t

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Permanent leak of `vm_committed_as` committed memory when `do_vmi_munmap()` fails in `unmap_source_vma()`.
- [High] Permanent leak of `vm_committed_as` (delta size) in the `move_vma()` error path when page table moves fail.
- [High] Permanent leak of `mm->locked_vm` when `MREMAP_DONTUNMAP` is called on a locked VMA.
- [High] `mremap_to()` with `MREMAP_SHRINK` permanently inflates committed memory by failing to reset `vrm->delta`.
--

--- Patch [13]: [PATCH v2 13/13] mm/mremap: convert mremap code to use vma_flags_t ---
commit 2acb878e611c682f5c4e70b5f73085e003532a49
Author: Lorenzo Stoakes <ljs@kernel.org>

mm/mremap: convert mremap code to use vma_flags_t

This commit replaces legacy vm_flags_t with vma_flags_t bitwise values
across the mremap codebase. It updates flag modification functions to their
corresponding vma_* counterparts. No functional changes are intended.

> diff --git a/mm/mremap.c b/mm/mremap.c
> index 384ef4cc2195b..b64aa1f6e07ed 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c

[ ... ]

> @@ -985,7 +985,7 @@ static bool vrm_calc_charge(struct vma_remap_struct *vrm)
>  {
>  	unsigned long charged;
>  
> -	if (!(vrm->vma->vm_flags & VM_ACCOUNT))
> +	if (!vma_test(vrm->vma, VMA_ACCOUNT_BIT))
>  		return true;

[Severity: High]
This isn't a bug introduced by this patch, but since we're updating the
accounting logic here, could this lead to a committed memory leak when
handling a shrink operation?

When mremap_to() processes MREMAP_SHRINK, it sets vrm->old_len to
vrm->new_len but appears to leave vrm->delta at its original value. When
vrm_calc_charge() is subsequently called during move_vma(), it uses
vrm->delta to charge new committed memory, effectively re-charging the
memory that was just shrunk.

Does this permanently inflate vm_committed_as under strict overcommit?

[ ... ]

> @@ -1195,7 +1195,7 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
>  	 * portions of the original VMA that remain.
>  	 */
>  	if (accountable_move) {
> -		vm_flags_clear(vma, VM_ACCOUNT);
> +		vma_clear_flags(vma, VMA_ACCOUNT_BIT);
>  		/* We are about to split vma, so store the start/end. */
>  		vm_start = vma->vm_start;
>  		vm_end = vma->vm_end;

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak of committed
memory here during error handling in unmap_source_vma().

If do_vmi_munmap() fails shortly after this block (e.g., due to OOM when
splitting an adjacent VMA), the error path calls vm_acct_memory() but
returns early without restoring VMA_ACCOUNT_BIT on the surviving VMA:

	if (err) {
		/* OOM: unable to split vma, just get accounts right */
		vm_acct_memory(len >> PAGE_SHIFT);
		return;
	}

Because the bit is stripped, will the system permanently leak this charge
since do_vmi_munmap() won't unaccount it when the VMA is eventually torn
down?

[Severity: High]
This is also a pre-existing issue on the same path. Can this lead to a
similar leak if move_page_tables() fails inside copy_vma_and_data()?

In that scenario, copy_vma_and_data() assigns new_vma to vrm->vma and
returns an error. Then, move_vma() invokes unmap_source_vma() to clean up
new_vma.

This block will strip VMA_ACCOUNT_BIT from new_vma before unmapping
it, which seems to prevent do_vmi_munmap() from uncharging the delta added
earlier by vrm_calc_charge().

[ ... ]

> @@ -1330,8 +1332,8 @@ static void dontunmap_complete(struct vma_remap_struct *vrm,
>  	unsigned long old_start = vrm->vma->vm_start;
>  	unsigned long old_end = vrm->vma->vm_end;
>  
> -	/* We always clear VM_LOCKED[ONFAULT] on the old VMA. */
> -	vm_flags_clear(vrm->vma, VM_LOCKED_MASK);
> +	/* We always clear VMA_LOCKED[ONFAULT]_BIT on the old VMA. */
> +	vma_clear_flags_mask(vrm->vma, VMA_LOCKED_MASK);

[Severity: High]
This is a pre-existing issue, but could stripping the locked mask here leak
the mm->locked_vm count?

By clearing VMA_LOCKED_MASK unconditionally from the old VMA without
decrementing mm->locked_vm or unlocking the pages, it seems the old VMA
will no longer account for its locked pages when it is eventually unmapped.

Since move_vma() already added the new VMA's size to mm->locked_vm, does
this permanently inflate the process's locked memory count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-vma-flags-mm-v2-0-0fa2357d5431@kernel.org?part=13