diff mbox series

mm/mmap: Fix error return in do_vmi_align_munmap()

Message ID ef2c7c0eeb166acf050597f49eb118d94f18bd39.camel@infradead.org
State New
Headers
Series mm/mmap: Fix error return in do_vmi_align_munmap() |

Commit Message

David Woodhouse June 28, 2023, 10:42 a.m. UTC 
  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(-)

Comments

Liam R. Howlett June 28, 2023, 1:13 p.m. UTC | #1 
* 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
> 
>
diff mbox series

Patch

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;
 	/*