From c0cf3ef5e0f47e385920450b245d22bead93e7ad Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 5 Sep 2016 21:42:32 -0400 Subject: [PATCH 1/6] nfs_write_end(): fix handling of short copies What matters when deciding if we should make a page uptodate is not how much we _wanted_ to copy, but how much we actually have copied. As it is, on architectures that do not zero tail on short copy we can leave uninitialized data in page marked uptodate. Cc: stable@vger.kernel.org Signed-off-by: Al Viro --- fs/nfs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 9ea85ae23c32..a1de8ef63e56 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -374,7 +374,7 @@ static int nfs_write_end(struct file *file, struct address_space *mapping, */ if (!PageUptodate(page)) { unsigned pglen = nfs_page_length(page); - unsigned end = offset + len; + unsigned end = offset + copied; if (pglen == 0) { zero_user_segments(page, 0, offset, From b9de313cf05fe08fa59efaf19756ec5283af672a Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 5 Sep 2016 22:20:03 -0400 Subject: [PATCH 2/6] fix ceph_write_end() don't zero on short copies; if the page was uptodate it's just plain wrong, and if it wasn't we'll be better off just returning 0 and buggering off. Signed-off-by: Al Viro --- fs/ceph/addr.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c index ef3ebd780aff..834be0943a26 100644 --- a/fs/ceph/addr.c +++ b/fs/ceph/addr.c @@ -1276,25 +1276,27 @@ static int ceph_write_end(struct file *file, struct address_space *mapping, struct page *page, void *fsdata) { struct inode *inode = file_inode(file); - unsigned from = pos & (PAGE_SIZE - 1); int check_cap = 0; dout("write_end file %p inode %p page %p %d~%d (%d)\n", file, inode, page, (int)pos, (int)copied, (int)len); /* zero the stale part of the page if we did a short copy */ - if (copied < len) - zero_user_segment(page, from+copied, len); + if (!PageUptodate(page)) { + if (copied < len) { + copied = 0; + goto out; + } + SetPageUptodate(page); + } /* did file size increase? */ if (pos+copied > i_size_read(inode)) check_cap = ceph_inode_set_size(inode, pos+copied); - if (!PageUptodate(page)) - SetPageUptodate(page); - set_page_dirty(page); +out: unlock_page(page); put_page(page); From 43388b21e72d36204822bcc3119e42abe6ebceef Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 5 Sep 2016 22:06:35 -0400 Subject: [PATCH 3/6] fix gfs2_stuffed_write_end() on short copies a) the page is uptodate - ->write_begin() would either fail (in which case we don't reach ->write_end()), or unstuff the inode, or find the page already uptodate, or do a successful call of stuffed_readpage(), which would've made it uptodate b) zeroing the tail in pagecache is wrong. kill -9 at the right time while writing unmodified file contents to the same file should _not_ leave us in a situation when read() from the file will be reporting it full of zeroes. Especially since that effect will be transient - at some later point the page will be evicted and then we'll be back to the real file contents. Signed-off-by: Al Viro --- fs/gfs2/aops.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c index 5a6f52ea2722..6b039d7ce160 100644 --- a/fs/gfs2/aops.c +++ b/fs/gfs2/aops.c @@ -839,12 +839,10 @@ static int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh, BUG_ON((pos + len) > (dibh->b_size - sizeof(struct gfs2_dinode))); kaddr = kmap_atomic(page); memcpy(buf + pos, kaddr + pos, copied); - memset(kaddr + pos + copied, 0, len - copied); flush_dcache_page(page); kunmap_atomic(kaddr); - if (!PageUptodate(page)) - SetPageUptodate(page); + WARN_ON(!PageUptodate(page)); unlock_page(page); put_page(page); From 77469c3f570e329acb631c5c03780eacdca2a534 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 29 Aug 2016 20:56:35 -0400 Subject: [PATCH 4/6] 9p: saner ->write_end() on failing copy into non-uptodate page If we had a short copy into an uptodate page, there's no reason whatsoever to zero anything; OTOH, if that page had _not_ been uptodate, we must have been trying to overwrite it completely and got a short copy. In that case, overwriting the end with zeroes, marking uptodate and sending to server is just plain wrong. Just unlock, keep it non-uptodate and return 0. Signed-off-by: Al Viro --- fs/9p/vfs_addr.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c index 6181ad79e1a5..ff8ece89fb99 100644 --- a/fs/9p/vfs_addr.c +++ b/fs/9p/vfs_addr.c @@ -309,18 +309,10 @@ static int v9fs_write_end(struct file *filp, struct address_space *mapping, p9_debug(P9_DEBUG_VFS, "filp %p, mapping %p\n", filp, mapping); - if (unlikely(copied < len)) { - /* - * zero out the rest of the area - */ - unsigned from = pos & (PAGE_SIZE - 1); - - zero_user(page, from + copied, len - copied); - flush_dcache_page(page); + if (unlikely(copied < len && !PageUptodate(page))) { + copied = 0; + goto out; } - - if (!PageUptodate(page)) - SetPageUptodate(page); /* * No need to use i_size_read() here, the i_size * cannot change under us because we hold the i_mutex. @@ -330,6 +322,7 @@ static int v9fs_write_end(struct file *filp, struct address_space *mapping, i_size_write(inode, last_pos); } set_page_dirty(page); +out: unlock_page(page); put_page(page); From 92e50d2d42d7cb9fcd816b47b580622032d38293 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 29 Aug 2016 22:04:51 -0400 Subject: [PATCH 5/6] exofs: don't mess with simple_write_{begin,end} ... and don't zero anything on short copy; just unlock and return 0 if that has happened on non-uptodate page. Signed-off-by: Al Viro --- fs/exofs/inode.c | 68 +++++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c index d8072bc074a4..0ac62811b341 100644 --- a/fs/exofs/inode.c +++ b/fs/exofs/inode.c @@ -870,46 +870,31 @@ int exofs_write_begin(struct file *file, struct address_space *mapping, page = *pagep; if (page == NULL) { - ret = simple_write_begin(file, mapping, pos, len, flags, pagep, - fsdata); - if (ret) { - EXOFS_DBGMSG("simple_write_begin failed\n"); - goto out; + page = grab_cache_page_write_begin(mapping, pos >> PAGE_SHIFT, + flags); + if (!page) { + EXOFS_DBGMSG("grab_cache_page_write_begin failed\n"); + return -ENOMEM; } - - page = *pagep; + *pagep = page; } /* read modify write */ if (!PageUptodate(page) && (len != PAGE_SIZE)) { loff_t i_size = i_size_read(mapping->host); pgoff_t end_index = i_size >> PAGE_SHIFT; - size_t rlen; - if (page->index < end_index) - rlen = PAGE_SIZE; - else if (page->index == end_index) - rlen = i_size & ~PAGE_MASK; - else - rlen = 0; - - if (!rlen) { + if (page->index > end_index) { clear_highpage(page); SetPageUptodate(page); - goto out; - } - - ret = _readpage(page, true); - if (ret) { - /*SetPageError was done by _readpage. Is it ok?*/ - unlock_page(page); - EXOFS_DBGMSG("__readpage failed\n"); + } else { + ret = _readpage(page, true); + if (ret) { + unlock_page(page); + EXOFS_DBGMSG("__readpage failed\n"); + } } } -out: - if (unlikely(ret)) - _write_failed(mapping->host, pos + len); - return ret; } @@ -929,18 +914,25 @@ static int exofs_write_end(struct file *file, struct address_space *mapping, struct page *page, void *fsdata) { struct inode *inode = mapping->host; - /* According to comment in simple_write_end i_mutex is held */ - loff_t i_size = inode->i_size; - int ret; + loff_t last_pos = pos + copied; - ret = simple_write_end(file, mapping,pos, len, copied, page, fsdata); - if (unlikely(ret)) - _write_failed(inode, pos + len); - - /* TODO: once simple_write_end marks inode dirty remove */ - if (i_size != inode->i_size) + if (!PageUptodate(page)) { + if (copied < len) { + _write_failed(inode, pos + len); + copied = 0; + goto out; + } + SetPageUptodate(page); + } + if (last_pos > inode->i_size) { + i_size_write(inode, last_pos); mark_inode_dirty(inode); - return ret; + } + set_page_dirty(page); +out: + unlock_page(page); + put_page(page); + return copied; } static int exofs_releasepage(struct page *page, gfp_t gfp) From 04fff6416cb7876091f0b2f413caf43e3618d5ad Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 29 Aug 2016 22:39:56 -0400 Subject: [PATCH 6/6] simple_write_end(): don't zero in short copy into uptodate Signed-off-by: Al Viro --- fs/libfs.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/fs/libfs.c b/fs/libfs.c index 48826d4da189..76048705d922 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -465,6 +465,8 @@ EXPORT_SYMBOL(simple_write_begin); * is not called, so a filesystem that actually does store data in .write_inode * should extend on what's done here with a call to mark_inode_dirty() in the * case that i_size has changed. + * + * Use *ONLY* with simple_readpage() */ int simple_write_end(struct file *file, struct address_space *mapping, loff_t pos, unsigned len, unsigned copied, @@ -474,14 +476,14 @@ int simple_write_end(struct file *file, struct address_space *mapping, loff_t last_pos = pos + copied; /* zero the stale part of the page if we did a short copy */ - if (copied < len) { - unsigned from = pos & (PAGE_SIZE - 1); + if (!PageUptodate(page)) { + if (copied < len) { + unsigned from = pos & (PAGE_SIZE - 1); - zero_user(page, from + copied, len - copied); - } - - if (!PageUptodate(page)) + zero_user(page, from + copied, len - copied); + } SetPageUptodate(page); + } /* * No need to use i_size_read() here, the i_size * cannot change under us because we hold the i_mutex.