On Wed 09-12-20 17:15:02, Miklos Szeredi wrote:
> On Wed, Dec 9, 2020 at 2:59 PM Jan Kara <j...@suse.cz> wrote:
> >
> > On Wed 09-12-20 14:38:42, Jan Kara wrote:
> > > Hello!
> > >
> > > so I was debugging the dnotify crash below (it's 100% reproducible for me)
> > > and I came to the following. The reproducer opens 'file0' on FUSE
> > > filesystem which is a directory at that point. Then it attached dnotify
> > > mark to the directory 'file0' and then it does something to the FUSE fs
> > > which I don't understand but the result is that when FUSE is unmounted the
> > > 'file0' inode is actually a regular file (note that I've verified this is
> > > really the same inode pointer). This then confuses dnotify which doesn't
> > > tear down its structures properly and eventually crashes. So my question
> > > is: How can an inode on FUSE filesystem morph from a dir to a regular 
> > > file?
> > > I presume this could confuse much more things than just dnotify?
> > >
> > > Before I dwelve more into FUSE internals, any idea Miklos what could have
> > > gone wrong and how to debug this further?
> >
> > I've got an idea where to look and indeed it is the fuse_do_getattr() call
> > that finds attributes returned by the server are inconsistent so it calls
> > make_bad_inode() which, among other things, does:
> >
> >         inode->i_mode = S_IFREG;
> >
> > Indeed calling make_bad_inode() on a live inode doesn't look like a good
> > idea. IMHO FUSE needs to come up with some other means of marking the inode
> > as stale. Miklos?
> 
> Something like the attached.  It's untested and needs the
> fuse_is_bad() test in more ops...

The patch fixes the problem for me (the reproducer no longer crashes the
kernel). So feel free to add:

Tested-by: Jan Kara <j...@suse.cz>

                                                                Honza

> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index ff7dbeb16f88..1172179c9fba 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -202,7 +202,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, 
> unsigned int flags)
>       int ret;
>  
>       inode = d_inode_rcu(entry);
> -     if (inode && is_bad_inode(inode))
> +     if (inode && fuse_is_bad(inode))
>               goto invalid;
>       else if (time_before64(fuse_dentry_time(entry), get_jiffies_64()) ||
>                (flags & LOOKUP_REVAL)) {
> @@ -1030,7 +1030,7 @@ static int fuse_do_getattr(struct inode *inode, struct 
> kstat *stat,
>       if (!err) {
>               if (fuse_invalid_attr(&outarg.attr) ||
>                   (inode->i_mode ^ outarg.attr.mode) & S_IFMT) {
> -                     make_bad_inode(inode);
> +                     fuse_make_bad(inode);
>                       err = -EIO;
>               } else {
>                       fuse_change_attributes(inode, &outarg.attr,
> @@ -1327,7 +1327,7 @@ static const char *fuse_get_link(struct dentry *dentry, 
> struct inode *inode,
>       int err;
>  
>       err = -EIO;
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               goto out_err;
>  
>       if (fc->cache_symlinks)
> @@ -1375,7 +1375,7 @@ static int fuse_dir_fsync(struct file *file, loff_t 
> start, loff_t end,
>       struct fuse_conn *fc = get_fuse_conn(inode);
>       int err;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       if (fc->no_fsyncdir)
> @@ -1664,7 +1664,7 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr 
> *attr,
>  
>       if (fuse_invalid_attr(&outarg.attr) ||
>           (inode->i_mode ^ outarg.attr.mode) & S_IFMT) {
> -             make_bad_inode(inode);
> +             fuse_make_bad(inode);
>               err = -EIO;
>               goto error;
>       }
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index c03034e8c152..30fdb3adf9b9 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -463,7 +463,7 @@ static int fuse_flush(struct file *file, fl_owner_t id)
>       FUSE_ARGS(args);
>       int err;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       err = write_inode_now(inode, 1);
> @@ -535,7 +535,7 @@ static int fuse_fsync(struct file *file, loff_t start, 
> loff_t end,
>       struct fuse_conn *fc = get_fuse_conn(inode);
>       int err;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       inode_lock(inode);
> @@ -859,7 +859,7 @@ static int fuse_readpage(struct file *file, struct page 
> *page)
>       int err;
>  
>       err = -EIO;
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               goto out;
>  
>       err = fuse_do_readpage(file, page);
> @@ -952,7 +952,7 @@ static void fuse_readahead(struct readahead_control *rac)
>       struct fuse_conn *fc = get_fuse_conn(inode);
>       unsigned int i, max_pages, nr_pages = 0;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return;
>  
>       max_pages = min_t(unsigned int, fc->max_pages,
> @@ -1555,7 +1555,7 @@ static ssize_t fuse_file_read_iter(struct kiocb *iocb, 
> struct iov_iter *to)
>       struct fuse_file *ff = file->private_data;
>       struct inode *inode = file_inode(file);
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       if (FUSE_IS_DAX(inode))
> @@ -1573,7 +1573,7 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, 
> struct iov_iter *from)
>       struct fuse_file *ff = file->private_data;
>       struct inode *inode = file_inode(file);
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       if (FUSE_IS_DAX(inode))
> @@ -2172,7 +2172,7 @@ static int fuse_writepages(struct address_space 
> *mapping,
>       int err;
>  
>       err = -EIO;
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               goto out;
>  
>       data.inode = inode;
> @@ -2954,7 +2954,7 @@ long fuse_ioctl_common(struct file *file, unsigned int 
> cmd,
>       if (!fuse_allow_current_process(fc))
>               return -EACCES;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       return fuse_do_ioctl(file, cmd, arg, flags);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index d51598017d13..8484f0053687 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -172,6 +172,8 @@ enum {
>       FUSE_I_INIT_RDPLUS,
>       /** An operation changing file size is in progress  */
>       FUSE_I_SIZE_UNSTABLE,
> +     /* Bad inode */
> +     FUSE_I_BAD,
>  };
>  
>  struct fuse_conn;
> @@ -858,6 +860,16 @@ static inline u64 fuse_get_attr_version(struct fuse_conn 
> *fc)
>       return atomic64_read(&fc->attr_version);
>  }
>  
> +static inline void fuse_make_bad(struct inode *inode)
> +{
> +     set_bit(FUSE_I_BAD, &get_fuse_inode(inode)->state);
> +}
> +
> +static inline bool fuse_is_bad(struct inode *inode)
> +{
> +     return test_bit(FUSE_I_BAD, &get_fuse_inode(inode)->state);
> +}
> +
>  /** Device operations */
>  extern const struct file_operations fuse_dev_operations;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 1a47afc95f80..f94b0bb57619 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -132,7 +132,7 @@ static void fuse_evict_inode(struct inode *inode)
>                       fi->forget = NULL;
>               }
>       }
> -     if (S_ISREG(inode->i_mode) && !is_bad_inode(inode)) {
> +     if (S_ISREG(inode->i_mode) && !fuse_is_bad(inode)) {
>               WARN_ON(!list_empty(&fi->write_files));
>               WARN_ON(!list_empty(&fi->queued_writes));
>       }
> @@ -342,7 +342,7 @@ struct inode *fuse_iget(struct super_block *sb, u64 
> nodeid,
>               unlock_new_inode(inode);
>       } else if ((inode->i_mode ^ attr->mode) & S_IFMT) {
>               /* Inode has changed type, any I/O on the old should fail */
> -             make_bad_inode(inode);
> +             fuse_make_bad(inode);
>               iput(inode);
>               goto retry;
>       }
> diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
> index 3b5e91045871..3441ffa740f3 100644
> --- a/fs/fuse/readdir.c
> +++ b/fs/fuse/readdir.c
> @@ -207,7 +207,7 @@ static int fuse_direntplus_link(struct file *file,
>                       dput(dentry);
>                       goto retry;
>               }
> -             if (is_bad_inode(inode)) {
> +             if (fuse_is_bad(inode)) {
>                       dput(dentry);
>                       return -EIO;
>               }
> @@ -568,7 +568,7 @@ int fuse_readdir(struct file *file, struct dir_context 
> *ctx)
>       struct inode *inode = file_inode(file);
>       int err;
>  
> -     if (is_bad_inode(inode))
> +     if (fuse_is_bad(inode))
>               return -EIO;
>  
>       mutex_lock(&ff->readdir.lock);

-- 
Jan Kara <j...@suse.com>
SUSE Labs, CR

Reply via email to