On Wed, Dec 9, 2020 at 2:59 PM Jan Kara <jack@xxxxxxx> 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...
Thanks,
Miklos
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);
