[RFC PATCH 0/2] virtiofs submounts forgotten after client memory pressure

[RFC PATCH 0/2] virtiofs submounts forgotten after client memory pressure

[Krister Johansen]

Hi,
I recently ran into a situation where a virtiofs client began
encountering EBADF after the client system had an OOM.  After
reproducing the issue and debugging, it appears that the problem is
caused by a virtiofsd submount being forgotten once the dentry
referencing that submount is killed by the shrinker.  In this particular
case, the submount had been bind mounted into a container's mount
namespace.  The reference count on the original dentry was 0, making it
eligible for eviction.  However, because this dentry was also the last
reference the client knew it had, it sent a forget message to the
server.  This caused all future references to the FUSE node-id from
virtiofsd perspective to become invalid.  Subsequent attempts to used
the node-id received an EBADF from the server.

This pair of patches modifies the virtiofs submount code to perform a
lookup on the nodeid that forms the root of the submount.  The patch
before this pulls the revalidate lookup code into a helper function that
can be used both in revalidate and submount superblock fill.

I'm not enamored with this approach, but was hard pressed to think of a
more clever idea.

In the meantime, it's been tested via:

- fstests for virtiofs
- fstests for fuse (against passthrough_ll)
- manual testing to watch how refcounts change between client and server
  in response to filesytem access, umount, and eviction by the shrinker.

Thanks,

-K

Krister Johansen (2):
  fuse: revalidate: move lookup into a separate function
  fuse: ensure that submounts lookup their root

 fs/fuse/dir.c    | 87 +++++++++++++++++++++++++++++++++---------------
 fs/fuse/fuse_i.h |  6 ++++
 fs/fuse/inode.c  | 32 +++++++++++++++---
 3 files changed, 94 insertions(+), 31 deletions(-)

-- 
2.25.1

[RFC PATCH 1/2] fuse: revalidate: move lookup into a separate function

[Krister Johansen]

If this refactoring seems cumbersome, it's because the goal is to move
the lookup parts of fuse_dentry_revalidate into a common function.  This
function will be used elsewhere in a separate commit.  In the meantime,
the new function fuse_dentry_revalidate_lookup is responsible for just
the lookup and validation portions of the revalidate dance.  The
fuse_dentry_revalidate function retains the responsibility for
invalidating and mutating any state associated with the origial
fuse_inode and dentry.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 fs/fuse/dir.c | 87 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index f67bef9d83c4..bdf5526a0733 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -193,6 +193,59 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
 	args->out_args[0].value = outarg;
 }
 
+static int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
+					 struct dentry *entry,
+					 struct inode *inode,
+					 struct fuse_entry_out *outarg,
+					 bool *lookedup)
+{
+	struct dentry *parent;
+	struct fuse_forget_link *forget;
+	struct fuse_inode *fi;
+	FUSE_ARGS(args);
+	int ret;
+
+	forget = fuse_alloc_forget();
+	ret = -ENOMEM;
+	if (!forget)
+		goto out;
+
+	parent = dget_parent(entry);
+	fuse_lookup_init(fm->fc, &args, get_node_id(d_inode(parent)),
+			 &entry->d_name, outarg);
+	ret = fuse_simple_request(fm, &args);
+	dput(parent);
+
+	/* Zero nodeid is same as -ENOENT */
+	if (!ret && !outarg->nodeid)
+		ret = -ENOENT;
+	if (!ret) {
+		fi = get_fuse_inode(inode);
+		if (outarg->nodeid != get_node_id(inode) ||
+		    (bool) IS_AUTOMOUNT(inode) != (bool) (outarg->attr.flags & FUSE_ATTR_SUBMOUNT)) {
+			fuse_queue_forget(fm->fc, forget,
+					  outarg->nodeid, 1);
+			goto invalid;
+		}
+		*lookedup = true;
+	}
+	kfree(forget);
+	if (ret == -ENOMEM || ret == -EINTR)
+		goto out;
+	if (ret || fuse_invalid_attr(&outarg->attr) ||
+	    fuse_stale_inode(inode, outarg->generation, &outarg->attr)) {
+		goto invalid;
+	}
+
+	ret = 1;
+out:
+	return ret;
+
+invalid:
+	ret = 0;
+	goto out;
+}
+
 /*
  * Check whether the dentry is still valid
  *
@@ -216,9 +269,8 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 	else if (time_before64(fuse_dentry_time(entry), get_jiffies_64()) ||
 		 (flags & (LOOKUP_EXCL | LOOKUP_REVAL | LOOKUP_RENAME_TARGET))) {
 		struct fuse_entry_out outarg;
-		FUSE_ARGS(args);
-		struct fuse_forget_link *forget;
 		u64 attr_version;
+		bool lookedup = false;
 
 		/* For negative dentries, always do a fresh lookup */
 		if (!inode)
