fuse: report a request refused for a live nodeid as stale

A FUSE request that names a nodeid and nothing else is only sent for an inode the client has looked up and holds a reference to, and the server owes the client that inode until it is sent FUSE_FORGET. When a server answers such a request with ENOENT it is describing a handle it was obliged to honour, not a file that has gone away, and the client passes that ENOENT to the caller unchanged: a file that never stopped existing is reported missing.

This series reports that case as stale instead, so the VFS retries the lookup rather than handing a spurious ENOENT to userspace. The path opens say EOPENSTALE, which is what an open says when the cached information it started from has gone stale and which path_openat() resolves into ECHILD under LOOKUP_RCU or ESTALE otherwise; everything else says ESTALE, which retry_estale() answers by repeating the lookup once under LOOKUP_REVAL. NFS reports its own stale opens the same way.

The conversion is written twice, once in fs/fuse for any transport and once in fs/fuse/virtio_fs.c at reply completion, and each one fixes the symptom without the other. They are sent together because they were found together; if only one is wanted, patch 3 is the general one and patch 5 is the virtio-specific one, and either can be dropped.

Why it matters, and how it was found: a server that releases an inode as soon as a rename displaces the name it was looked up by will refuse opens for inodes the client still holds. On such a server, roughly one open in eight during a rename race is refused while stat continues to describe the file. That reaches real programs. git's t5318 fails 52 to 53 subtests per round against such a mount and 0 against local disk on the same machine in the same alternating run, and the mechanism is visible underneath it: .git/index opens ending ENOENT are 49 of 116 on the affected mount against 9 of 157 on local disk, where those 9 are the legitimate ones a fresh repository makes before an index exists. The user-visible failure is a destroyed git index.

Measurements. All four arms are one kernel source, 6.18.5, with one config, built from trees that differ only in whether these patches are applied, run against the same server with the same probe for the same duration:

  arm  fs/fuse  virtio_fs  opens    refused
   A   stock    stock      323076   45201 (14.0%)
   B   stock    patched     54378   0
   C   patched  stock      323776   0
   D   patched  patched    315114   0

Every refusal on arm A is the contradiction: open said ENOENT while stat on the same path still described the file. Arms C and A carry almost identical open counts against the same server, so the pair differing only in fs/fuse is 45201 refusals against none. Arms B and C are why the two conversions are sent together but either can be
dropped: each removes the symptom without the other.

For the record, the same fault measured 12.9% on a 6.8 distro kernel three weeks earlier, so nothing between 6.8 and 6.18 addressed it, which is consistent with these patches applying to current mainline with only hunk-offset movement.

The first two patches are selftest housekeeping that patch 4 needs: a build artifact that was not ignored, and a rename of the libfuse3 pkg-config variables so a second test can share the same guard. Patch 4 is the test itself, which mounts a libfuse3 server that refuses one request for a live nodeid and asserts the caller does not see ENOENT. It skips where libfuse3 or fusermount3 is unavailable.

checkpatch is clean apart from two warnings I believe are false positives, and I would rather name them than leave them to be
rediscovered: MAINTAINERS already carries
F: tools/testing/selftests/filesystems/fuse/ under FUSE FILESYSTEM [CORE], so the new selftest needs no MAINTAINERS change; and the char *fuse_argv[] in the test cannot be static const, because struct fuse_args carries a plain char **argv that libfuse may modify.

Aaron Paterson (5):
selftests/fuse: ignore the built acl cache test selftests/fuse: name the libfuse3 flags for the library
fuse: report a request refused for a live nodeid as stale selftests/fuse: cover a request refused for a live nodeid
virtiofs: report a request refused for a live nodeid as stale

 fs/fuse/dir.c                                 |  10 +
 fs/fuse/file.c                                |  11 +
 fs/fuse/fuse_i.h                              |  19 +
 fs/fuse/inode.c                               |   2 +-
 fs/fuse/virtio_fs.c                           |  62 +++
 .../selftests/filesystems/fuse/.gitignore     |   2 +
 .../selftests/filesystems/fuse/Makefile       |  16 +-
 .../filesystems/fuse/fuse_estale_test.c       | 450 ++++++++++++++++++
 8 files changed, 565 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/filesystems/fuse/fuse_estale_test.c

2.55.0.553.g4ad8c266be

The Makefile builds fuse_acl_cache_test into the source directory when libfuse3 is present, but the binary is not ignored, so a build leaves the tree
dirty. The two other programs built here are already listed.

---
 tools/testing/selftests/filesystems/fuse/.gitignore | 1 +
 1 file changed, 1 insertion(+)

