diff mbox series

[hotfix,6.12] mm/mmap: correct error handling in mmap_region()

Message ID 20241001133726.160275-1-lorenzo.stoakes@oracle.com (mailing list archive)
State New
Headers show
Series [hotfix,6.12] mm/mmap: correct error handling in mmap_region() | expand

Commit Message

Lorenzo Stoakes Oct. 1, 2024, 1:37 p.m. UTC 
Commit f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
changed how error handling is performed in mmap_region().

The error value defaults to -ENOMEM, but then gets reassigned immediately
to the result of vms_gather_munmap_vmas() if we are performing a MAP_FIXED
mapping over existing VMAs (and thus unmapping them).

This overwrites the error value, potentially clearing it.

After this, we invoke may_expand_vm() and possibly vm_area_alloc(), and
check to see if they failed. If they do so, then we perform error-handling
logic, but importantly, we do NOT update the error code.

This means that, if vms_gather_munmap_vmas() succeeds, but one of these
calls does not, the function will return indicating no error, but rather an
address value of zero, which is entirely incorrect.

Correct this and avoid future confusion by strictly setting error on each
and every occasion we jump to the error handling logic, and set the error
code immediately prior to doing so.

This way we can see at a glance that the error code is always correct.

Many thanks to Vegard Nossum who spotted this issue in discussion around
this problem.

Reported-by: Bert Karwatzki <spasswolf@web.de>
Link: https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@web.de/
Suggested-by: Vegard Nossum <vegard.nossum@oracle.com>
Fixes: f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/mmap.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

--
2.46.2

Comments

Vlastimil Babka Oct. 1, 2024, 4:42 p.m. UTC | #1 
On 10/1/24 15:37, Lorenzo Stoakes wrote:
> Commit f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> changed how error handling is performed in mmap_region().
> 
> The error value defaults to -ENOMEM, but then gets reassigned immediately
> to the result of vms_gather_munmap_vmas() if we are performing a MAP_FIXED
> mapping over existing VMAs (and thus unmapping them).
> 
> This overwrites the error value, potentially clearing it.
> 
> After this, we invoke may_expand_vm() and possibly vm_area_alloc(), and
> check to see if they failed. If they do so, then we perform error-handling
> logic, but importantly, we do NOT update the error code.
> 
> This means that, if vms_gather_munmap_vmas() succeeds, but one of these
> calls does not, the function will return indicating no error, but rather an
> address value of zero, which is entirely incorrect.
> 
> Correct this and avoid future confusion by strictly setting error on each
> and every occasion we jump to the error handling logic, and set the error
> code immediately prior to doing so.
> 
> This way we can see at a glance that the error code is always correct.
> 
> Many thanks to Vegard Nossum who spotted this issue in discussion around
> this problem.
> 
> Reported-by: Bert Karwatzki <spasswolf@web.de>
> Link: https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@web.de/

I'd be surprised if that fixed the reported issue, but yeah this is a bug to
fix anyway.

> Suggested-by: Vegard Nossum <vegard.nossum@oracle.com>
> Fixes: f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Lorenzo Stoakes Oct. 1, 2024, 4:51 p.m. UTC | #2 
On Tue, Oct 01, 2024 at 06:42:47PM GMT, Vlastimil Babka wrote:
> On 10/1/24 15:37, Lorenzo Stoakes wrote:
> > Commit f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > changed how error handling is performed in mmap_region().
> >
> > The error value defaults to -ENOMEM, but then gets reassigned immediately
> > to the result of vms_gather_munmap_vmas() if we are performing a MAP_FIXED
> > mapping over existing VMAs (and thus unmapping them).
> >
> > This overwrites the error value, potentially clearing it.
> >
> > After this, we invoke may_expand_vm() and possibly vm_area_alloc(), and
> > check to see if they failed. If they do so, then we perform error-handling
> > logic, but importantly, we do NOT update the error code.
> >
> > This means that, if vms_gather_munmap_vmas() succeeds, but one of these
> > calls does not, the function will return indicating no error, but rather an
> > address value of zero, which is entirely incorrect.
> >
> > Correct this and avoid future confusion by strictly setting error on each
> > and every occasion we jump to the error handling logic, and set the error
> > code immediately prior to doing so.
> >
> > This way we can see at a glance that the error code is always correct.
> >
> > Many thanks to Vegard Nossum who spotted this issue in discussion around
> > this problem.
> >
> > Reported-by: Bert Karwatzki <spasswolf@web.de>
> > Link: https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@web.de/
>
> I'd be surprised if that fixed the reported issue, but yeah this is a bug to
> fix anyway.

