Re: [PATCH] fuse: fix illegal access to inode with reused nodeid

[Miklos Szeredi <miklos@szeredi.hu>]

On Wed, 9 Jun 2021 at 20:12, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Server responds to LOOKUP and other ops (READDIRPLUS/CREATE/MKNOD/...)
> with outarg containing nodeid and generation.
>
> If a fuse inode is found in inode cache with the same nodeid but
> different generation, the existing fuse inode should be unhashed and
> marked "bad" and a new inode with the new generation should be hashed
> instead.
>
> This can happen, for example, with passhrough fuse filesystem that
> returns the real filesystem ino/generation on lookup and where real inode
> numbers can get recycled due to real files being unlinked not via the fuse
> passthrough filesystem.
>
> With current code, this situation will not be detected and an old fuse
> dentry that used to point to an older generation real inode, can be used
> to access a completely new inode, which should be accessed only via the
> new dentry.
>
> Note that because the FORGET message carries the nodeid w/o generation,
> the server should wait to get FORGET counts for the nlookup counts of
> the old and reused inodes combined, before it can free the resources
> associated to that nodeid.
>
> Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxgDMGUpK35huwqFYGH_idBB8S6eLiz85o0DDKOyDH4Syg@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> I was able to reproduce this issue with a passthrough fs that stored
> ino+generation and uses then to open fd on lookup.
>
> I extended libfuse's test_syscalls [1] program to demonstrate the issue
> described in commit message.
>
> Max, IIUC, you are making a modification to virtiofs-rs that would
> result is being exposed to this bug.  You are welcome to try out the
> test and let me know if you can reproduce the issue.
>
> Note that some test_syscalls test fail with cache enabled, so libfuse's
> test_examples.py only runs test_syscalls in cache disabled config.
>
> Thanks,
> Amir.
>
> [1] https://github.com/amir73il/libfuse/commits/test-reused-inodes
>
>  fs/fuse/dir.c     | 3 ++-
>  fs/fuse/fuse_i.h  | 9 +++++++++
>  fs/fuse/inode.c   | 4 ++--
>  fs/fuse/readdir.c | 7 +++++--
>  4 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 1b6c001a7dd1..b06628fd7d8e 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -239,7 +239,8 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>                 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_stale_inode(inode, outarg.generation,
> +                                            &outarg.attr)) {

This changes behavior in the inode_wrong_type() case.  I guess that
was not intended.


>                                 fuse_queue_forget(fm->fc, forget,
>                                                   outarg.nodeid, 1);
>                                 goto invalid;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 7e463e220053..f1bd28c176a9 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -867,6 +867,15 @@ static inline u64 fuse_get_attr_version(struct fuse_conn *fc)
>         return atomic64_read(&fc->attr_version);
>  }
>
> +static inline bool fuse_stale_inode(const struct inode *inode, int generation,
> +                                   struct fuse_attr *attr)
> +{
> +       return inode->i_generation != generation ||
> +               inode_wrong_type(inode, attr->mode) ||
> +               (bool) IS_AUTOMOUNT(inode) !=
> +               (bool) (attr->flags & FUSE_ATTR_SUBMOUNT);
> +}
> +
>  static inline void fuse_make_bad(struct inode *inode)
>  {
>         remove_inode_hash(inode);
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 393e36b74dc4..257bb3e1cac8 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -350,8 +350,8 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>                 inode->i_generation = generation;
>                 fuse_init_inode(inode, attr);
>                 unlock_new_inode(inode);
> -       } else if (inode_wrong_type(inode, attr->mode)) {
> -               /* Inode has changed type, any I/O on the old should fail */
> +       } else if (fuse_stale_inode(inode, generation, attr)) {

This one adds the automount check.  That should be okay, since
directories must never have aliases and such a beast would error out
anyway later on d_splice_alias.

> +               /* nodeid was reused, any I/O on the old inode should fail */
>                 fuse_make_bad(inode);
>                 iput(inode);
>                 goto retry;
> diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
> index 277f7041d55a..bc267832310c 100644
> --- a/fs/fuse/readdir.c
> +++ b/fs/fuse/readdir.c
> @@ -200,9 +200,12 @@ static int fuse_direntplus_link(struct file *file,
>         if (!d_in_lookup(dentry)) {
>                 struct fuse_inode *fi;
>                 inode = d_inode(dentry);
> +               if (inode && get_node_id(inode) != o->nodeid)
> +                       inode = NULL;
>                 if (!inode ||
> -                   get_node_id(inode) != o->nodeid ||
> -                   inode_wrong_type(inode, o->attr.mode)) {
> +                   fuse_stale_inode(inode, o->generation, &o->attr)) {

Again, automount check added, I'm not at all sure how automount and
readdirplus interact.    This needs to be looked at (though it's
mostly a separate issue from this patch).

Thanks,
Miklos