From: Nick Piggin Some prepare/commit_write implementations have possible pre-existing bugs, possible data leaks (by setting uptodate too early) and data corruption (by not reading in non-modified parts of a !uptodate page). Others are (also) broken when commit_write passes in a 0 length commit with a !uptodate page (a change caused by buffered write deadlock fix patch). Fix filesystems as best we can. GFS2, OCFS2, Reiserfs, JFFS are nontrivial and are likely broken. All others at least need a glance. Also, update vfs.txt to describe the prepare_write/commit_write changes. Signed-off-by: Nick Piggin Signed-off-by: Andrew Morton --- Documentation/filesystems/vfs.txt | 28 ++++++++---- fs/affs/file.c | 4 + fs/cifs/file.c | 43 ++++++------------- fs/ecryptfs/mmap.c | 17 +++---- fs/ext3/inode.c | 63 +++++++++++++++------------- fs/fat/inode.c | 6 ++ fs/fuse/file.c | 3 + fs/hostfs/hostfs_kern.c | 13 +++++ fs/jffs/inode-v23.c | 11 ++++ fs/jffs2/file.c | 3 + fs/nfs/file.c | 3 + fs/smbfs/file.c | 9 ++++ fs/udf/file.c | 7 +++ 13 files changed, 130 insertions(+), 80 deletions(-) diff -puN Documentation/filesystems/vfs.txt~fs-prepare_write-fixes Documentation/filesystems/vfs.txt --- a/Documentation/filesystems/vfs.txt~fs-prepare_write-fixes +++ a/Documentation/filesystems/vfs.txt @@ -607,22 +607,32 @@ struct address_space_operations { the given range of bytes is about to be written. The address_space should check that the write will be able to complete, by allocating space if necessary and doing any other - internal housekeeping. If the write will update parts of - any basic-blocks on storage, then those blocks should be - pre-read (if they haven't been read already) so that the + internal housekeeping. If the write will *not* update any + parts of any basic-blocks of the page, then those blocks should + be pre-read (if they are not already uptodate) so that the updated blocks can be written out properly. The page will be locked. If prepare_write wants to unlock the page it, like readpage, may do so and return AOP_TRUNCATED_PAGE. In this case the prepare_write will be retried one the lock is - regained. + regained.a + prepare_write should *not* set the page uptodate unless it has + actually been filled with valid data (eg. that may be returned + with a read(2) syscall). commit_write: If prepare_write succeeds, new data will be copied - into the page and then commit_write will be called. It will - typically update the size of the file (if appropriate) and - mark the inode as dirty, and do any other related housekeeping - operations. It should avoid returning an error if possible - - errors should have been handled by prepare_write. + into the page and then commit_write will be called. commit_write may + be called with a range that is smaller than that passed in to + prepare_write, it could even be zero. If the page is not uptodate, + the range will *only* be zero or the full length that was passed to + prepare_write, if it is zero, the page should not be marked uptodate + (success should still be returned, if possible -- the write will be + retried). + commit_write will typically update the size of the file (if + appropriate) and mark the inode as dirty, and do any other related + housekeeping operations. A successful commit_write should mark the page + uptodate if it is not already. It should avoid returning an error if + possible - errors should have been handled by prepare_write. bmap: called by the VFS to map a logical block offset within object to physical block number. This method is used by the FIBMAP diff -puN fs/affs/file.c~fs-prepare_write-fixes fs/affs/file.c --- a/fs/affs/file.c~fs-prepare_write-fixes +++ a/fs/affs/file.c @@ -655,6 +655,10 @@ static int affs_commit_write_ofs(struct int written; pr_debug("AFFS: commit_write(%u, %ld, %d, %d)\n", (u32)inode->i_ino, page->index, from, to); + + if (to - from == 0) + return 0; + bsize = AFFS_SB(sb)->s_data_blksize; data = page_address(page); diff -puN fs/cifs/file.c~fs-prepare_write-fixes fs/cifs/file.c --- a/fs/cifs/file.c~fs-prepare_write-fixes +++ a/fs/cifs/file.c @@ -1365,6 +1365,9 @@ static int cifs_commit_write(struct file loff_t position = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; char *page_data; + if (to - offset == 0) + return 0; + xid = GetXid(); cFYI(1, ("commit write for page %p up to position %lld for %d", page, position, to)); @@ -1418,8 +1421,6 @@ static int cifs_commit_write(struct file rc = 0; /* else if (rc < 0) should we set writebehind rc? */ kunmap(page); - } else { - set_page_dirty(page); } FreeXid(xid); @@ -1973,36 +1974,20 @@ int is_size_safe_to_change(struct cifsIn static int cifs_prepare_write(struct file *file, struct page *page, unsigned from, unsigned to) { - int rc = 0; loff_t offset = (loff_t)page->index << PAGE_CACHE_SHIFT; cFYI(1, ("prepare write for page %p from %d to %d",page,from,to)); - if (!PageUptodate(page)) { - /* if (to - from != PAGE_CACHE_SIZE) { - void *kaddr = kmap_atomic(page, KM_USER0); - memset(kaddr, 0, from); - memset(kaddr + to, 0, PAGE_CACHE_SIZE - to); - flush_dcache_page(page); - kunmap_atomic(kaddr, KM_USER0); - } */ - /* If we are writing a full page it will be up to date, - no need to read from the server */ - if ((to == PAGE_CACHE_SIZE) && (from == 0)) - SetPageUptodate(page); - - /* might as well read a page, it is fast enough */ - if ((file->f_flags & O_ACCMODE) != O_WRONLY) { - rc = cifs_readpage_worker(file, page, &offset); - } else { - /* should we try using another file handle if there is one - - how would we lock it to prevent close of that handle - racing with this read? - In any case this will be written out by commit_write */ - } - } - /* BB should we pass any errors back? - e.g. if we do not have read access to the file */ - return 0; + if (PageUptodate(page)) + return 0; + if (to - from == PAGE_CACHE_SIZE) + return 0; + + /* might as well read a page, it is fast enough */ + if ((file->f_flags & O_ACCMODE) != O_WRONLY) + return cifs_readpage_worker(file, page, &offset); + + printk("CIFS does not yet support partial page writes on O_WRONLY files\n"); + return -EIO; } const struct address_space_operations cifs_addr_ops = { diff -puN fs/ecryptfs/mmap.c~fs-prepare_write-fixes fs/ecryptfs/mmap.c --- a/fs/ecryptfs/mmap.c~fs-prepare_write-fixes +++ a/fs/ecryptfs/mmap.c @@ -337,16 +337,12 @@ out: static int ecryptfs_prepare_write(struct file *file, struct page *page, unsigned from, unsigned to) { - int rc = 0; - kmap(page); - if (from == 0 && to == PAGE_CACHE_SIZE) - goto out; /* If we are writing a full page, it will be - up to date. */ - if (!PageUptodate(page)) - rc = ecryptfs_do_readpage(file, page, page->index); -out: - return rc; + if (PageUptodate(page)) + return 0; + if (to - from == PAGE_CACHE_SIZE) + return 0; + return ecryptfs_do_readpage(file, page, page->index); } int ecryptfs_grab_and_map_lower_page(struct page **lower_page, @@ -634,6 +630,9 @@ static int ecryptfs_commit_write(struct struct ecryptfs_crypt_stat *crypt_stat; int rc; + if (to - from == 0) + return 0; + inode = page->mapping->host; lower_inode = ecryptfs_inode_to_lower(inode); lower_file = ecryptfs_file_to_lower(file); diff -puN fs/ext3/inode.c~fs-prepare_write-fixes fs/ext3/inode.c --- a/fs/ext3/inode.c~fs-prepare_write-fixes +++ a/fs/ext3/inode.c @@ -1214,21 +1214,24 @@ static int ext3_ordered_commit_write(str struct inode *inode = page->mapping->host; int ret = 0, ret2; - ret = walk_page_buffers(handle, page_buffers(page), - from, to, NULL, ext3_journal_dirty_data); + if (to - from > 0) { + ret = walk_page_buffers(handle, page_buffers(page), + from, to, NULL, ext3_journal_dirty_data); - if (ret == 0) { - /* - * generic_commit_write() will run mark_inode_dirty() if i_size - * changes. So let's piggyback the i_disksize mark_inode_dirty - * into that. - */ - loff_t new_i_size; + if (ret == 0) { + /* + * generic_commit_write() will run mark_inode_dirty() + * if i_size changes. So let's piggyback the + * i_disksize mark_inode_dirty into that. + */ + loff_t new_i_size; - new_i_size = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; - if (new_i_size > EXT3_I(inode)->i_disksize) - EXT3_I(inode)->i_disksize = new_i_size; - ret = generic_commit_write(file, page, from, to); + new_i_size = ((loff_t)page->index << PAGE_CACHE_SHIFT) + + to; + if (new_i_size > EXT3_I(inode)->i_disksize) + EXT3_I(inode)->i_disksize = new_i_size; + ret = generic_commit_write(file, page, from, to); + } } ret2 = ext3_journal_stop(handle); if (!ret) @@ -1268,23 +1271,25 @@ static int ext3_journalled_commit_write( int partial = 0; loff_t pos; - /* - * Here we duplicate the generic_commit_write() functionality - */ - pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; + if (to - from > 0) { + /* + * Here we duplicate the generic_commit_write() functionality + */ + pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to; - ret = walk_page_buffers(handle, page_buffers(page), from, - to, &partial, commit_write_fn); - if (!partial) - SetPageUptodate(page); - if (pos > inode->i_size) - i_size_write(inode, pos); - EXT3_I(inode)->i_state |= EXT3_STATE_JDATA; - if (inode->i_size > EXT3_I(inode)->i_disksize) { - EXT3_I(inode)->i_disksize = inode->i_size; - ret2 = ext3_mark_inode_dirty(handle, inode); - if (!ret) - ret = ret2; + ret = walk_page_buffers(handle, page_buffers(page), from, + to, &partial, commit_write_fn); + if (!partial) + SetPageUptodate(page); + if (pos > inode->i_size) + i_size_write(inode, pos); + EXT3_I(inode)->i_state |= EXT3_STATE_JDATA; + if (inode->i_size > EXT3_I(inode)->i_disksize) { + EXT3_I(inode)->i_disksize = inode->i_size; + ret2 = ext3_mark_inode_dirty(handle, inode); + if (!ret) + ret = ret2; + } } ret2 = ext3_journal_stop(handle); if (!ret) diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c --- a/fs/fat/inode.c~fs-prepare_write-fixes +++ a/fs/fat/inode.c @@ -150,7 +150,11 @@ static int fat_commit_write(struct file unsigned from, unsigned to) { struct inode *inode = page->mapping->host; - int err = generic_commit_write(file, page, from, to); + int err; + if (to - from > 0) + return 0; + + err = generic_commit_write(file, page, from, to); if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) { inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC; MSDOS_I(inode)->i_attrs |= ATTR_ARCH; diff -puN fs/fuse/file.c~fs-prepare_write-fixes fs/fuse/file.c --- a/fs/fuse/file.c~fs-prepare_write-fixes +++ a/fs/fuse/file.c @@ -467,6 +467,9 @@ static int fuse_commit_write(struct file if (is_bad_inode(inode)) return -EIO; + if (to - from == 0) + return 0; + req = fuse_get_req(fc); if (IS_ERR(req)) return PTR_ERR(req); diff -puN fs/hostfs/hostfs_kern.c~fs-prepare_write-fixes fs/hostfs/hostfs_kern.c --- a/fs/hostfs/hostfs_kern.c~fs-prepare_write-fixes +++ a/fs/hostfs/hostfs_kern.c @@ -469,6 +469,11 @@ int hostfs_prepare_write(struct file *fi long long start, tmp; int err; + if (PageUptodate(page)) + return 0; + if (to - from == PAGE_CACHE_SIZE) + return 0; + start = (long long) page->index << PAGE_CACHE_SHIFT; buffer = kmap(page); if(from != 0){ @@ -498,10 +503,14 @@ int hostfs_commit_write(struct file *fil long long start; int err = 0; + if (to - from == 0) + return 0; + start = (((long long) page->index) << PAGE_CACHE_SHIFT) + from; buffer = kmap(page); err = write_file(FILE_HOSTFS_I(file)->fd, &start, buffer + from, to - from); + kunmap(page); if(err > 0) err = 0; /* Actually, if !err, write_file has added to-from to start, so, despite @@ -511,7 +520,9 @@ int hostfs_commit_write(struct file *fil if(!err && (start > inode->i_size)) inode->i_size = start; - kunmap(page); + if (!PageUptodate(page) && to - from == PAGE_CACHE_SIZE) + SetPageUptodate(page); + return(err); } diff -puN fs/jffs/inode-v23.c~fs-prepare_write-fixes fs/jffs/inode-v23.c --- a/fs/jffs/inode-v23.c~fs-prepare_write-fixes +++ a/fs/jffs/inode-v23.c @@ -1531,7 +1531,7 @@ jffs_prepare_write(struct file *filp, st /* FIXME: we should detect some error conditions here */ /* Bugger that. We should make sure the page is uptodate */ - if (!PageUptodate(page) && (from || to < PAGE_CACHE_SIZE)) + if (!PageUptodate(page) && (to - from < PAGE_CACHE_SIZE)) return jffs_do_readpage_nolock(filp, page); return 0; @@ -1541,11 +1541,18 @@ static int jffs_commit_write(struct file *filp, struct page *page, unsigned from, unsigned to) { + int ret; + void *addr = page_address(page) + from; /* XXX: PAGE_CACHE_SHIFT or PAGE_SHIFT */ loff_t pos = page_offset(page) + from; - return jffs_file_write(filp, addr, to-from, &pos); + if (to - from == 0) + return 0; + + ret = jffs_file_write(filp, addr, to-from, &pos); + if (!PageUptodate(page) && (to - from == PAGE_CACHE_SIZE)) + SetPageUptodate(page); } /* jffs_commit_write() */ /* This is our ioctl() routine. */ diff -puN fs/jffs2/file.c~fs-prepare_write-fixes fs/jffs2/file.c --- a/fs/jffs2/file.c~fs-prepare_write-fixes +++ a/fs/jffs2/file.c @@ -222,6 +222,9 @@ static int jffs2_commit_write (struct fi D1(printk(KERN_DEBUG "jffs2_commit_write(): ino #%lu, page at 0x%lx, range %d-%d, flags %lx\n", inode->i_ino, pg->index << PAGE_CACHE_SHIFT, start, end, pg->flags)); + if (end - start == 0) + return 0; + if (end == PAGE_CACHE_SIZE) { if (!start) { /* We need to avoid deadlock with page_cache_read() in diff -puN fs/nfs/file.c~fs-prepare_write-fixes fs/nfs/file.c --- a/fs/nfs/file.c~fs-prepare_write-fixes +++ a/fs/nfs/file.c @@ -299,6 +299,9 @@ static int nfs_commit_write(struct file { long status; + if (to - offset == 0) + return 0; + lock_kernel(); status = nfs_updatepage(file, page, offset, to-offset); unlock_kernel(); diff -puN fs/smbfs/file.c~fs-prepare_write-fixes fs/smbfs/file.c --- a/fs/smbfs/file.c~fs-prepare_write-fixes +++ a/fs/smbfs/file.c @@ -293,6 +293,10 @@ out: static int smb_prepare_write(struct file *file, struct page *page, unsigned offset, unsigned to) { + if (!PageUptodate(page) && to - offset < PAGE_CACHE_SIZE) { + printk("SMB does not yet support partial page writes\n"); + return -EIO; + } return 0; } @@ -301,10 +305,15 @@ static int smb_commit_write(struct file { int status; + if (to - offset == 0) + return 0; + status = -EFAULT; lock_kernel(); status = smb_updatepage(file, page, offset, to-offset); unlock_kernel(); + if (!PageUptodate(page) && to - offset == PAGE_CACHE_SIZE) + SetPageUptodate(page); return status; } diff -puN fs/udf/file.c~fs-prepare_write-fixes fs/udf/file.c --- a/fs/udf/file.c~fs-prepare_write-fixes +++ a/fs/udf/file.c @@ -75,6 +75,10 @@ static int udf_adinicb_writepage(struct static int udf_adinicb_prepare_write(struct file *file, struct page *page, unsigned offset, unsigned to) { + if (!PageUptodate(page) && to - offset < PAGE_CACHE_SIZE) { + printk("UDF does not yet support partial page writes\n"); + return -EIO; + } kmap(page); return 0; } @@ -84,6 +88,9 @@ static int udf_adinicb_commit_write(stru struct inode *inode = page->mapping->host; char *kaddr = page_address(page); + if (to - offset == 0) + return 0; + memcpy(UDF_I_DATA(inode) + UDF_I_LENEATTR(inode) + offset, kaddr + offset, to - offset); mark_inode_dirty(inode); _