Bug 244178 - [FUSEFS]: underlying file modified leads to corrupted fuse data
Summary: [FUSEFS]: underlying file modified leads to corrupted fuse data
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-16 22:17 UTC by Ben RUBSON
Modified: 2020-03-22 15:25 UTC (History)
4 users (show)

See Also:


Attachments
invalidate cache when a file changes unexpectedly (14.08 KB, patch)
2020-03-09 00:25 UTC, Alan Somers
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben RUBSON 2020-02-16 22:17:40 UTC
Hi,

I still face, with 12.1-RELEASE-p2, the issue discussed here :
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230258#c35
Which continued there :
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235774

The issue is the following.

Let's assume raw file is the underlying file.
And fuse file the corresponding fuse file.

1. create raw file with some data
2. open fuse file
3. verify fuse size : correct
4. verify fuse data : correct
5. add some data to raw file
6. verify fuse size : correct (*)
7. verify fuse data : garbage
(*) as expected we have to use fuse options attr_timeout=0 entry_timeout=0.

So, if we keep the fuse file opened, while the underlying one is modified :
- we read the fuse file up to the expected size, perfect ;
- but we read garbage.

If we close and re-open the fuse file, then we correctly read it.

There are 2 possible workarounds :
- use FUSE direct_io mount option ;
- sysctl vfs.fusefs.data_cache_mode = 0.

I don't know whether or not the default behavior is correct.
But at least with Linux, we correctly read the data even keeping the fuse file opened.
There may then still be a cache issue.

Many thanks for your feedback !
Comment 1 Alan Somers freebsd_committer 2020-02-16 23:23:52 UTC
In step 5, are you extending the underlying file from EOF onwards, or are you rewriting the whole thing?  And in step 7, is the data correct up until the old EOF, or is it all wrong?

Do you have a standalone test case that can demonstrate the problem?

It sounds to me like the correct answer might be for the fuse server to return FOPEN_DIRECT_IO for every FUSE_OPEN request.  That would prevent the kernel from using the cache at all.
Comment 2 Ben RUBSON 2020-02-17 21:48:51 UTC
Thank you for your prompt response Alan.

Whatever the file is extended or rewriten in step 5, the data which has been read in step 4 is returned in step 7, up to the old EOF.