index fb51603fe419..25c779065806 100644
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+fuse_acl_cache_test
 fuse_mnt
 fusectl_test
 write_extend_eof_test

2.55.0.553.g4ad8c266be

The flags describe libfuse3 rather than the one test that currently uses them, so any further test needing the library can share them.

---
 tools/testing/selftests/filesystems/fuse/Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 95a1ee947ca7..1ea87008ef9e 100644
@@ -6,10 +6,10 @@ TEST_GEN_PROGS := fusectl_test
 TEST_GEN_PROGS += write_extend_eof_test
 TEST_GEN_FILES := fuse_mnt
 
-# fuse_acl_cache_test requires libfuse3; add it only when the library is present.
-ACL_CFLAGS := $(shell pkg-config fuse3 --cflags 2>/dev/null)
-ACL_LDLIBS := $(shell pkg-config fuse3 --libs 2>/dev/null)
-ifneq ($(ACL_CFLAGS),)
+# These tests require libfuse3; add them only when the library is present.
+FUSE3_CFLAGS := $(shell pkg-config fuse3 --cflags 2>/dev/null)
+FUSE3_LDLIBS := $(shell pkg-config fuse3 --libs 2>/dev/null)
+ifneq ($(FUSE3_CFLAGS),)
 TEST_GEN_PROGS += fuse_acl_cache_test
 endif
 

@@ -30,5 +30,5 @@ endif
 $(OUTPUT)/fuse_mnt: CFLAGS += $(VAR_CFLAGS)
 $(OUTPUT)/fuse_mnt: LDLIBS += $(VAR_LDLIBS)
 
-$(OUTPUT)/fuse_acl_cache_test: CFLAGS += $(ACL_CFLAGS)
-$(OUTPUT)/fuse_acl_cache_test: LDLIBS += $(ACL_LDLIBS)
+$(OUTPUT)/fuse_acl_cache_test: CFLAGS += $(FUSE3_CFLAGS)
+$(OUTPUT)/fuse_acl_cache_test: LDLIBS += $(FUSE3_LDLIBS)

2.55.0.553.g4ad8c266be

A request naming a nodeid and nothing else is only sent for an inode the client has looked up and holds a reference to, and the server owes the client that inode until it is sent FUSE_FORGET. ENOENT to such a request describes a handle the server was obliged to honour rather than something that has gone away, and the caller cannot tell the difference:
a file that never stopped existing is reported missing.

Report it so the caller can recover.

The path opens say EOPENSTALE. fuse has not needed it before, its only other use of a stale error being fuse_get_dentry(), where ESTALE answers an export handle that cannot be resolved, which is a different question. EOPENSTALE is what an open says when the cached information it started from has gone stale, and path_openat() decides what that means for the walk in progress, answering ECHILD under LOOKUP_RCU so it drops to REF-walk and ESTALE otherwise so the name is resolved again under LOOKUP_REVAL. NFS reports its own stale opens this way, in nfs4_file_open() and nfs_atomic_open(). The conversion sits at the two callers that opened by name rather than in fuse_file_open(), which fuse_priv_ioctl_prepare() also reaches: it opens the inode to serve FS_IOC_GETFLAGS and FS_IOC_FSGETXATTR and returns what it gets through vfs_fileattr_get(), with no open behind it to translate the internal errno.

The getattr, setattr, readlink and statfs paths say ESTALE through one helper, which retry_estale() answers by repeating the lookup once under LOOKUP_REVAL. fs/namei.c, fs/open.c, fs/stat.c, fs/statfs.c, fs/utimes.c and fs/xattr.c all reach it. EOPENSTALE would be wrong for them, since path_openat() is the only place that translates it and nothing would outside an open.

Requests carrying a name are left alone, since ENOENT is then ambiguous and is frequently what the caller asked to be told. fuse already reads it that way: fuse_unlink() and fuse_rmdir() treat ENOENT as grounds to invalidate the entry, which is a statement about the name rather than the inode. The xattr requests belong with those, carrying an attribute name whose absence a server may report as ENOENT rather than ENODATA.

Requests against an already open descriptor are left alone as well, having no equivalent retry to reach, and FUSE_IOCTL and FUSE_POLL hand the server's errno to userspace verbatim.

Observed with virtiofs on macOS, where a server releases an inode as soon as a rename displaces the name it was looked up by and roughly one open in eight during a rename race is refused while stat continues to describe the file.

