Re: fuse_notify_inval_inode() may be ineffective when getattr request is in progress

[Miklos Szeredi <miklos@szeredi.hu>]

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

On Mon, May 4, 2020 at 1:00 PM Krzysztof Rusek <rusek@9livesdata.com> wrote:
>
> Hello,
>
> I'm working on a user-space file system implementation utilizing fuse
> kernel module (and libfuse user-space library). This file system
> implementation provides a custom ioctl operation that uses
> fuse_lowlevel_notify_inval_inode() function (which translates to
> fuse_notify_inval_inode() in kernel) to notify the kernel that the file
> was changed by the ioctl. However, under certain circumstances,
> fuse_notify_inval_inode() call is ineffective, resulting in incorrect
> file attributes being cached by the kernel. The problem occurs when
> ioctl() is executed in parallel with getattr().
>
> I noticed this problem on RedHat 7.7 (3.10.0-1062.el7.x86_64), but I
> believe mainline kernel is affected as well.
>
> I think there is a bug in the way fuse_notify_inval_inode() invalidates
> file attributes. If getattr() started before fuse_notify_inval_inode()
> was called, then the attributes returned by user-space process may be
> out of date, and should not be cached. But fuse_notify_inval_inode()
> only clears attribute validity time, which does not prevent getattr()
> result from being cached.
>
> More precisely, this bug occurs in the following scenario:
>
> 1. kernel starts handling ioctl
> 2. file system process receives ioctl request
> 3. kernel starts handling getattr
> 4. file system process receives getattr request
> 5. file system process executes getattr
> 6. file system process executes ioctl, changing file state
> 7. file system process invokes fuse_lowlevel_notify_inval_inode(), which
> invalidates file attributes in kernel
> 8. file system process sends ioctl reply, ioctl handling ends
> 9. file system process sends getattr reply, attributes are incorrectly
> cached in the kernel
>
> (Note that this scenario assumes that file system implementation is
> multi-threaded, therefore allowing ioctl() and getattr() to be handled
> in parallel.)

Can you please try the attached patch?

Thanks,
Miklos

[-- Attachment #2: fuse-update-attr_version-counter-on-fuse_notify_inval_inode.patch --]
[-- Type: text/x-patch, Size: 741 bytes --]

---
 fs/fuse/inode.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -321,6 +321,8 @@ struct inode *fuse_iget(struct super_blo
 int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
 			     loff_t offset, loff_t len)
 {
+	struct fuse_conn *fc = get_fuse_conn_super(sb);
+	struct fuse_inode *fi;
 	struct inode *inode;
 	pgoff_t pg_start;
 	pgoff_t pg_end;
@@ -329,6 +331,11 @@ int fuse_reverse_inval_inode(struct supe
 	if (!inode)
 		return -ENOENT;
 
+	fi = get_fuse_inode(inode);
+	spin_lock(&fi->lock);
+	fi->attr_version = atomic64_inc_return(&fc->attr_version);
+	spin_unlock(&fi->lock);
+
 	fuse_invalidate_attr(inode);
 	forget_all_cached_acls(inode);
 	if (offset >= 0) {