Re: page allocator regression on nommu

[David Howells]

Paul Mundt <lethal@xxxxxxxxxxxx> wrote:

> Yeah, that looks a bit suspect. __put_nommu_region() is safe to be called
> without a call to add_nommu_region(), but we happen to trip over the
> BUG_ON() in this case because we've never made a single addition to the
> region tree.
> 
> We probably ought to just up_write() and return if nommu_region_tree ==
> RB_ROOT, which is what I'll do unless David objects.

I think that's the wrong thing to do.  I think we're better moving the call to
add_nommu_region() to above the "/* set up the mapping */" comment.  We hold
the region semaphore at this point, so the fact that it winds up in the tree
briefly won't cause a race, and it means __put_nommu_region() can be used with
impunity to correctly clean up.

See attached patch.

David
---
From: David Howells <dhowells@xxxxxxxxxx>
Subject: [PATCH] NOMMU: Fix error handling in do_mmap_pgoff()

Fix the error handling in do_mmap_pgoff().  If do_mmap_shared_file() or
do_mmap_private() fail, we jump to the error_put_region label at which point we
cann __put_nommu_region() on the region - but we haven't yet added the region
to the tree, and so __put_nommu_region() may BUG because the region tree is
empty or it may corrupt the region tree.

To get around this, we can afford to add the region to the region tree before
calling do_mmap_shared_file() or do_mmap_private() as we keep nommu_region_sem
write-locked, so no-one can race with us by seeing a transient region.

Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
---

 mm/nommu.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)


diff --git a/mm/nommu.c b/mm/nommu.c
index 7466c7a..aabe86c 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1347,6 +1347,7 @@ unsigned long do_mmap_pgoff(struct file *file,
 	}
 
 	vma->vm_region = region;
+	add_nommu_region(region);
 
 	/* set up the mapping */
 	if (file && vma->vm_flags & VM_SHARED)
@@ -1356,8 +1357,6 @@ unsigned long do_mmap_pgoff(struct file *file,
 	if (ret < 0)
 		goto error_put_region;
 
-	add_nommu_region(region);
-
 	/* okay... we have a mapping; now we have to register it */
 	result = vma->vm_start;
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/