On Mon, Jun 21, 2021 at 12:27 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Wed, 9 Jun 2021 at 20:12, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > 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. > Right. fixed in v2. > > > 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). > I removed the automount check completely from fuse_stale_inode() for v2. Thanks, Amir.
