From 4a4ac4eba1010ef9a804569058ab29e3450c0315 Mon Sep 17 00:00:00 2001 From: Maxim Patlasov Date: Mon, 12 Aug 2013 20:39:30 +0400 Subject: fuse: postpone end_page_writeback() in fuse_writepage_locked() The patch fixes a race between ftruncate(2), mmap-ed write and write(2): 1) An user makes a page dirty via mmap-ed write. 2) The user performs shrinking truncate(2) intended to purge the page. 3) Before fuse_do_setattr calls truncate_pagecache, the page goes to writeback. fuse_writepage_locked fills FUSE_WRITE request and releases the original page by end_page_writeback. 4) fuse_do_setattr() completes and successfully returns. Since now, i_mutex is free. 5) Ordinary write(2) extends i_size back to cover the page. Note that fuse_send_write_pages do wait for fuse writeback, but for another page->index. 6) fuse_writepage_locked proceeds by queueing FUSE_WRITE request. fuse_send_writepage is supposed to crop inarg->size of the request, but it doesn't because i_size has already been extended back. Moving end_page_writeback to the end of fuse_writepage_locked fixes the race because now the fact that truncate_pagecache is successfully returned infers that fuse_writepage_locked has already called end_page_writeback. And this, in turn, infers that fuse_flush_writepages has already called fuse_send_writepage, and the latter used valid (shrunk) i_size. write(2) could not extend it because of i_mutex held by ftruncate(2). Signed-off-by: Maxim Patlasov Signed-off-by: Miklos Szeredi Cc: stable@vger.kernel.org --- fs/fuse/file.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 5c121fe19c5f..d1715b30f6c4 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1529,7 +1529,6 @@ static int fuse_writepage_locked(struct page *page) inc_bdi_stat(mapping->backing_dev_info, BDI_WRITEBACK); inc_zone_page_state(tmp_page, NR_WRITEBACK_TEMP); - end_page_writeback(page); spin_lock(&fc->lock); list_add(&req->writepages_entry, &fi->writepages); @@ -1537,6 +1536,8 @@ static int fuse_writepage_locked(struct page *page) fuse_flush_writepages(inode); spin_unlock(&fc->lock); + end_page_writeback(page); + return 0; err_free: -- cgit v1.2.3-59-g8ed1b From d331a415aef98717393dda0be69b7947da08eba3 Mon Sep 17 00:00:00 2001 From: Anand Avati Date: Tue, 20 Aug 2013 02:21:07 -0400 Subject: fuse: invalidate inode attributes on xattr modification Calls like setxattr and removexattr result in updation of ctime. Therefore invalidate inode attributes to force a refresh. Signed-off-by: Anand Avati Reviewed-by: Brian Foster Signed-off-by: Miklos Szeredi Cc: stable@vger.kernel.org --- fs/fuse/dir.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs') diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 72a5d5b04494..c49b8c722e27 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1749,6 +1749,8 @@ static int fuse_setxattr(struct dentry *entry, const char *name, fc->no_setxattr = 1; err = -EOPNOTSUPP; } + if (!err) + fuse_invalidate_attr(inode); return err; } @@ -1878,6 +1880,8 @@ static int fuse_removexattr(struct dentry *entry, const char *name) fc->no_removexattr = 1; err = -EOPNOTSUPP; } + if (!err) + fuse_invalidate_attr(inode); return err; } -- cgit v1.2.3-59-g8ed1b From 06a7c3c2781409af95000c60a5df743fd4e2f8b4 Mon Sep 17 00:00:00 2001 From: Maxim Patlasov Date: Fri, 30 Aug 2013 17:06:04 +0400 Subject: fuse: hotfix truncate_pagecache() issue The way how fuse calls truncate_pagecache() from fuse_change_attributes() is completely wrong. Because, w/o i_mutex held, we never sure whether 'oldsize' and 'attr->size' are valid by the time of execution of truncate_pagecache(inode, oldsize, attr->size). In fact, as soon as we released fc->lock in the middle of fuse_change_attributes(), we completely loose control of actions which may happen with given inode until we reach truncate_pagecache. The list of potentially dangerous actions includes mmap-ed reads and writes, ftruncate(2) and write(2) extending file size. The typical outcome of doing truncate_pagecache() with outdated arguments is data corruption from user point of view. This is (in some sense) acceptable in cases when the issue is triggered by a change of the file on the server (i.e. externally wrt fuse operation), but it is absolutely intolerable in scenarios when a single fuse client modifies a file without any external intervention. A real life case I discovered by fsx-linux looked like this: 1. Shrinking ftruncate(2) comes to fuse_do_setattr(). The latter sends FUSE_SETATTR to the server synchronously, but before getting fc->lock ... 2. fuse_dentry_revalidate() is asynchronously called. It sends FUSE_LOOKUP to the server synchronously, then calls fuse_change_attributes(). The latter updates i_size, releases fc->lock, but before comparing oldsize vs attr->size.. 3. fuse_do_setattr() from the first step proceeds by acquiring fc->lock and updating attributes and i_size, but now oldsize is equal to outarg.attr.size because i_size has just been updated (step 2). Hence, fuse_do_setattr() returns w/o calling truncate_pagecache(). 4. As soon as ftruncate(2) completes, the user extends file size by write(2) making a hole in the middle of file, then reads data from the hole either by read(2) or mmap-ed read. The user expects to get zero data from the hole, but gets stale data because truncate_pagecache() is not executed yet. The scenario above illustrates one side of the problem: not truncating the page cache even though we should. Another side corresponds to truncating page cache too late, when the state of inode changed significantly. Theoretically, the following is possible: 1. As in the previous scenario fuse_dentry_revalidate() discovered that i_size changed (due to our own fuse_do_setattr()) and is going to call truncate_pagecache() for some 'new_size' it believes valid right now. But by the time that particular truncate_pagecache() is called ... 2. fuse_do_setattr() returns (either having called truncate_pagecache() or not -- it doesn't matter). 3. The file is extended either by write(2) or ftruncate(2) or fallocate(2). 4. mmap-ed write makes a page in the extended region dirty. The result will be the lost of data user wrote on the fourth step. The patch is a hotfix resolving the issue in a simplistic way: let's skip dangerous i_size update and truncate_pagecache if an operation changing file size is in progress. This simplistic approach looks correct for the cases w/o external changes. And to handle them properly, more sophisticated and intrusive techniques (e.g. NFS-like one) would be required. I'd like to postpone it until the issue is well discussed on the mailing list(s). Changed in v2: - improved patch description to cover both sides of the issue. Signed-off-by: Maxim Patlasov Signed-off-by: Miklos Szeredi Cc: stable@vger.kernel.org --- fs/fuse/dir.c | 7 ++++++- fs/fuse/file.c | 8 +++++++- fs/fuse/fuse_i.h | 2 ++ fs/fuse/inode.c | 3 ++- 4 files changed, 17 insertions(+), 3 deletions(-) (limited to 'fs') diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index c49b8c722e27..c8334f75c8c9 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1590,6 +1590,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr, struct file *file) { struct fuse_conn *fc = get_fuse_conn(inode); + struct fuse_inode *fi = get_fuse_inode(inode); struct fuse_req *req; struct fuse_setattr_in inarg; struct fuse_attr_out outarg; @@ -1617,8 +1618,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr, if (IS_ERR(req)) return PTR_ERR(req); - if (is_truncate) + if (is_truncate) { fuse_set_nowrite(inode); + set_bit(FUSE_I_SIZE_UNSTABLE, &fi->state); + } memset(&inarg, 0, sizeof(inarg)); memset(&outarg, 0, sizeof(outarg)); @@ -1680,12 +1683,14 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr, invalidate_inode_pages2(inode->i_mapping); } + clear_bit(FUSE_I_SIZE_UNSTABLE, &fi->state); return 0; error: if (is_truncate) fuse_release_nowrite(inode); + clear_bit(FUSE_I_SIZE_UNSTABLE, &fi->state); return err; } diff --git a/fs/fuse/file.c b/fs/fuse/file.c index d1715b30f6c4..d409deafc67b 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -629,7 +629,8 @@ static void fuse_read_update_size(struct inode *inode, loff_t size, struct fuse_inode *fi = get_fuse_inode(inode); spin_lock(&fc->lock); - if (attr_ver == fi->attr_version && size < inode->i_size) { + if (attr_ver == fi->attr_version && size < inode->i_size && + !test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) { fi->attr_version = ++fc->attr_version; i_size_write(inode, size); } @@ -1032,12 +1033,16 @@ static ssize_t fuse_perform_write(struct file *file, { struct inode *inode = mapping->host; struct fuse_conn *fc = get_fuse_conn(inode); + struct fuse_inode *fi = get_fuse_inode(inode); int err = 0; ssize_t res = 0; if (is_bad_inode(inode)) return -EIO; + if (inode->i_size < pos + iov_iter_count(ii)) + set_bit(FUSE_I_SIZE_UNSTABLE, &fi->state); + do { struct fuse_req *req; ssize_t count; @@ -1073,6 +1078,7 @@ static ssize_t fuse_perform_write(struct file *file, if (res > 0) fuse_write_update_size(inode, pos); + clear_bit(FUSE_I_SIZE_UNSTABLE, &fi->state); fuse_invalidate_attr(inode); return res > 0 ? res : err; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index fde7249a3a96..5ced199b50bb 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -115,6 +115,8 @@ struct fuse_inode { enum { /** Advise readdirplus */ FUSE_I_ADVISE_RDPLUS, + /** An operation changing file size is in progress */ + FUSE_I_SIZE_UNSTABLE, }; struct fuse_conn; diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 0b578598c6ac..e0fe703ee3d6 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -201,7 +201,8 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr, struct timespec old_mtime; spin_lock(&fc->lock); - if (attr_version != 0 && fi->attr_version > attr_version) { + if ((attr_version != 0 && fi->attr_version > attr_version) || + test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) { spin_unlock(&fc->lock); return; } -- cgit v1.2.3-59-g8ed1b From efeb9e60d48f7778fdcad4a0f3ad9ea9b19e5dfd Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Tue, 3 Sep 2013 14:28:38 +0200 Subject: fuse: readdir: check for slash in names Userspace can add names containing a slash character to the directory listing. Don't allow this as it could cause all sorts of trouble. Signed-off-by: Miklos Szeredi Cc: stable@vger.kernel.org --- fs/fuse/dir.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs') diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index c8334f75c8c9..8fec28ff4a0d 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1174,6 +1174,8 @@ static int parse_dirfile(char *buf, size_t nbytes, struct file *file, return -EIO; if (reclen > nbytes) break; + if (memchr(dirent->name, '/', dirent->namelen) != NULL) + return -EIO; if (!dir_emit(ctx, dirent->name, dirent->namelen, dirent->ino, dirent->type)) @@ -1320,6 +1322,8 @@ static int parse_dirplusfile(char *buf, size_t nbytes, struct file *file, return -EIO; if (reclen > nbytes) break; + if (memchr(dirent->name, '/', dirent->namelen) != NULL) + return -EIO; if (!over) { /* We fill entries into dstbuf only as much as -- cgit v1.2.3-59-g8ed1b From 05726acabef10faffcbc400c109ddb6c9d7560e4 Mon Sep 17 00:00:00 2001 From: Dong Fang Date: Tue, 30 Jul 2013 22:50:01 -0400 Subject: fuse: use list_for_each_entry() for list traversing Signed-off-by: Miklos Szeredi --- fs/fuse/dev.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'fs') diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 1d55f9465400..ef74ad5fd362 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1765,11 +1765,9 @@ static int fuse_notify(struct fuse_conn *fc, enum fuse_notify_code code, /* Look up request on processing list by unique ID */ static struct fuse_req *request_find(struct fuse_conn *fc, u64 unique) { - struct list_head *entry; + struct fuse_req *req; - list_for_each(entry, &fc->processing) { - struct fuse_req *req; - req = list_entry(entry, struct fuse_req, list); + list_for_each_entry(req, &fc->processing, list) { if (req->in.h.unique == unique || req->intr_unique == unique) return req; } -- cgit v1.2.3-59-g8ed1b