[PATCH] mm/mmap: Fix error return in do_vmi_align_munmap()

[PATCH] mm/mmap: Fix error return in do_vmi_align_munmap()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2010 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

If mas_store_gfp() in the gather loop failed, the 'error' variable that
ultimately gets returned was not being set. In many cases, its original
value of -ENOMEM was still in place, and that was fine. But if VMAs had
been split at the start or end of the range, then 'error' could be zero.

Change to the 'error = foo(); if (error) goto …' idiom to fix the bug.

Also clean up a later case which avoided the same bug by *explicitly*
setting error = -ENOMEM right before calling the function that might
return -ENOMEM.

In a final cosmetic change, move the 'Point of no return' comment to
*after* the goto. That's been in the wrong place since the preallocation
was removed, and this new error path was added.

Fixes: 606c812eb1d5 ("mm/mmap: Fix error path in do_vmi_align_munmap()")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 mm/mmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index d600404580b2..13128e908470 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2387,7 +2387,8 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		}
 		vma_start_write(next);
 		mas_set_range(&mas_detach, next->vm_start, next->vm_end - 1);
-		if (mas_store_gfp(&mas_detach, next, GFP_KERNEL))
+		error = mas_store_gfp(&mas_detach, next, GFP_KERNEL);
+		if (error)
 			goto munmap_gather_failed;
 		vma_mark_detached(next, true);
 		if (next->vm_flags & VM_LOCKED)
@@ -2436,12 +2437,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		BUG_ON(count != test_count);
 	}
 #endif
-	/* Point of no return */
-	error = -ENOMEM;
 	vma_iter_set(vmi, start);
-	if (vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL))
+	error = vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL);
+	if (error)
 		goto clear_tree_failed;
 
+	/* Point of no return */
 	mm->locked_vm -= locked_vm;
 	mm->map_count -= count;
 	/*
-- 
2.34.1



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] mm/mmap: Fix error return in do_vmi_align_munmap()

[Greg KH]

On Wed, Jun 28, 2023 at 11:42:45AM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If mas_store_gfp() in the gather loop failed, the 'error' variable that
> ultimately gets returned was not being set. In many cases, its original
> value of -ENOMEM was still in place, and that was fine. But if VMAs had
> been split at the start or end of the range, then 'error' could be zero.
> 
> Change to the 'error = foo(); if (error) goto …' idiom to fix the bug.
> 
> Also clean up a later case which avoided the same bug by *explicitly*
> setting error = -ENOMEM right before calling the function that might
> return -ENOMEM.
> 
> In a final cosmetic change, move the 'Point of no return' comment to
> *after* the goto. That's been in the wrong place since the preallocation
> was removed, and this new error path was added.
> 
> Fixes: 606c812eb1d5 ("mm/mmap: Fix error path in do_vmi_align_munmap()")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  mm/mmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] mm/mmap: Fix error return in do_vmi_align_munmap()

[Liam R. Howlett]

* David Woodhouse <dwmw2@infradead.org> [230628 06:43]:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If mas_store_gfp() in the gather loop failed, the 'error' variable that
> ultimately gets returned was not being set. In many cases, its original
> value of -ENOMEM was still in place, and that was fine. But if VMAs had
> been split at the start or end of the range, then 'error' could be zero.
> 
> Change to the 'error = foo(); if (error) goto …' idiom to fix the bug.
> 
> Also clean up a later case which avoided the same bug by *explicitly*
> setting error = -ENOMEM right before calling the function that might
> return -ENOMEM.
> 
> In a final cosmetic change, move the 'Point of no return' comment to
> *after* the goto. That's been in the wrong place since the preallocation
> was removed, and this new error path was added.
> 
> Fixes: 606c812eb1d5 ("mm/mmap: Fix error path in do_vmi_align_munmap()")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

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

> ---
>  mm/mmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d600404580b2..13128e908470 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2387,7 +2387,8 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		}
>  		vma_start_write(next);
>  		mas_set_range(&mas_detach, next->vm_start, next->vm_end - 1);
> -		if (mas_store_gfp(&mas_detach, next, GFP_KERNEL))
> +		error = mas_store_gfp(&mas_detach, next, GFP_KERNEL);
> +		if (error)
>  			goto munmap_gather_failed;
>  		vma_mark_detached(next, true);
>  		if (next->vm_flags & VM_LOCKED)
> @@ -2436,12 +2437,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		BUG_ON(count != test_count);
>  	}
>  #endif
> -	/* Point of no return */
> -	error = -ENOMEM;
>  	vma_iter_set(vmi, start);
> -	if (vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL))
> +	error = vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL);
> +	if (error)
>  		goto clear_tree_failed;
>  
> +	/* Point of no return */
>  	mm->locked_vm -= locked_vm;
>  	mm->map_count -= count;
>  	/*
> -- 
> 2.34.1
> 
> 

[GIT PULL] Fix error return in do_vmi_align_munmap()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

Hi Linus,

The following changes since commit 6aeadf7896bff4ca230702daba8788455e6b866e:

  Merge tag 'docs-arm64-move' of git://git.lwn.net/linux (2023-06-27 21:52:15 -0700)

are available in the Git repository at:

  git://git.infradead.org/users/dwmw2/linux unmap-fix-20230629

for you to fetch changes up to 6c26bd4384da24841bac4f067741bbca18b0fb74:

  mm/mmap: Fix error return in do_vmi_align_munmap() (2023-06-28 14:39:07 +0100)


It's not *imperative* that this be a pull request instead of being
committed again through whatever tree wants to take it, but the commit
ID is already being referenced in the stable backports so ideally we'd
keep it the same.

----------------------------------------------------------------
David Woodhouse (1):
      mm/mmap: Fix error return in do_vmi_align_munmap()

 mm/mmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [GIT PULL] Fix error return in do_vmi_align_munmap()

[pr-tracker-bot]

The pull request you sent on Thu, 29 Jun 2023 22:17:09 +0100:

> git://git.infradead.org/users/dwmw2/linux unmap-fix-20230629

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/43ec8a620b38291c959afb47816cf2de6207125a

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html