---
 fs/fuse/dir.c    | 10 ++++++++++
 fs/fuse/file.c   | 11 +++++++++++
 fs/fuse/fuse_i.h | 19 +++++++++++++++++++
 fs/fuse/inode.c  |  2 +-
 4 files changed, 41 insertions(+), 1 deletion(-)

index e49b4e874b15..217a03999050 100644
@@ -1532,6 +1532,8 @@ static int fuse_do_getattr(struct mnt_idmap *idmap, struct inode *inode,
 			if (stat)
 				fuse_fillattr(idmap, inode, &outarg.attr, stat);
 		}
+	} else {
+		err = fuse_stale_inode_err(err);
 	}
 	return err;
 }

@@ -1848,6 +1850,7 @@ static int fuse_readlink_folio(struct inode *inode, struct folio *folio)
 
 	fuse_invalidate_atime(inode);
 
+	res = fuse_stale_inode_err(res);
 	if (res < 0)
 		return res;
 

@@ -1910,6 +1913,12 @@ static int fuse_dir_open(struct inode *inode, struct file *file)
 		return err;
 
 	err = fuse_do_open(fm, get_node_id(inode), file, true);
+	/*
+	 * As in fuse_open_common(): a path walk stands behind this open,
+	 * so the refusal is EOPENSTALE for path_openat() to answer.
+	 */
+	if (err == -ENOENT)
+		err = -EOPENSTALE;
 	if (!err) {
 		struct fuse_file *ff = file->private_data;
 

@@ -2249,6 +2258,7 @@ int fuse_do_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	if (err) {
 		if (err == -EINTR)
 			fuse_invalidate_attr(inode);
+		err = fuse_stale_inode_err(err);
 		goto error;
 	}
 

index 8d6135a6108a..308b15c30f34 100644
@@ -279,6 +279,17 @@ static int fuse_open(struct inode *inode, struct file *file)
 		fuse_set_nowrite(inode);
 
 	err = fuse_do_open(fm, get_node_id(inode), file, false);
+	/*
+	 * This open reached the server through a path walk, so a refusal
+	 * for the live nodeid is reported as EOPENSTALE rather than the
+	 * ESTALE the nodeid-only requests say: path_openat() picks the
+	 * cheapest retry for the walk in progress, ECHILD under LOOKUP_RCU
+	 * and ESTALE otherwise. fuse_file_open() is left alone because
+	 * fuse_priv_ioctl_prepare() reaches it to serve FS_IOC_GETFLAGS,
+	 * with no open behind it to translate the internal errno.
+	 */
+	if (err == -ENOENT)
+		err = -EOPENSTALE;
 	if (!err) {
 		ff = file->private_data;
 		err = fuse_finish_open(inode, file);

index c8d4c5f3af7e..e6b2597d6dd6 100644
@@ -1250,6 +1250,25 @@ void fuse_inode_uncached_io_end(struct fuse_inode *fi);
 int fuse_file_io_open(struct file *file, struct inode *inode);
 void fuse_file_io_release(struct fuse_file *ff, struct inode *inode);
 
+/*
+ * Report a request refused for a live nodeid as stale.
+ *
+ * A request that names a nodeid and nothing else is only sent for an
+ * inode the client has looked up and holds a reference to, and the
+ * server owes the client that inode until it is sent FUSE_FORGET.
+ * ENOENT from the server describes the inode itself rather than a name
+ * that has gone away. Report the handle as stale, which the caller
+ * answers by resolving the name again under LOOKUP_REVAL and acting on
+ * whatever it refers to now. A name that really has gone fails that
+ * second lookup and the caller still sees ENOENT.
+ */
+static inline int fuse_stale_inode_err(int err)
+{
+	if (err == -ENOENT)
+		return -ESTALE;
+	return err;
+}
+
 /* file.c */
 struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
 				 unsigned int open_flags, bool isdir);

index e9552be3637b..cde0a5335089 100644
@@ -666,7 +666,7 @@ static int fuse_statfs(struct dentry *dentry, struct kstatfs *buf)
 	err = fuse_simple_request(fm, &args);
 	if (!err)
 		convert_fuse_statfs(buf, &outarg.st);
-	return err;
+	return fuse_stale_inode_err(err);
 }
 
 static struct fuse_sync_bucket *fuse_sync_bucket_alloc(void)

2.55.0.553.g4ad8c266be

Add a test that mounts a libfuse3 server which can be told to refuse a request for an inode the client still holds a reference to, and check that the caller recovers instead of being told the file is gone.

