[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6a73a722-6bb5-6462-e7ff-a55866374758@fastmail.fm>
Date: Tue, 25 Jul 2023 01:16:08 +0200
From: Bernd Schubert <bernd.schubert@...tmail.fm>
To: Krister Johansen <kjlx@...pleofstupid.com>,
Miklos Szeredi <miklos@...redi.hu>,
linux-fsdevel@...r.kernel.org
Cc: linux-kernel@...r.kernel.org,
German Maglione <gmaglione@...hat.com>,
Greg Kurz <groug@...d.org>, Max Reitz <mreitz@...hat.com>
Subject: Re: [RFC PATCH 2/2] fuse: ensure that submounts lookup their root
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@...pleofstupid.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
Powered by blists - more mailing lists