Re: [PATCH 4/6] mm/mremap: restore VM_ACCOUNT if the unmap fails

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Fri, Oct 30, 2026 at 02:42:49PM +0000, Josef Bacik wrote:
> If the unmap at the end of an mremap() move fails, the source VMA stays
> without VM_ACCOUNT. Neither munmap() nor exit_mmap() uncharges it then, so
> the charge for all of its pages stays in vm_committed_as after the process
> is gone.
>
> unmap_source_vma() clears the flag on the whole source VMA before the unmap
> and sets it again on the remaining parts afterwards, but its error path
> returns without doing so.

Yeah this whole VMA_ACCOUNT_BIT removal/reinstatement is a total hack and I hate
it.

The real long-term fix is to change that to something not terrible I
think... :)

>
> This was reproduced with fault injection: each failure on an anonymous
> mapping left Committed_AS higher by the size of the whole VMA after
> everything was unmapped.
>
> Fix by setting VM_ACCOUNT again on everything in the range of the original

It'd be extremely pedantic to say 'VMA_ACCOUNT_BIT' here, but it's the
kernel, so here I am being _that_ nitty ;)

> VMA. A failed unmap may have split the VMA already, so walk the range.
>
> Fixes: 51df7bcb6151 ("mm/mremap: account memory on do_munmap() failure")

Again, this requires crazy memory pressure, fatal signal etc. etc. so yes,
no cc: stable is valid.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Logically generally OK, but I want you to rework this as per below.

> ---
>  mm/mremap.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 6f941aeb1b5f..256eac721a07 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -1246,9 +1246,20 @@ static void unmap_source_vma(struct vma_remap_struct *vrm)
>  	vrm->vma = NULL; /* Invalidated. */
>  	vrm->vmi_needs_invalidate = true;
>  	if (err) {
> -		/* OOM: unable to split vma, just get accounts right */
> -		if (accountable_move)
> +		/*
> +		 * OOM: unable to split vma, just get accounts right. Both the
> +		 * old and the new range remain mapped, so charge the range a
> +		 * second time, and reinstate the flag on all that is left of
> +		 * the VMA so the charge is returned when it gets unmapped.
> +		 */

This is quite schloppy. I don't think we need this bit.

> +		if (accountable_move) {
>  			vm_acct_memory(len >> PAGE_SHIFT);
> +			vma_iter_set(&vmi, vm_start);
> +			for_each_vma_range(vmi, vma, vm_end) {
> +				vma_start_write(vma);
> +				vma_set_flags(vma, VMA_ACCOUNT_BIT);
> +			}
> +		}

I'd rather separate this out into its own helper. That also means you could
share code with the bit at the end?

Like:

static void vma_range_set_account(struct vma_iterator *vmi,
		unsigned long start, unsigned long end)
{
        struct vm_area_struct *vma;

        vma_iter_set(vmi, start);
        for_each_vma_range(*vmi, vma, end) {
                vma_start_write(vma);
                vma_set_flags(vma, VMA_ACCOUNT_BIT);
        }
}

Then the above error path becomes:

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

And on success at the end of the function you can reduce that bit to:

	/*
	 * ...
	 * do_vmi_munmap() will have restored the VMI back to addr.
	 */
        if (accountable_move)
                vma_range_set_account(&vmi, vm_start, vm_end);

Which is nice :)


>  		return;
>  	}
>
>
> --
> 2.55.0
>

--
Cheers, Lorenzo