The server answers ENOENT on demand for each request the fix converts, FUSE_OPEN, FUSE_GETATTR, FUSE_SETATTR, FUSE_READLINK and FUSE_STATFS, while continuing to serve every other request for the same inode, which is how a server that releases an inode too early behaves. open(), stat(), chmod(), readlink() and statfs() are then expected to succeed, having resolved the name again under LOOKUP_REVAL, and the request counters confirm the retry happened rather than the answer being served from cache.

Each of those requests carries a nodeid, so an ENOENT answering one of them describes a handle rather than a name. FUSE_LOOKUP is the exception and is covered the other way round: a name the server does not have must still report ENOENT, since there the refusal is the answer.

---
 .../selftests/filesystems/fuse/.gitignore     |   1 +
 .../selftests/filesystems/fuse/Makefile       |   4 +
 .../filesystems/fuse/fuse_estale_test.c       | 450 ++++++++++++++++++
 3 files changed, 455 insertions(+)
 create mode 100644 tools/testing/selftests/filesystems/fuse/fuse_estale_test.c

index 25c779065806..9cb3048128d5 100644
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 fuse_acl_cache_test
 fuse_mnt
+fuse_estale_test
 fusectl_test
 write_extend_eof_test

index 1ea87008ef9e..f564cb37b3a5 100644
@@ -11,6 +11,7 @@ FUSE3_CFLAGS := $(shell pkg-config fuse3 --cflags 2>/dev/null)
 FUSE3_LDLIBS := $(shell pkg-config fuse3 --libs 2>/dev/null)
 ifneq ($(FUSE3_CFLAGS),)
 TEST_GEN_PROGS += fuse_acl_cache_test
+TEST_GEN_PROGS += fuse_estale_test
 endif
 
 include ../../lib.mk

@@ -32,3 +33,6 @@ $(OUTPUT)/fuse_mnt: LDLIBS += $(VAR_LDLIBS)
 
 $(OUTPUT)/fuse_acl_cache_test: CFLAGS += $(FUSE3_CFLAGS)
 $(OUTPUT)/fuse_acl_cache_test: LDLIBS += $(FUSE3_LDLIBS)
+
+$(OUTPUT)/fuse_estale_test: CFLAGS += $(FUSE3_CFLAGS)
+$(OUTPUT)/fuse_estale_test: LDLIBS += $(FUSE3_LDLIBS)

2.55.0.553.g4ad8c266be

A request naming a nodeid and nothing else is only sent for an inode the client has looked up and holds a reference to, and the server owes the client that inode until it is sent FUSE_FORGET. ENOENT to such a request describes a handle the server was obliged to honour rather than something that has gone away, and the caller cannot tell the difference.

Report it at request completion so the caller can recover.

The opens say EOPENSTALE, which is what an open says when the cached information it started from has gone stale. path_openat() decides what that means for the walk in progress, answering ECHILD under LOOKUP_RCU so it drops to REF-walk and ESTALE otherwise so the name is resolved again under LOOKUP_REVAL. NFS reports its own stale opens the same way.

The rest say ESTALE, which retry_estale() answers by repeating the lookup once under LOOKUP_REVAL. fs/namei.c, fs/open.c, fs/stat.c, fs/statfs.c, fs/utimes.c and fs/xattr.c all reach it. EOPENSTALE would be wrong for these, since path_openat() is the only place that translates it and nothing would outside an open.

Converted: FUSE_OPEN, FUSE_OPENDIR, FUSE_GETATTR, FUSE_SETATTR, FUSE_READLINK and FUSE_STATFS, the last sending no payload beside the nodeid.

Not converted:

- Requests carrying a name, where ENOENT is ambiguous and is often what the caller asked to be told. FUSE_LOOKUP reports a missing name in a living directory and the directory operations carry a parent nodeid beside one. The xattr requests carry an attribute name, and a server answering ENOENT rather than ENODATA for a missing attribute would have a correct reply turned into a retry that cannot succeed.

- Requests against an already open descriptor, which have no equivalent retry to reach, and FUSE_IOCTL and FUSE_POLL, which hand the server's errno to userspace verbatim.

commit 68b69fa0edb2 ("virtiofs: add FUSE protocol validation") already inspects replies at this point for servers that break the protocol. Nodeid lifetime is another rule a server can break.

Found with a server that releases an inode as soon as a rename displaces the name it was looked up by, where roughly one open in eight during a rename race is refused while stat continues to describe the file.

---
 fs/fuse/virtio_fs.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

index f15e516ebcb5..7b03bf87c2ae 100644
@@ -780,6 +780,66 @@ static bool virtio_fs_verify_response(struct fuse_req *req, unsigned int len)
 	return true;
 }
 
