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 <[email protected]>
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 [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
