lkml.org 
[lkml]   [1998]   [Jan]   [18]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
DateSun, 18 Jan 1998 12:07:06 -0500
FromBill Hawes <>
Subjectpatch for 2.1.79 mm/filemap.c
The attached patch for filemap.c fixes a problem in generic_file_write
that was causing corruption in certain NFS writes. This was occurring
because pages beyond the current end of file were not being cleared
before being marked up-to-date for a partial write. If the pattern of
writes then left gaps in the file, these would typically contain garbage
from the prior use of the page. (Maybe a security problem as well --
writing bytes sparsely would let you see various pages.)

I've also added a comment about the current practice of setting
PG_uptodate before actually copying the data into the page. I don't
think this is right, but it will require some further analysis because
of the complicated paths in nfs_updatepage. (And to see what other fs
are doing in updatepage.)

Testing the patch using gcc build with make bootstrap and make compare
is now clean.

The patch also includes a fix for a bug in get_cached_page() that was
causing oopses in smbfs -- the page_address() function was being called
for a NULL page, instead of returning 0. I posted this before, but it
hasn't appeared in the pre-80 files so I'm leaving it in this patch.

Regards,
Bill--- mm/filemap.c.old	Tue Jan 13 10:38:36 1998
+++ mm/filemap.c	Sun Jan 18 12:22:15 1998
@@ -1349,11 +1349,10 @@
 		if (!(page = __find_page(inode, pgpos, *hash))) {
 			if (!page_cache) {
 				page_cache = __get_free_page(GFP_KERNEL);
-				if (!page_cache) {
-					status = -ENOMEM;
-					break;
-				}
-				continue;
+				if (page_cache)
+					continue;
+				status = -ENOMEM;
+				break;
 			}
 			page = mem_map + MAP_NR(page_cache);
 			add_to_page_cache(page, inode, pgpos, hash);
@@ -1361,34 +1360,45 @@
 		}
 
 		/*
-		 * WSH 06/05/97: restructured slightly to make sure we release
-		 * the page on an error exit.  Removed explicit setting of
-		 * PG_locked, as that's handled below the i_op->xxx interface.
+		 * Note: setting of the PG_locked bit is handled
+		 * below the i_op->xxx interface.
 		 */
 		didread = 0;
 page_wait:
 		wait_on_page(page);
+		if (PageUptodate(page))
+			goto do_update_page;
 
 		/*
-		 * If the page is not uptodate, and we're writing less
+		 * The page is not up-to-date ... if we're writing less
 		 * than a full page of data, we may have to read it first.
-		 * However, don't bother with reading the page when it's
-		 * after the current end of file.
+		 * But if the page is past the current end of file, we must
+		 * clear it before updating.
 		 */
-		if (!PageUptodate(page)) {
-			if (bytes < PAGE_SIZE && pgpos < inode->i_size) {
-				status = -EIO; /* two tries ... error out */
-				if (didread < 2)
-					status = inode->i_op->readpage(dentry,
-									page);
+		if (bytes < PAGE_SIZE) {
+			if (pgpos < inode->i_size) {
+				status = -EIO;
+				if (didread >= 2)
+					goto done_with_page;
+				status = inode->i_op->readpage(dentry, page);
 				if (status < 0)
 					goto done_with_page;
 				didread++;
 				goto page_wait;
+			} else {
+				/* Must clear for partial writes */
+				memset((void *) page_address(page), 0,
+					 PAGE_SIZE);
 			}
-			set_bit(PG_uptodate, &page->flags);
 		}
+		/*
+		 * N.B. We should defer setting PG_uptodate at least until
+		 * the data is copied. A failure in i_op->updatepage() could
+		 * leave the page with garbage data.
+		 */
+		set_bit(PG_uptodate, &page->flags);
 
+do_update_page:
 		/* Alright, the page is there.  Now update it. */
 		status = inode->i_op->updatepage(dentry, page, buf,
 							offset, bytes, sync);
@@ -1408,9 +1418,7 @@
 
 	if (page_cache)
 		free_page(page_cache);
-	if (written)
-		return written;
-	return status;
+	return written ? written : status;
 }
 
 /*
@@ -1429,7 +1437,7 @@
 {
 	struct page * page;
 	struct page ** hash;
-	unsigned long page_cache;
+	unsigned long page_cache = 0;
 
 	hash = page_hash(inode, offset);
 	page = __find_page(inode, offset, *hash);
@@ -1443,14 +1451,15 @@
 		add_to_page_cache(page, inode, offset, hash);
 	}
 	if (atomic_read(&page->count) != 2)
-		printk("get_cached_page: page count=%d\n",
+		printk(KERN_ERR "get_cached_page: page count=%d\n",
 			atomic_read(&page->count));
 	if (test_bit(PG_locked, &page->flags))
-		printk("get_cached_page: page already locked!\n");
+		printk(KERN_ERR "get_cached_page: page already locked!\n");
 	set_bit(PG_locked, &page->flags);
+	page_cache = page_address(page);
 
 out:
-	return page_address(page);
+	return page_cache;
 }
 
 /*
\
 
 \ /
  Last update: 2005-03-22 12:41    [W:0.324 / U:0.040 seconds]
©2003-2008 Jasper Spaans