Possible bug in namei.c:page_symlink

Possible bug in namei.c:page_symlink

[Neil Brown]

Looking in 2.6.16-rc2-mm1...

include/linux/fs.h says:

 *
 * @AOP_TRUNCATED_PAGE: The AOP method that was handed a locked page has
 *  			unlocked it and the page might have been truncated.
 *  			The caller should back up to acquiring a new page and
 *  			trying again.  The aop will be taking reasonable
 *  			precautions not to livelock.  If the caller held a page
 *  			reference, it should drop it before retrying.  Returned
 *  			by readpage(), prepare_write(), and commit_write().


so any caller of commit_write should check for AOP_TRUNCATED_PAGE.

However fs/namei.c(page_symlink):

------------------
int page_symlink(struct inode *inode, const char *symname, int len)
{
	struct address_space *mapping = inode->i_mapping;
	struct page *page = grab_cache_page(mapping, 0);
	int err = -ENOMEM;
	char *kaddr;

	if (!page)
		goto fail;
	err = mapping->a_ops->prepare_write(NULL, page, 0, len-1);
	if (err)
		goto fail_map;
	kaddr = kmap_atomic(page, KM_USER0);
	memcpy(kaddr, symname, len-1);
	kunmap_atomic(kaddr, KM_USER0);
	mapping->a_ops->commit_write(NULL, page, 0, len-1);
-------------------

So the return value of commit_write is ignored, and the page is
assumed to be locked by later code.  Equally prepare_write can return
AOP_TRUNCATED_PAGE, but this isn't checked.

So: is there a reason that the following patch is not needed?

NeilBrown

Signed-off-by: Neil Brown <neilb@suse.de>

diff ./fs/namei.c~current~ ./fs/namei.c
--- ./fs/namei.c~current~	2006-02-27 11:40:31.000000000 +1100
+++ ./fs/namei.c	2006-02-27 11:44:07.000000000 +1100
@@ -2612,19 +2612,30 @@ void page_put_link(struct dentry *dentry
 int page_symlink(struct inode *inode, const char *symname, int len)
 {
 	struct address_space *mapping = inode->i_mapping;
-	struct page *page = grab_cache_page(mapping, 0);
 	int err = -ENOMEM;
 	char *kaddr;
 
+ retry:
+	struct page *page = grab_cache_page(mapping, 0);
 	if (!page)
 		goto fail;
 	err = mapping->a_ops->prepare_write(NULL, page, 0, len-1);
+	if (err == AOP_TRUNCATED_PAGE) {
+		page_cache_release(page);
+		goto retry;
+	}
 	if (err)
 		goto fail_map;
 	kaddr = kmap_atomic(page, KM_USER0);
 	memcpy(kaddr, symname, len-1);
 	kunmap_atomic(kaddr, KM_USER0);
-	mapping->a_ops->commit_write(NULL, page, 0, len-1);
+	err = mapping->a_ops->commit_write(NULL, page, 0, len-1);
+	if (err = AOP_TRUNCATED_PAGE) {
+		page_cache_release(page);
+		goto retry;
+	}
+	if (err)
+		goto fail_map;
 	/*
 	 * Notice that we are _not_ going to block here - end of page is
 	 * unmapped, so this will only try to map the rest of page, see
@@ -2634,11 +2645,13 @@ int page_symlink(struct inode *inode, co
 	 */
 	if (!PageUptodate(page)) {
 		err = mapping->a_ops->readpage(NULL, page);
-		wait_on_page_locked(page);
+		if (err != AOP_TRUNCATED_PAGE)
+			wait_on_page_locked(page);
 	} else {
 		unlock_page(page);
 	}
 	page_cache_release(page);
+	} while(0);
 	if (err < 0)
 		goto fail;
 	mark_inode_dirty(inode);