@@ -230,38 +282,19 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 
 		fm = get_fuse_mount(inode);
 
-		forget = fuse_alloc_forget();
-		ret = -ENOMEM;
-		if (!forget)
-			goto out;
-
 		attr_version = fuse_get_attr_version(fm->fc);
 
-		parent = dget_parent(entry);
-		fuse_lookup_init(fm->fc, &args, get_node_id(d_inode(parent)),
-				 &entry->d_name, &outarg);
-		ret = fuse_simple_request(fm, &args);
-		dput(parent);
-		/* Zero nodeid is same as -ENOENT */
-		if (!ret && !outarg.nodeid)
-			ret = -ENOENT;
-		if (!ret) {
+		ret = fuse_dentry_revalidate_lookup(fm, entry, inode, &outarg,
+						    &lookedup);
+		if (ret == -ENOMEM || ret == -EINTR)
+			goto out;
+		if (lookedup) {
 			fi = get_fuse_inode(inode);
-			if (outarg.nodeid != get_node_id(inode) ||
-			    (bool) IS_AUTOMOUNT(inode) != (bool) (outarg.attr.flags & FUSE_ATTR_SUBMOUNT)) {
-				fuse_queue_forget(fm->fc, forget,
-						  outarg.nodeid, 1);
-				goto invalid;
-			}
 			spin_lock(&fi->lock);
 			fi->nlookup++;
 			spin_unlock(&fi->lock);
 		}
-		kfree(forget);
-		if (ret == -ENOMEM || ret == -EINTR)
-			goto out;
-		if (ret || fuse_invalid_attr(&outarg.attr) ||
-		    fuse_stale_inode(inode, outarg.generation, &outarg.attr))
+		if (ret <= 0)
 			goto invalid;
 
 		forget_all_cached_acls(inode);
-- 
2.25.1

[RFC PATCH 2/2] fuse: ensure that submounts lookup their root

[Krister Johansen]

Prior to this commit, the submount code assumed that the inode for the
root filesystem could not be evicted.  When eviction occurs the server
may forget the inode.  This author has observed a submount get an EBADF
from a virtiofsd server that resulted from the sole dentry / inode
pair getting evicted from a mount namespace and superblock where they
were originally referenced.  The dentry shrinker triggered a forget
after killing the dentry with the last reference.

As a result, a container that was also using this submount failed to
access its filesystem because it had borrowed the reference instead of
taking its own when setting up its superblock for the submount.

Fix by ensuring that submount superblock configuration looks up the
nodeid for the submount as well.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 fs/fuse/dir.c    | 10 +++++-----
 fs/fuse/fuse_i.h |  6 ++++++
 fs/fuse/inode.c  | 32 ++++++++++++++++++++++++++++----
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index bdf5526a0733..fe6b3fd4a49c 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -193,11 +193,11 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
 	args->out_args[0].value = outarg;
 }
 
-static int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
-					 struct dentry *entry,
-					 struct inode *inode,
-					 struct fuse_entry_out *outarg,
-					 bool *lookedup)
+int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
+				  struct dentry *entry,
+				  struct inode *inode,
+				  struct fuse_entry_out *outarg,
+				  bool *lookedup)
 {
 	struct dentry *parent;
 	struct fuse_forget_link *forget;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 9b7fc7d3c7f1..77b123eddb6d 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1309,6 +1309,12 @@ void fuse_dax_dontcache(struct inode *inode, unsigned int flags);
 bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
 void fuse_dax_cancel_work(struct fuse_conn *fc);
 
+/* dir.c */
+int fuse_dentry_revalidate_lookup(struct fuse_mount *fm, struct dentry *entry,
+				  struct inode *inode,
+				  struct fuse_entry_out *outarg,
+				  bool *lookedup);
+
 /* ioctl.c */
 long fuse_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 long fuse_file_compat_ioctl(struct file *file, unsigned int cmd,
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index f19d748890f0..1032e4b05d9c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1441,6 +1441,10 @@ static int fuse_fill_super_submount(struct super_block *sb,
 	struct super_block *parent_sb = parent_fi->inode.i_sb;
 	struct fuse_attr root_attr;
 	struct inode *root;
+	struct inode *parent;
+	struct dentry *pdent;
+	bool lookedup = false;
+	int ret;
 
 	fuse_sb_defaults(sb);
 	fm->sb = sb;
@@ -1456,14 +1460,34 @@ static int fuse_fill_super_submount(struct super_block *sb,
 	if (parent_sb->s_subtype && !sb->s_subtype)
 		return -ENOMEM;
 
+	/*
+	 * It is necessary to lookup the parent_if->nodeid in case the dentry
+	 * that triggered the automount of the submount is later evicted.
+	 * If this dentry is evicted without the lookup count getting increased
+	 * on the submount root, then the server can subsequently forget this
+	 * nodeid which leads to errors when trying to access the root of the
+	 * submount.
+	 */
+	parent = &parent_fi->inode;
+	pdent = d_find_alias(parent);
+	if (pdent) {
+		struct fuse_entry_out outarg;
+
+		ret = fuse_dentry_revalidate_lookup(fm, pdent, parent, &outarg,
+						    &lookedup);
+		dput(pdent);
+		if (ret < 0)
+			return ret;
+	}
+
 	fuse_fill_attr_from_inode(&root_attr, parent_fi);
 	root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0);
 	/*
-	 * This inode is just a duplicate, so it is not looked up and
-	 * its nlookup should not be incremented.  fuse_iget() does
-	 * that, though, so undo it here.
+	 * fuse_iget() sets nlookup to 1 at creation time.  If this nodeid was
+	 * not successfully looked up then decrement the count.
 	 */
-	get_fuse_inode(root)->nlookup--;
+	if (!lookedup)
+		get_fuse_inode(root)->nlookup--;
 	sb->s_d_op = &fuse_dentry_operations;
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root)
-- 
2.25.1

Re: [RFC PATCH 2/2] fuse: ensure that submounts lookup their root

[Bernd Schubert]

On 7/11/23 03:37, Krister Johansen wrote:
> Prior to this commit, the submount code assumed that the inode for the
> root filesystem could not be evicted.  When eviction occurs the server
> may forget the inode.  This author has observed a submount get an EBADF
> from a virtiofsd server that resulted from the sole dentry / inode
> pair getting evicted from a mount namespace and superblock where they
> were originally referenced.  The dentry shrinker triggered a forget
> after killing the dentry with the last reference.
> 
> As a result, a container that was also using this submount failed to
> access its filesystem because it had borrowed the reference instead of
> taking its own when setting up its superblock for the submount.
> 
> Fix by ensuring that submount superblock configuration looks up the
> nodeid for the submount as well.
> 
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> ---
>   fs/fuse/dir.c    | 10 +++++-----
>   fs/fuse/fuse_i.h |  6 ++++++
>   fs/fuse/inode.c  | 32 ++++++++++++++++++++++++++++----
>   3 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index bdf5526a0733..fe6b3fd4a49c 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -193,11 +193,11 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
>   	args->out_args[0].value = outarg;
>   }
>   
> -static int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> -					 struct dentry *entry,
> -					 struct inode *inode,
> -					 struct fuse_entry_out *outarg,
> -					 bool *lookedup)
> +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> +				  struct dentry *entry,
> +				  struct inode *inode,
> +				  struct fuse_entry_out *outarg,
> +				  bool *lookedup)
>   {
>   	struct dentry *parent;
>   	struct fuse_forget_link *forget;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 9b7fc7d3c7f1..77b123eddb6d 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1309,6 +1309,12 @@ void fuse_dax_dontcache(struct inode *inode, unsigned int flags);
>   bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
>   void fuse_dax_cancel_work(struct fuse_conn *fc);
>   
> +/* dir.c */
> +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm, struct dentry *entry,
> +				  struct inode *inode,
> +				  struct fuse_entry_out *outarg,
> +				  bool *lookedup);
> +
>   /* ioctl.c */
>   long fuse_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>   long fuse_file_compat_ioctl(struct file *file, unsigned int cmd,
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index f19d748890f0..1032e4b05d9c 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1441,6 +1441,10 @@ static int fuse_fill_super_submount(struct super_block *sb,
>   	struct super_block *parent_sb = parent_fi->inode.i_sb;
>   	struct fuse_attr root_attr;
>   	struct inode *root;
> +	struct inode *parent;
> +	struct dentry *pdent;
> +	bool lookedup = false;
> +	int ret;
>   
>   	fuse_sb_defaults(sb);
>   	fm->sb = sb;
> @@ -1456,14 +1460,34 @@ static int fuse_fill_super_submount(struct super_block *sb,
>   	if (parent_sb->s_subtype && !sb->s_subtype)
>   		return -ENOMEM;
>   
> +	/*
> +	 * It is necessary to lookup the parent_if->nodeid in case the dentry
> +	 * that triggered the automount of the submount is later evicted.
> +	 * If this dentry is evicted without the lookup count getting increased
> +	 * on the submount root, then the server can subsequently forget this
> +	 * nodeid which leads to errors when trying to access the root of the
> +	 * submount.
> +	 */
> +	parent = &parent_fi->inode;
> +	pdent = d_find_alias(parent);
> +	if (pdent) {
> +		struct fuse_entry_out outarg;
> +
> +		ret = fuse_dentry_revalidate_lookup(fm, pdent, parent, &outarg,
> +						    &lookedup);
> +		dput(pdent);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	fuse_fill_attr_from_inode(&root_attr, parent_fi);
>   	root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0);
>   	/*
> -	 * This inode is just a duplicate, so it is not looked up and
> -	 * its nlookup should not be incremented.  fuse_iget() does
> -	 * that, though, so undo it here.
> +	 * fuse_iget() sets nlookup to 1 at creation time.  If this nodeid was
> +	 * not successfully looked up then decrement the count.
>   	 */
> -	get_fuse_inode(root)->nlookup--;
> +	if (!lookedup)
> +		get_fuse_inode(root)->nlookup--;

How does a submount work with a parent mismatch? I wonder if this 
function should return an error if lookup of the parent failed.


Thanks,
Bernd

Re: [RFC PATCH 2/2] fuse: ensure that submounts lookup their root

[Krister Johansen]

On Tue, Jul 25, 2023 at 01:16:08AM +0200, Bernd Schubert wrote:
> On 7/11/23 03:37, Krister Johansen wrote:
> > Prior to this commit, the submount code assumed that the inode for the
> > root filesystem could not be evicted.  When eviction occurs the server
> > may forget the inode.  This author has observed a submount get an EBADF
> > from a virtiofsd server that resulted from the sole dentry / inode
> > pair getting evicted from a mount namespace and superblock where they
> > were originally referenced.  The dentry shrinker triggered a forget
> > after killing the dentry with the last reference.
> > 
> > As a result, a container that was also using this submount failed to
> > access its filesystem because it had borrowed the reference instead of
> > taking its own when setting up its superblock for the submount.
> > 
> > Fix by ensuring that submount superblock configuration looks up the
> > nodeid for the submount as well.
> > 
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> >   fs/fuse/dir.c    | 10 +++++-----
> >   fs/fuse/fuse_i.h |  6 ++++++
> >   fs/fuse/inode.c  | 32 ++++++++++++++++++++++++++++----
> >   3 files changed, 39 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index bdf5526a0733..fe6b3fd4a49c 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -193,11 +193,11 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
> >   	args->out_args[0].value = outarg;
> >   }
> > -static int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> > -					 struct dentry *entry,
> > -					 struct inode *inode,
> > -					 struct fuse_entry_out *outarg,
> > -					 bool *lookedup)
> > +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> > +				  struct dentry *entry,
> > +				  struct inode *inode,
> > +				  struct fuse_entry_out *outarg,
> > +				  bool *lookedup)
> >   {
> >   	struct dentry *parent;
> >   	struct fuse_forget_link *forget;
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 9b7fc7d3c7f1..77b123eddb6d 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -1309,6 +1309,12 @@ void fuse_dax_dontcache(struct inode *inode, unsigned int flags);
> >   bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
> >   void fuse_dax_cancel_work(struct fuse_conn *fc);
> > +/* dir.c */
> > +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm, struct dentry *entry,
> > +				  struct inode *inode,
> > +				  struct fuse_entry_out *outarg,
> > +				  bool *lookedup);
> > +
> >   /* ioctl.c */
> >   long fuse_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> >   long fuse_file_compat_ioctl(struct file *file, unsigned int cmd,
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index f19d748890f0..1032e4b05d9c 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1441,6 +1441,10 @@ static int fuse_fill_super_submount(struct super_block *sb,
> >   	struct super_block *parent_sb = parent_fi->inode.i_sb;
> >   	struct fuse_attr root_attr;
> >   	struct inode *root;
> > +	struct inode *parent;
> > +	struct dentry *pdent;
> > +	bool lookedup = false;
> > +	int ret;
> >   	fuse_sb_defaults(sb);
> >   	fm->sb = sb;
> > @@ -1456,14 +1460,34 @@ static int fuse_fill_super_submount(struct super_block *sb,
> >   	if (parent_sb->s_subtype && !sb->s_subtype)
> >   		return -ENOMEM;
> > +	/*
> > +	 * It is necessary to lookup the parent_if->nodeid in case the dentry
> > +	 * that triggered the automount of the submount is later evicted.
> > +	 * If this dentry is evicted without the lookup count getting increased
> > +	 * on the submount root, then the server can subsequently forget this
> > +	 * nodeid which leads to errors when trying to access the root of the
> > +	 * submount.
> > +	 */
> > +	parent = &parent_fi->inode;
> > +	pdent = d_find_alias(parent);
> > +	if (pdent) {
> > +		struct fuse_entry_out outarg;
> > +
> > +		ret = fuse_dentry_revalidate_lookup(fm, pdent, parent, &outarg,
> > +						    &lookedup);
> > +		dput(pdent);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >   	fuse_fill_attr_from_inode(&root_attr, parent_fi);
> >   	root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0);
> >   	/*
> > -	 * This inode is just a duplicate, so it is not looked up and
> > -	 * its nlookup should not be incremented.  fuse_iget() does
> > -	 * that, though, so undo it here.
> > +	 * fuse_iget() sets nlookup to 1 at creation time.  If this nodeid was
> > +	 * not successfully looked up then decrement the count.
> >   	 */
> > -	get_fuse_inode(root)->nlookup--;
> > +	if (!lookedup)
> > +		get_fuse_inode(root)->nlookup--;
> 
> How does a submount work with a parent mismatch? I wonder if this function
> should return an error if lookup of the parent failed.

Thanks for the feedback.  This was definitely one of the things that I
didn't like about this patch.  The original code doesn't care if the
parent nodeid is valid, which is why I kept the old behavior as a
fallback if lookup failed.  I managed to console myself that it wasn't
any worse, but would love to remove that if it's okay to fail the mount
at this point.

The other thing that was annoying me was my refactoring of the
fuse_dentry_revalidate() function.  I'm annoyed by having the lookedup
boolean pointer as an argument to fuse_dentry_revalidate_lookup, but
couldn't quite work out how to handle the weird nlookup manipulations in
fuse_dentry_revalidate and fuse_fill_super_submount without it.

-K