+/*
+ * Report a request refused for a live nodeid as stale.
+ *
+ * These requests carry a nodeid and no name, so the client only sends them for
+ * an inode it has already looked up and holds a reference to, and a server owes
+ * the client that inode until it is sent FUSE_FORGET. A server answering with
+ * ENOENT is describing a handle it was obliged to honour rather than a name
+ * that has gone away, and the caller has no reason to doubt it.
+ *
+ * Saying the handle is stale is something the caller knows how to answer: it
+ * repeats the lookup under LOOKUP_REVAL and acts on whatever the name refers to
+ * now. A name that genuinely has gone fails the retried lookup, so a caller
+ * still learns it is gone. retry_estale() is what does this, and fs/namei.c,
+ * fs/open.c, fs/stat.c, fs/statfs.c, fs/utimes.c and fs/xattr.c all reach it,
+ * which is what makes the conversion useful rather than a rename of the error.
+ *
+ * A request that names something which can itself be absent is left alone,
+ * because ENOENT is then ambiguous and is frequently the answer the caller
+ * asked for. FUSE_LOOKUP reports a missing name in a living directory and the
+ * directory operations carry a parent nodeid beside one, so neither can be
+ * read as a statement about the inode. The extended attribute requests belong
+ * with them: they carry an attribute name, and while a server should report a
+ * missing attribute as ENODATA, one that answers ENOENT instead would have a
+ * correct reply turned into a retry that cannot succeed.
+ *
+ * Requests against an already open descriptor are also left alone, since there
+ * is no equivalent retry to reach and converting the error would rename a
+ * failure rather than repair it, and FUSE_IOCTL and FUSE_POLL hand the
+ * server's errno to userspace verbatim.
+ */
+static void virtio_fs_fixup_stale_error(struct fuse_req *req)
+{
+	if (req->out.h.error != -ENOENT)
+		return;
+
+	switch (req->in.h.opcode) {
+	case FUSE_OPEN:
+	case FUSE_OPENDIR:
+		/*
+		 * An open says EOPENSTALE, and path_openat() decides what the
+		 * walk in progress should make of it: ECHILD under LOOKUP_RCU
+		 * so it drops to REF-walk, ESTALE otherwise so the name is
+		 * resolved again under LOOKUP_REVAL. Naming ESTALE here would
+		 * take the second in both cases and skip a cheaper retry.
+		 */
+		req->out.h.error = -EOPENSTALE;
+		break;
+	case FUSE_GETATTR:
+	case FUSE_SETATTR:
+	case FUSE_READLINK:
+	case FUSE_STATFS:
+		/*
+		 * Nothing translates EOPENSTALE outside the open path, so
+		 * these say ESTALE directly, which retry_estale() answers.
+		 */
+		req->out.h.error = -ESTALE;
+		break;
+	}
+}
+
 /* Work function for request completion */
 static void virtio_fs_request_complete(struct fuse_req *req,
 				       struct virtio_fs_vq *fsvq)

@@ -810,6 +870,8 @@ static void virtio_fs_request_complete(struct fuse_req *req,
 
 	clear_bit(FR_SENT, &req->flags);
 
+	virtio_fs_fixup_stale_error(req);
+
 	fuse_request_end(req);
 	spin_lock(&fsvq->lock);
 	dec_in_flight_req(fsvq);

2.55.0.553.g4ad8c266be

The measurements in the cover letter were taken on 6.18.5. I have now run the same comparison on current mainline, so the numbers sit on the tree the series is posted against: v7.2-15794-g1b78070aaef6, inside the 7.3 merge window.

Both kernels come from the mainline git tree directly rather than from applying the patches at build time. The unpatched arm is that commit as it stands; the patched arm is the same commit with these five applied. So the only difference between the two kernels is the series as posted.

  arm  fs/fuse  virtio_fs  opens    refused
   A   stock    stock      315076   22646 (7.2%)
   D   patched  patched    407169   0

Every refusal on arm A is the contradiction the probe exists to report:
open answered ENOENT while stat on the same path a moment later still described the file. On arm D there are none.

The refusal rate is lower here than the 14.0% the cover letter reports for 6.18.5. It is a race, so the rate moves with load and with how fast the guest gets through its loop, and the arms above were taken while the host was busy building the other kernel. What does not move is which side of zero each arm lands on.

The probe is the one described in the cover letter: one process replaces a file by rename while another opens the same name as fast as it can. The server is Apple's Virtualization virtio-fs, which releases an inode as soon as a rename displaces the name it was looked up by, and is what makes the refusal observable at all.