Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* fuse_notify_inval_inode() may be ineffective when getattr request is in progress
@ 2020-05-04 11:00 Krzysztof Rusek
  2020-05-18 13:08 ` Miklos Szeredi
  0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Rusek @ 2020-05-04 11:00 UTC (permalink / raw)
  To: Miklos Szeredi, linux-fsdevel

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

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.)

I'm attaching an archive with a reproduction test for this problem. The 
archive contains a README file explaining how to compile/run it.

By the way, to avoid a somehow similar race when write() is executed in 
parallel with getattr(), there is a special FUSE_I_SIZE_UNSTABLE bit set 
for the duration of write() operation.

Best regards,
Krzysztof Rusek

[-- Attachment #2: fusebug.tar.gz --]
[-- Type: application/gzip, Size: 2582 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: fuse_notify_inval_inode() may be ineffective when getattr request is in progress
  2020-05-04 11:00 fuse_notify_inval_inode() may be ineffective when getattr request is in progress Krzysztof Rusek
@ 2020-05-18 13:08 ` Miklos Szeredi
  2020-05-20 12:23   ` Krzysztof Rusek
  0 siblings, 1 reply; 4+ messages in thread
From: Miklos Szeredi @ 2020-05-18 13:08 UTC (permalink / raw)
  To: Krzysztof Rusek; +Cc: linux-fsdevel

[-- 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) {

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: fuse_notify_inval_inode() may be ineffective when getattr request is in progress
  2020-05-18 13:08 ` Miklos Szeredi
@ 2020-05-20 12:23   ` Krzysztof Rusek
  2020-05-20 12:26     ` Miklos Szeredi
  0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Rusek @ 2020-05-20 12:23 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel

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

Hi Miklos,

I've checked that after applying the patch, the problem no longer occurs.

Since I'm running tests on RedHat 7.7, I had to slightly modify the 
patch, because on that kernel version locking around attr_version is 
done differently. I'm attaching modified patch, just in case.

Best regards,
Krzysztof Rusek

On 5/18/20 3:08 PM, Miklos Szeredi wrote:
> 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.el7.diff --]
[-- Type: text/x-patch, Size: 703 bytes --]

--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -332,6 +332,8 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 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;
@@ -340,6 +342,11 @@ int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
 	if (!inode)
 		return -ENOENT;
 
+	fi = get_fuse_inode(inode);
+	spin_lock(&fc->lock);
+	fi->attr_version = ++fc->attr_version;
+	spin_unlock(&fc->lock);
+
 	fuse_invalidate_attr(inode);
 	if (offset >= 0) {
 		pg_start = offset >> PAGE_CACHE_SHIFT;

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: fuse_notify_inval_inode() may be ineffective when getattr request is in progress
  2020-05-20 12:23   ` Krzysztof Rusek
@ 2020-05-20 12:26     ` Miklos Szeredi
  0 siblings, 0 replies; 4+ messages in thread
From: Miklos Szeredi @ 2020-05-20 12:26 UTC (permalink / raw)
  To: Krzysztof Rusek; +Cc: linux-fsdevel

On Wed, May 20, 2020 at 2:24 PM Krzysztof Rusek <rusek@9livesdata.com> wrote:
>
> Hi Miklos,
>
> I've checked that after applying the patch, the problem no longer occurs.
>
> Since I'm running tests on RedHat 7.7, I had to slightly modify the
> patch, because on that kernel version locking around attr_version is
> done differently. I'm attaching modified patch, just in case.

Hi Krzysztof,

Thanks for the report and testing!

Miklos

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-05-20 12:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-04 11:00 fuse_notify_inval_inode() may be ineffective when getattr request is in progress Krzysztof Rusek
2020-05-18 13:08 ` Miklos Szeredi
2020-05-20 12:23   ` Krzysztof Rusek
2020-05-20 12:26     ` Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).