Yeah it seems that you brought about a tear in the space-time continuum and
the very moment you said that Bert reported that yes sadly this doesn't fix
it :>)

I thought maybe some very weird bug by this function returning a non-error
result (of zero!) when it should have failed might have triggered some
later maple tree corruption but yeah, long shot I guess :(

Still, ultimately it spawned from the report and forms part of an overall
fix of the function so may as well vaguely keep the R-b tag, I kept it a
link rather than 'closes' so should be fine.

>
> > Suggested-by: Vegard Nossum <vegard.nossum@oracle.com>
> > Fixes: f8d112a4e657 ("mm/mmap: avoid zeroing vma tree in mmap_region()")
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
>

Thanks!
Andrew Morton Oct. 1, 2024, 9:44 p.m. UTC | #3 
On Tue, 1 Oct 2024 17:51:07 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> > I'd be surprised if that fixed the reported issue, but yeah this is a bug to
> > fix anyway.
> 
> Yeah it seems that you brought about a tear in the space-time continuum and
> the very moment you said that Bert reported that yes sadly this doesn't fix
> it :>)

Please resend with a new changelog?
Lorenzo Stoakes Oct. 2, 2024, 7:36 a.m. UTC | #4 
On Tue, Oct 01, 2024 at 02:44:33PM GMT, Andrew Morton wrote:
> On Tue, 1 Oct 2024 17:51:07 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> > > I'd be surprised if that fixed the reported issue, but yeah this is a bug to
> > > fix anyway.
> >
> > Yeah it seems that you brought about a tear in the space-time continuum and
> > the very moment you said that Bert reported that yes sadly this doesn't fix
> > it :>)
>
> Please resend with a new changelog?

Note that this does fix an absolutely critical issue, just not the one that
triggered this.

I'll resend dropping the reported-by and link tags and adding Vlasta's
reviewed-by but everything else is valid.
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index dd4b35a25aeb..9c0fb43064b5 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1371,7 +1371,7 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	struct maple_tree mt_detach;
 	unsigned long end = addr + len;
 	bool writable_file_mapping = false;
-	int error = -ENOMEM;
+	int error;
 	VMA_ITERATOR(vmi, mm, addr);
 	VMG_STATE(vmg, mm, &vmi, addr, end, vm_flags, pgoff);

@@ -1396,8 +1396,10 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	}

 	/* Check against address space limit. */
-	if (!may_expand_vm(mm, vm_flags, pglen - vms.nr_pages))
+	if (!may_expand_vm(mm, vm_flags, pglen - vms.nr_pages)) {
+		error = -ENOMEM;
 		goto abort_munmap;
+	}

 	/*
 	 * Private writable mapping: check memory availability
@@ -1405,8 +1407,11 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	if (accountable_mapping(file, vm_flags)) {
 		charged = pglen;
 		charged -= vms.nr_accounted;
-		if (charged && security_vm_enough_memory_mm(mm, charged))
-			goto abort_munmap;
+		if (charged) {
+			error = security_vm_enough_memory_mm(mm, charged);
+			if (error)
+				goto abort_munmap;
+		}

 		vms.nr_accounted = 0;
 		vm_flags |= VM_ACCOUNT;
@@ -1422,8 +1427,10 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	 * not unmapped, but the maps are removed from the list.
 	 */
 	vma = vm_area_alloc(mm);
-	if (!vma)
+	if (!vma) {
+		error = -ENOMEM;
 		goto unacct_error;
+	}

 	vma_iter_config(&vmi, addr, end);
 	vma_set_range(vma, addr, end, pgoff);
@@ -1453,9 +1460,10 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 		 * Expansion is handled above, merging is handled below.
 		 * Drivers should not alter the address of the VMA.
 		 */
-		error = -EINVAL;
-		if (WARN_ON((addr != vma->vm_start)))
+		if (WARN_ON((addr != vma->vm_start))) {
+			error = -EINVAL;
 			goto close_and_free_vma;
+		}

 		vma_iter_config(&vmi, addr, end);
 		/*
@@ -1500,13 +1508,15 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	}

 	/* Allow architectures to sanity-check the vm_flags */
-	error = -EINVAL;
-	if (!arch_validate_flags(vma->vm_flags))
+	if (!arch_validate_flags(vma->vm_flags)) {
+		error = -EINVAL;
 		goto close_and_free_vma;
+	}

-	error = -ENOMEM;
-	if (vma_iter_prealloc(&vmi, vma))
+	if (vma_iter_prealloc(&vmi, vma)) {
+		error = -ENOMEM;
 		goto close_and_free_vma;
+	}

 	/* Lock the VMA since it is modified after insertion into VMA tree */
 	vma_start_write(vma);