[PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

[PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

[Lorenzo Stoakes]

While an OOM failure in commit_merge() isn't really feasible due to the
allocation which might fail (a maple tree pre-allocation) being 'too small
to fail', we do need to handle this case correctly regardless.

In vma_merge_existing_range(), we can theoretically encounter failures
which result in an OOM error in two ways - firstly dup_anon_vma() might
fail with an OOM error, and secondly commit_merge() failing, ultimately, to
pre-allocate a maple tree node.

The abort logic for dup_anon_vma() resets the VMA iterator to the initial
range, ensuring that any logic looping on this iterator will correctly
proceed to the next VMA.

However the commit_merge() abort logic does not do the same thing. This
resulted in a syzbot report occurring because mlockall() iterates through
VMAs, is tolerant of errors, but ended up with an incorrect previous VMA
being specified due to incorrect iterator state.

While making this change, it became apparent we are duplicating logic - the
logic introduced in commit 41e6ddcaa0f1 ("mm/vma: add give_up_on_oom option
on modify/merge, use in uffd release") duplicates the vmg->give_up_on_oom
check in both abort branches.

Additionally, we observe that we can perform the anon_dup check safely on
dup_anon_vma() failure, as this will not be modified should this call fail.

Finally, we need to reset the iterator in both cases, so now we can simply
use the exact same code to abort for both.

We remove the VM_WARN_ON(err != -ENOMEM) as it would be silly for this to
be otherwise and it allows us to implement the abort check more neatly.

Reported-by: syzbot+d16409ea9ecc16ed261a@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-mm/6842cc67.a00a0220.29ac89.003b.GAE@google.com/
Fixes: 47b16d0462a4 ("mm: abort vma_modify() on merge out of memory failure")
Cc: stable@vger.kernel.org
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/vma.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/mm/vma.c b/mm/vma.c
index 1497d7d28096..01b1d26d87b4 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -967,26 +967,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
 		err = dup_anon_vma(next, middle, &anon_dup);
 	}

-	if (err)
+	if (err || commit_merge(vmg))
 		goto abort;

-	err = commit_merge(vmg);
-	if (err) {
-		VM_WARN_ON(err != -ENOMEM);
-
-		if (anon_dup)
-			unlink_anon_vmas(anon_dup);
-
-		/*
-		 * We've cleaned up any cloned anon_vma's, no VMAs have been
-		 * modified, no harm no foul if the user requests that we not
-		 * report this and just give up, leaving the VMAs unmerged.
-		 */
-		if (!vmg->give_up_on_oom)
-			vmg->state = VMA_MERGE_ERROR_NOMEM;
-		return NULL;
-	}
-
 	khugepaged_enter_vma(vmg->target, vmg->flags);
 	vmg->state = VMA_MERGE_SUCCESS;
 	return vmg->target;
@@ -995,6 +978,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
 	vma_iter_set(vmg->vmi, start);
 	vma_iter_load(vmg->vmi);

+	if (anon_dup)
+		unlink_anon_vmas(anon_dup);
+
 	/*
 	 * This means we have failed to clone anon_vma's correctly, but no
 	 * actual changes to VMAs have occurred, so no harm no foul - if the
--
2.49.0

Re: [PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

[Pedro Falcato]

On Fri, Jun 06, 2025 at 01:50:32PM +0100, Lorenzo Stoakes wrote:
> While an OOM failure in commit_merge() isn't really feasible due to the
> allocation which might fail (a maple tree pre-allocation) being 'too small
> to fail', we do need to handle this case correctly regardless.
> 
> In vma_merge_existing_range(), we can theoretically encounter failures
> which result in an OOM error in two ways - firstly dup_anon_vma() might
> fail with an OOM error, and secondly commit_merge() failing, ultimately, to
> pre-allocate a maple tree node.
> 
> The abort logic for dup_anon_vma() resets the VMA iterator to the initial
> range, ensuring that any logic looping on this iterator will correctly
> proceed to the next VMA.
> 
> However the commit_merge() abort logic does not do the same thing. This
> resulted in a syzbot report occurring because mlockall() iterates through
> VMAs, is tolerant of errors, but ended up with an incorrect previous VMA
> being specified due to incorrect iterator state.
> 
> While making this change, it became apparent we are duplicating logic - the
> logic introduced in commit 41e6ddcaa0f1 ("mm/vma: add give_up_on_oom option
> on modify/merge, use in uffd release") duplicates the vmg->give_up_on_oom
> check in both abort branches.
> 
> Additionally, we observe that we can perform the anon_dup check safely on
> dup_anon_vma() failure, as this will not be modified should this call fail.
> 
> Finally, we need to reset the iterator in both cases, so now we can simply
> use the exact same code to abort for both.
> 
> We remove the VM_WARN_ON(err != -ENOMEM) as it would be silly for this to
> be otherwise and it allows us to implement the abort check more neatly.
> 
> Reported-by: syzbot+d16409ea9ecc16ed261a@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-mm/6842cc67.a00a0220.29ac89.003b.GAE@google.com/
> Fixes: 47b16d0462a4 ("mm: abort vma_modify() on merge out of memory failure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

> ---
>  mm/vma.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)

Neat cleanup, thanks!

-- 
Pedro

Re: [PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

[Vlastimil Babka]

On 6/6/25 14:50, Lorenzo Stoakes wrote:
> While an OOM failure in commit_merge() isn't really feasible due to the
> allocation which might fail (a maple tree pre-allocation) being 'too small
> to fail', we do need to handle this case correctly regardless.
> 
> In vma_merge_existing_range(), we can theoretically encounter failures
> which result in an OOM error in two ways - firstly dup_anon_vma() might
> fail with an OOM error, and secondly commit_merge() failing, ultimately, to
> pre-allocate a maple tree node.
> 
> The abort logic for dup_anon_vma() resets the VMA iterator to the initial
> range, ensuring that any logic looping on this iterator will correctly
> proceed to the next VMA.
> 
> However the commit_merge() abort logic does not do the same thing. This
> resulted in a syzbot report occurring because mlockall() iterates through
> VMAs, is tolerant of errors, but ended up with an incorrect previous VMA
> being specified due to incorrect iterator state.
> 
> While making this change, it became apparent we are duplicating logic - the
> logic introduced in commit 41e6ddcaa0f1 ("mm/vma: add give_up_on_oom option
> on modify/merge, use in uffd release") duplicates the vmg->give_up_on_oom
> check in both abort branches.
> 
> Additionally, we observe that we can perform the anon_dup check safely on
> dup_anon_vma() failure, as this will not be modified should this call fail.
> 
> Finally, we need to reset the iterator in both cases, so now we can simply
> use the exact same code to abort for both.
> 
> We remove the VM_WARN_ON(err != -ENOMEM) as it would be silly for this to
> be otherwise and it allows us to implement the abort check more neatly.
> 
> Reported-by: syzbot+d16409ea9ecc16ed261a@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-mm/6842cc67.a00a0220.29ac89.003b.GAE@google.com/
> Fixes: 47b16d0462a4 ("mm: abort vma_modify() on merge out of memory failure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Nice.

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Re: [PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250606 08:51]:
> While an OOM failure in commit_merge() isn't really feasible due to the
> allocation which might fail (a maple tree pre-allocation) being 'too small
> to fail', we do need to handle this case correctly regardless.
> 
> In vma_merge_existing_range(), we can theoretically encounter failures
> which result in an OOM error in two ways - firstly dup_anon_vma() might
> fail with an OOM error, and secondly commit_merge() failing, ultimately, to
> pre-allocate a maple tree node.
> 
> The abort logic for dup_anon_vma() resets the VMA iterator to the initial
> range, ensuring that any logic looping on this iterator will correctly
> proceed to the next VMA.
> 
> However the commit_merge() abort logic does not do the same thing. This
> resulted in a syzbot report occurring because mlockall() iterates through
> VMAs, is tolerant of errors, but ended up with an incorrect previous VMA
> being specified due to incorrect iterator state.
> 
> While making this change, it became apparent we are duplicating logic - the
> logic introduced in commit 41e6ddcaa0f1 ("mm/vma: add give_up_on_oom option
> on modify/merge, use in uffd release") duplicates the vmg->give_up_on_oom
> check in both abort branches.
> 
> Additionally, we observe that we can perform the anon_dup check safely on
> dup_anon_vma() failure, as this will not be modified should this call fail.
> 
> Finally, we need to reset the iterator in both cases, so now we can simply
> use the exact same code to abort for both.
> 
> We remove the VM_WARN_ON(err != -ENOMEM) as it would be silly for this to
> be otherwise and it allows us to implement the abort check more neatly.

Nice clean up.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> 
> Reported-by: syzbot+d16409ea9ecc16ed261a@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-mm/6842cc67.a00a0220.29ac89.003b.GAE@google.com/
> Fixes: 47b16d0462a4 ("mm: abort vma_modify() on merge out of memory failure")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/vma.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/vma.c b/mm/vma.c
> index 1497d7d28096..01b1d26d87b4 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -967,26 +967,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
>  		err = dup_anon_vma(next, middle, &anon_dup);
>  	}
> 
> -	if (err)
> +	if (err || commit_merge(vmg))
>  		goto abort;
> 
> -	err = commit_merge(vmg);
> -	if (err) {
> -		VM_WARN_ON(err != -ENOMEM);
> -
> -		if (anon_dup)
> -			unlink_anon_vmas(anon_dup);
> -
> -		/*
> -		 * We've cleaned up any cloned anon_vma's, no VMAs have been
> -		 * modified, no harm no foul if the user requests that we not
> -		 * report this and just give up, leaving the VMAs unmerged.
> -		 */
> -		if (!vmg->give_up_on_oom)
> -			vmg->state = VMA_MERGE_ERROR_NOMEM;
> -		return NULL;
> -	}
> -
>  	khugepaged_enter_vma(vmg->target, vmg->flags);
>  	vmg->state = VMA_MERGE_SUCCESS;
>  	return vmg->target;
> @@ -995,6 +978,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
>  	vma_iter_set(vmg->vmi, start);
>  	vma_iter_load(vmg->vmi);
> 
> +	if (anon_dup)
> +		unlink_anon_vmas(anon_dup);
> +

Nit, I would have put this earlier in the abort path in case other
issues need to be addressed later.  Hardly worth a respin though, I
don't really see this ever needing to be done anyways.

>  	/*
>  	 * This means we have failed to clone anon_vma's correctly, but no
>  	 * actual changes to VMAs have occurred, so no harm no foul - if the
> --
> 2.49.0