[PATCH] Fix read() vs truncate race

do_generic_mapping_read()
{
	isize1 = i_size_read();
	...
	readpage
	copy_to_user up to isize1;
}

readpage()
{
	isize2 = i_size_read();
	...
	read blocks
	...
	zero-fill all blocks past isize2
}

If a second thread runs truncate and shrinks i_size, so isize1 and isize2 are
different, the read can return up to a page of zero-fill that shouldn't really
exist.

The trick is to read isize1 after doing the readpage.  I realised this is the
right way to do it without having to change the readpage API.

The patch should not cost any cycles when reading from pagecache.

Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

1 files changed, 50 insertions, 26 deletions

diff --git a/mm/filemap.c b/mm/filemap.c
index 6e59faaa6afb06..8474008f290f9a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -650,7 +650,8 @@ void do_generic_mapping_read(struct address_space *mapping,
 			     read_actor_t actor)
 {
 	struct inode *inode = mapping->host;
-	unsigned long index, offset;
+	unsigned long index, end_index, offset;
+	loff_t isize;
 	struct page *cached_page;
 	int error;
 	struct file_ra_state ra = *_ra;
@@ -659,26 +660,18 @@ void do_generic_mapping_read(struct address_space *mapping,
 	index = *ppos >> PAGE_CACHE_SHIFT;
 	offset = *ppos & ~PAGE_CACHE_MASK;
 
+	isize = i_size_read(inode);
+	end_index = isize >> PAGE_CACHE_SHIFT;
+	if (index > end_index)
+		goto out;
+
 	for (;;) {
 		struct page *page;
-		unsigned long end_index, nr, ret;
-		loff_t isize = i_size_read(inode);
-
-		end_index = isize >> PAGE_CACHE_SHIFT;
-			
-		if (index > end_index)
-			break;
-		nr = PAGE_CACHE_SIZE;
-		if (index == end_index) {
-			nr = isize & ~PAGE_CACHE_MASK;
-			if (nr <= offset)
-				break;
-		}
+		unsigned long nr, ret;
 
 		cond_resched();
 		page_cache_readahead(mapping, &ra, filp, index);
 
-		nr = nr - offset;
 find_page:
 		page = find_get_page(mapping, index);
 		if (unlikely(page == NULL)) {
@@ -688,6 +681,17 @@ find_page:
 		if (!PageUptodate(page))
 			goto page_not_up_to_date;
 page_ok:
+		/* nr is the maximum number of bytes to copy from this page */
+		nr = PAGE_CACHE_SIZE;
+		if (index == end_index) {
+			nr = isize & ~PAGE_CACHE_MASK;
+			if (nr <= offset) {
+				page_cache_release(page);
+				goto out;
+			}
+		}
+		nr = nr - offset;
+
 		/* If users can be writing to this page using arbitrary
 		 * virtual addresses, take care about potential aliasing
 		 * before reading the page on the kernel side.
@@ -719,7 +723,7 @@ page_ok:
 		page_cache_release(page);
 		if (ret == nr && desc->count)
 			continue;
-		break;
+		goto out;
 
 page_not_up_to_date:
 		/* Get exclusive access to the page ... */
@@ -739,22 +743,41 @@ page_not_up_to_date:
 		}
 
 readpage:
-		/* ... and start the actual read. The read will unlock the page. */
+		/* Start the actual read. The read will unlock the page. */
 		error = mapping->a_ops->readpage(filp, page);
 
-		if (!error) {
-			if (PageUptodate(page))
-				goto page_ok;
+		if (unlikely(error))
+			goto readpage_error;
+
+		if (!PageUptodate(page)) {
 			wait_on_page_locked(page);
-			if (PageUptodate(page))
-				goto page_ok;
-			error = -EIO;
+			if (!PageUptodate(page)) {
+				error = -EIO;
+				goto readpage_error;
+			}
+		}
+
+		/*
+		 * i_size must be checked after we have done ->readpage.
+		 *
+		 * Checking i_size after the readpage allows us to calculate
+		 * the correct value for "nr", which means the zero-filled
+		 * part of the page is not copied back to userspace (unless
+		 * another truncate extends the file - this is desired though).
+		 */
+		isize = i_size_read(inode);
+		end_index = isize >> PAGE_CACHE_SHIFT;
+		if (index > end_index) {
+			page_cache_release(page);
+			goto out;
 		}
+		goto page_ok;
 
+readpage_error:
 		/* UHHUH! A synchronous read error occurred. Report it */
 		desc->error = error;
 		page_cache_release(page);
-		break;
+		goto out;
 
 no_cached_page:
 		/*
@@ -765,7 +788,7 @@ no_cached_page:
 			cached_page = page_cache_alloc_cold(mapping);
 			if (!cached_page) {
 				desc->error = -ENOMEM;
-				break;
+				goto out;
 			}
 		}
 		error = add_to_page_cache_lru(cached_page, mapping,
@@ -774,13 +797,14 @@ no_cached_page:
 			if (error == -EEXIST)
 				goto find_page;
 			desc->error = error;
-			break;
+			goto out;
 		}
 		page = cached_page;
 		cached_page = NULL;
 		goto readpage;
 	}
 
+out:
 	*_ra = ra;
 
 	*ppos = ((loff_t) index << PAGE_CACHE_SHIFT) + offset;