Messages in this thread | | | Date | Tue, 1 Sep 2009 23:27:17 +0900 | From | Paul Mundt <> | Subject | Re: page allocator regression on nommu |
| |
On Tue, Sep 01, 2009 at 02:46:45PM +0100, David Howells wrote: > Paul Mundt <lethal@linux-sh.org> 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 <dhowells@redhat.com> > 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@redhat.com>
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 <lethal@linux-sh.org>
| |