Below is a standone Perl testcase, the last one failing from the fusefs-encfs test suite.
It asumes you mount a 1:1 fuse FS from /tmp/raw to /tmp/fuse, mounted with -o attr_timeout=0 (entry_timeout=0 is useless here, btw not sure what it's useful for).

If you don't have such a 1:1 fuse FS, you can use EncFS in insecure (i.e. 1:1) mode :
pkg install fusefs-encfs
mkdir -p /tmp/raw /tmp/fuse
encfs --insecure -o attr_timeout=0 --nodatacache /tmp/raw /tmp/fuse
# choose x mode, don't care about encryption numbers, disable file data encryption and choose null / no encryption of filenames.

Below Perl test case will then fail, until you use one of the 2 workarounds given above.
Or until you enable the in-loop open/close of the fuse file (and disable the global one).

Of course feel free if needed, thx !

### Standalone Perl test case :

# Get the file size from stat()
sub statSize
{
    my $f = shift;
    my @s = stat($f) or die("stat on '$f' failed");
    return $s[7];
}

# Get the file size by read()
sub readSize
{
    my $fh = shift;
    seek($fh, 0, 0);
    my $block = 4*1024;
    my $s;
    my $data;
    my $sum = read($fh, $data, $block);
    while ($s += read($fh, $data, $block))
    {
        $sum += $s;
    }
    return $sum;
}

# Verify that the file size matches the target (both stat() & read())
sub sizeVerify
{
    my $ok = 1;
    my $fh = shift;
    my $s0 = shift;
    $ss = statSize($fh);
    if ($s0 != $ss) {
        $ok = 0;
        print("# stat size $ss, expected $s0\n");
    }
    $sr = readSize($fh);
    if ($s0 != $sr) {
        $ok = 0;
        print("# read size $sr, expected $s0\n");
    }
    return $ok;
}

open(my $rfh, ">", "/tmp/raw/grow");
$rfh->autoflush;

open(my $vfh, "<", "/tmp/raw/grow");
open(my $ffh, "<", "/tmp/fuse/grow");

for($i=5; $i < 80; $i += 5)
{
    #open(my $ffh, "<", "/tmp/fuse/grow");
    printf($rfh "%04d.", $i) or die("write failed");
    sizeVerify($vfh, $i) or die("unexpected raw file size");
    sizeVerify($ffh, $i) or die("unexpected fuse file size");
    print "size:$i\n";
    seek($vfh, 0, 0);
    seek($ffh, 0, 0);
    print "raw :".<$vfh>."\n";
    print "fuse:".<$ffh>."\n---\n";
    #close($ffh);
}

### Failing result :

size:5
raw :0005.
fuse:0005.
---
size:10
raw :0005.0010.
fuse:0005.
---
size:15
raw :0005.0010.0015.
fuse:0005.
---
size:20
raw :0005.0010.0015.0020.
fuse:0005.
---
size:25
raw :0005.0010.0015.0020.0025.
fuse:0005.
---
size:30
raw :0005.0010.0015.0020.0025.0030.
fuse:0005.
---
size:35
raw :0005.0010.0015.0020.0025.0030.0035.
fuse:0005.
---
size:40
raw :0005.0010.0015.0020.0025.0030.0035.0040.
fuse:0005.
---
size:45
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.
fuse:0005.
---
size:50
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.
fuse:0005.
---
size:55
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.0055.
fuse:0005.
---
size:60
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.0055.0060.
fuse:0005.
---
size:65
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.0055.0060.0065.
fuse:0005.
---
size:70
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.0055.0060.0065.0070.
fuse:0005.
---
size:75
raw :0005.0010.0015.0020.0025.0030.0035.0040.0045.0050.0055.0060.0065.0070.0075.
fuse:0005.
---
Comment 3 Alan Somers freebsd_committer 2020-02-18 04:05:00 UTC
So there are two things going on here:

1) When fusefs discovers that a file's cached size is invalid, it's supposed to fix the size and discard any invalid cached buffers.  It looks like we're doing that correctly for truncations and for large extensions (I'm still trying to figure out how large "large" is), but as you found not for small extensions.

2) The test simulates a buggy fuse server.  If it's ever possible for the file system's data to change behind the kernel's back, then it the server MUST use direct_io.  The kernel is using a heuristic to detect this behind-the-back behavior, but what if the data changes yet the file's length stays the same?  The heuristic can't catch that.  The situation is somewhat better with newer FUSE protocol versions that have an asynchronous cache invalidation mechanism, but even then it's racy.  This is probably why nobody has reported the bug yet; well-behaved fuse servers don't trigger it.
Comment 4 Ben RUBSON 2020-02-18 14:08:36 UTC
A little bit of context, there are some use-cases where it is expected that raw/underlying file could be modified.
fusefs-encfs has a so-called "reverse" mode.
It gives you, through fuse, an encrypted view of your plain raw/underlying files.
You can then for example sync your on-the-fly encrypted files to a remote untrusted storage location.
Even if it's not to expect (as the result may rather look strange), a raw/underlying file could be modified while it's read through fuse.

Coming back to your first point, glad to see we may have found thanks to this a remaining cache issue.

And regarding the second point, you're certainly right that fuse server should enforce direct_io in such use cases.
I'll then certainly modify fusefs-encfs accordingly once this ticket will be "solved", so that we'll have the full story / it'll be consistent.
Comment 5 Alan Somers freebsd_committer 2020-03-09 00:25:35 UTC
Created attachment 212269 [details]
invalidate cache when a file changes unexpectedly

