Messages in this thread | | | Subject | Re: [patch] rfc: fix splice mapping race? | From | Peter Zijlstra <> | Date | Sun, 18 Jun 2006 12:02:45 +0200 |
| |
On Sun, 2006-06-18 at 11:41 +0200, Nick Piggin wrote: > Hi, I would be interested in confirmation/comments for this patch. > > I believe splice is unsafe to access the page mapping obtained > when the page was unlocked: the page could subsequently be truncated > and the mapping reclaimed (see set_page_dirty_lock comments). > > Modify the remove_mapping precondition to ensure the caller has > locked the page and obtained the correct mapping. Modify callers to > ensure the mapping is the correct one. > > In page migration, detect the missing mapping early and bail out if > that is the case: the page is not going to get un-truncated, so > retrying is just a waste of time. > > Signed-off-by: Nick Piggin <npiggin@suse.de>
Looks sane, except the change in migrate (comment there). I like the remove_mapping() pre-conditions.
> > Index: linux-2.6/fs/splice.c > =================================================================== > --- linux-2.6.orig/fs/splice.c > +++ linux-2.6/fs/splice.c > @@ -55,9 +55,12 @@ static int page_cache_pipe_buf_steal(str > struct pipe_buffer *buf) > { > struct page *page = buf->page; > - struct address_space *mapping = page_mapping(page); > + struct address_space *mapping; > > lock_page(page); > + mapping = page_mapping(page); > + if (!mapping) > + goto out_failed; > > WARN_ON(!PageUptodate(page)); > > @@ -74,6 +77,7 @@ static int page_cache_pipe_buf_steal(str > try_to_release_page(page, mapping_gfp_mask(mapping)); > > if (!remove_mapping(mapping, page)) { > +out_failed: > unlock_page(page); > return 1; > } > Index: linux-2.6/mm/migrate.c > =================================================================== > --- linux-2.6.orig/mm/migrate.c > +++ linux-2.6/mm/migrate.c > @@ -136,9 +136,13 @@ static int swap_page(struct page *page) > { > struct address_space *mapping = page_mapping(page); > > - if (page_mapped(page) && mapping) > + if (!mapping) > + return -EINVAL; /* page truncated. signal permanent failure */
Here, I think you need to unlock the page too.
> + > + if (page_mapped(page)) { > if (try_to_unmap(page, 1) != SWAP_SUCCESS) > goto unlock_retry; > + } > > if (PageDirty(page)) { > /* Page is dirty, try to write it out here */ > Index: linux-2.6/mm/vmscan.c > =================================================================== > --- linux-2.6.orig/mm/vmscan.c > +++ linux-2.6/mm/vmscan.c > @@ -362,8 +362,8 @@ pageout_t pageout(struct page *page, str > > int remove_mapping(struct address_space *mapping, struct page *page) > { > - if (!mapping) > - return 0; /* truncate got there first */ > + BUG_ON(!PageLocked(page)); > + BUG_ON(mapping != page->mapping); > > write_lock_irq(&mapping->tree_lock); > > @@ -532,7 +532,7 @@ static unsigned long shrink_page_list(st > goto free_it; > } > > - if (!remove_mapping(mapping, page)) > + if (!mapping || !remove_mapping(mapping, page)) > goto keep_locked; > > free_it: > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |