From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Brown Subject: Possible bug in namei.c:page_symlink Date: Mon, 27 Feb 2006 12:44:04 +1100 Message-ID: <17410.22884.225309.469100@cse.unsw.edu.au> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Return-path: Received: from cantor2.suse.de ([195.135.220.15]:27567 "EHLO mx2.suse.de") by vger.kernel.org with ESMTP id S1750826AbWB0Bot (ORCPT ); Sun, 26 Feb 2006 20:44:49 -0500 Received: from Relay2.suse.de (mail2.suse.de [195.135.221.8]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 8497F1CC3B for ; Mon, 27 Feb 2006 02:44:48 +0100 (CET) To: linux-fsdevel@vger.kernel.org Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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 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);