[PATCH] fuse: invalidate inode pagecache when atomic_o_trunc flag is enabled

[PATCH] fuse: invalidate inode pagecache when atomic_o_trunc flag is enabled

[Chad Austin]

When the atomic_o_trunc flag is set, fuse_do_setattr skips most of its
logic, including truncating the page cache.  Run the same invalidation
logic in fuse_finish_open when truncating.  This fixes a bug where
open(O_TRUNC) followed by a sequence of writes does not flush the page
cache, resulting in an mmap longer than the new file's length (from
another process) not properly zero-filling the last page.

Does fuse_finish_open() need additional locking?  I am not sure of the
locking requirements of truncate_pagecache or invalidate_inode_pages2.

The following C++ program reproduces the bug.  The FUSE daemon must
have enabled the ATOMIC_O_TRUNC flag
https://gist.github.com/chadaustin/8e4ba1e2cd2d023ff6f9eff921eb8bfc

Signed-off-by: Chad Austin <chadaustin@fb.com>
---
 fs/fuse/dir.c    |  7 ++-----
 fs/fuse/file.c   | 12 ++++++++++++
 fs/fuse/fuse_i.h |  2 ++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2496738..c387171 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1698,11 +1698,8 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 	 * Only call invalidate_inode_pages2() after removing
 	 * FUSE_NOWRITE, otherwise fuse_launder_page() would deadlock.
 	 */
-	if ((is_truncate || !is_wb) &&
-	    S_ISREG(inode->i_mode) && oldsize != outarg.attr.size) {
-		truncate_pagecache(inode, outarg.attr.size);
-		invalidate_inode_pages2(inode->i_mapping);
-	}
+	if ((is_truncate || !is_wb) && oldsize != outarg.attr.size)
+		fuse_truncate_inode_cache(inode, outarg.attr.size);
 
 	clear_bit(FUSE_I_SIZE_UNSTABLE, &fi->state);
 	return 0;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index cb7dff5..cb1968e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -188,6 +188,10 @@ void fuse_finish_open(struct inode *inode, struct file *file)
 		i_size_write(inode, 0);
 		spin_unlock(&fc->lock);
 		fuse_invalidate_attr(inode);
+
+		if (!fc->writeback_cache)
+			fuse_truncate_inode_cache(inode, 0);
+
 		if (fc->writeback_cache)
 			file_update_time(file);
 	}
@@ -195,6 +199,14 @@ void fuse_finish_open(struct inode *inode, struct file *file)
 		fuse_link_write_file(file);
 }
 
+void fuse_truncate_inode_cache(struct inode *inode, loff_t newsize)
+{
+	if (S_ISREG(inode->i_mode)) {
+		truncate_pagecache(inode, newsize);
+		invalidate_inode_pages2(inode->i_mapping);
+	}
+}
+
 int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index d5773ca..47baea4 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -913,6 +913,8 @@ void fuse_release_nowrite(struct inode *inode);
 
 u64 fuse_get_attr_version(struct fuse_conn *fc);
 
+void fuse_truncate_inode_cache(struct inode *inode, loff_t newsize);
+
 /**
  * File-system tells the kernel to invalidate cache for the given node id.
  */
-- 
2.9.5

Re: [PATCH] fuse: invalidate inode pagecache when atomic_o_trunc flag is enabled

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Tue, Dec 5, 2017 at 10:12 PM, Chad Austin <chadaustin@fb.com> wrote:
> When the atomic_o_trunc flag is set, fuse_do_setattr skips most of its
> logic, including truncating the page cache.  Run the same invalidation
> logic in fuse_finish_open when truncating.  This fixes a bug where
> open(O_TRUNC) followed by a sequence of writes does not flush the page
> cache, resulting in an mmap longer than the new file's length (from
> another process) not properly zero-filling the last page.
>
> Does fuse_finish_open() need additional locking?  I am not sure of the
> locking requirements of truncate_pagecache or invalidate_inode_pages2.
>
> The following C++ program reproduces the bug.  The FUSE daemon must
> have enabled the ATOMIC_O_TRUNC flag
> https://gist.github.com/chadaustin/8e4ba1e2cd2d023ff6f9eff921eb8bfc

Thanks for the report and the patch.

Here's a simpler patch (we don't actually need the
invalidate_inode_pages2(), because all the pages have already been
truncated).

I tested it with your reproducer and it fixes the issue for me.

Unless you see a problem with it, I'll queue this up for 4.16.

Thanks,
Miklos

[-- Attachment #2: fuse-atomic_o_trunc-should-truncate-pagecache.patch --]
[-- Type: text/x-patch, Size: 447 bytes --]

---
 fs/fuse/dir.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1629,8 +1629,12 @@ int fuse_do_setattr(struct dentry *dentr
 		return err;
 
 	if (attr->ia_valid & ATTR_OPEN) {
-		if (fc->atomic_o_trunc)
+		if (fc->atomic_o_trunc) {
+			WARN_ON(!(attr->ia_valid & ATTR_SIZE));
+			WARN_ON(attr->ia_size != 0);
+			truncate_pagecache(inode, 0);
 			return 0;
+		}
 		file = NULL;
 	}
 

Re: [PATCH] fuse: invalidate inode pagecache when atomic_o_trunc flag is enabled

[Chad Austin]

On Thu, Feb 8, 2018 at 6:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> Thanks for the report and the patch.
>
> Here's a simpler patch (we don't actually need the
> invalidate_inode_pages2(), because all the pages have already been
> truncated).
>
> I tested it with your reproducer and it fixes the issue for me.
>
> Unless you see a problem with it, I'll queue this up for 4.16.

Thanks!  I verified that your simpler patch also fixes our more
complex internal reproduction.  Looking forward to it landing!