Ben, please test the attached patch.  It causes fusefs to invalidate a file's cache when it detects that a buggy server has modified the file.
Comment 6 Ben RUBSON 2020-03-09 23:53:14 UTC
Alan, thank you very much for this patch, I just tested it and it works as expected : when the underlying file size is modified, cache is correctly invalidated.
The attached test case then now correctly runs.
Perfect.

Of course, as you already stated above, if the data changes but the file's length stays the same, then the patch does not help, and we then should use DIRECT_IO.

Thank you again !
Comment 7 Alan Somers freebsd_committer 2020-03-10 04:00:16 UTC
Code review in progress at https://reviews.freebsd.org/D24012
Comment 8 commit-hook freebsd_committer 2020-03-11 04:30:41 UTC
A commit references this bug:

Author: asomers
Date: Wed Mar 11 04:29:47 UTC 2020
New revision: 358867
URL: https://svnweb.freebsd.org/changeset/base/358867

Log:
  fusefs: avoid cache corruption with buggy fuse servers

  The FUSE protocol allows the client (kernel) to cache a file's size, if the
  server (userspace daemon) allows it. A well-behaved daemon obviously should
  not change a file's size while a client has it cached. But a buggy daemon
  might. If the kernel ever detects that that has happened, then it should
  invalidate the entire cache for that file. Previously, we would not only
  cache stale data, but in the case of a file extension while we had the size
  cached, we accidentally extended the cache with zeros.

  PR:		244178
  Reported by:	Ben RUBSON <ben.rubson@gmx.com>
  Reviewed by:	cem
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D24012

Changes:
  head/sys/fs/fuse/fuse_internal.c
  head/sys/fs/fuse/fuse_node.c
  head/sys/fs/fuse/fuse_node.h
  head/sys/fs/fuse/fuse_vnops.c
  head/tests/sys/fs/fusefs/Makefile
  head/tests/sys/fs/fusefs/cache.cc
  head/tests/sys/fs/fusefs/getattr.cc
  head/tests/sys/fs/fusefs/io.cc
  head/tests/sys/fs/fusefs/utils.cc
  head/tests/sys/fs/fusefs/utils.hh
Comment 9 commit-hook freebsd_committer 2020-03-22 15:25:18 UTC
A commit references this bug:

Author: asomers
Date: Sun Mar 22 15:24:27 UTC 2020
New revision: 359214
URL: https://svnweb.freebsd.org/changeset/base/359214

Log:
  MFC r358867:

  fusefs: avoid cache corruption with buggy fuse servers

  The FUSE protocol allows the client (kernel) to cache a file's size, if the
  server (userspace daemon) allows it. A well-behaved daemon obviously should
  not change a file's size while a client has it cached. But a buggy daemon
  might. If the kernel ever detects that that has happened, then it should
  invalidate the entire cache for that file. Previously, we would not only
  cache stale data, but in the case of a file extension while we had the size
  cached, we accidentally extended the cache with zeros.

  PR:		244178
  Reported by:	Ben RUBSON <ben.rubson@gmx.com>
  Reviewed by:	cem
  Differential Revision:	https://reviews.freebsd.org/D24012

Changes:
_U  stable/12/
  stable/12/sys/fs/fuse/fuse_internal.c
  stable/12/sys/fs/fuse/fuse_node.c
  stable/12/sys/fs/fuse/fuse_node.h
  stable/12/sys/fs/fuse/fuse_vnops.c
  stable/12/tests/sys/fs/fusefs/Makefile
  stable/12/tests/sys/fs/fusefs/cache.cc
  stable/12/tests/sys/fs/fusefs/getattr.cc
  stable/12/tests/sys/fs/fusefs/io.cc
  stable/12/tests/sys/fs/fusefs/utils.cc
  stable/12/tests/sys/fs/fusefs/utils.hh