Paul Mundt <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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;
On Tue, 2009-09-01 at 14:46 +0100, David Howells wrote:
> From: David Howells <[email protected]>
> 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 <[email protected]>
Looks sane to me. FWIW:
Acked-by: Pekka Enberg <[email protected]>
Pekka
On Tue, Sep 01, 2009 at 02:46:45PM +0100, David Howells wrote:
> Paul Mundt <[email protected]> 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.
>
[snip]
> From: David Howells <[email protected]>
> 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 <[email protected]>
Agreed, that does look cleaner. After playing around with it a bit, I concede
that the BUG_ON() is definitely worth preserving. :-)
Acked-by: Paul Mundt <[email protected]>