[PATCH 3/5] fuse: report a request refused for a live nodeid as stale

Aaron Paterson <[email protected]>
Newsgroups dev.linux.lists.virtualization,dev.linux.lists.fuse-devel,org.kernel.vger.linux-fsdevel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-kselftest
Message-ID <409f0fef91d68c38c52ecd77ccf50bf41b79ddad.1787873791.git.apaterson@pm.me>
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.

Signed-off-by: Aaron Paterson <[email protected]>
---
 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(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index e49b4e874b15..217a03999050 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -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;
 	}
 
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 8d6135a6108a..308b15c30f34 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -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);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index c8d4c5f3af7e..e6b2597d6dd6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -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);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index e9552be3637b..cde0a5335089 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.