kernel BUG at fs/notify/dnotify/dnotify.c:LINE! (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 27bba9c5 Merge tag 'scsi-fixes' of git://git.kernel.org/pu..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11b82225500000
kernel config: https://syzkaller.appspot.com/x/.config?x=330f3436df12fd44
dashboard link: https://syzkaller.appspot.com/bug?extid=f427adf9324b92652ccc
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11d3f015500000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17162d4d500000

Bisection is inconclusive: the issue happens on the oldest tested release.

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16570525500000
final oops: https://syzkaller.appspot.com/x/report.txt?x=15570525500000
console output: https://syzkaller.appspot.com/x/log.txt?x=11570525500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f427ad...@syzkaller.appspotmail.com

wlan1: Creating new IBSS network, BSSID 50:50:50:50:50:50
------------[ cut here ]------------
kernel BUG at fs/notify/dnotify/dnotify.c:118!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 648 Comm: kworker/u4:4 Not tainted 5.10.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: events_unbound fsnotify_mark_destroy_workfn
RIP: 0010:dnotify_free_mark fs/notify/dnotify/dnotify.c:118 [inline]
RIP: 0010:dnotify_free_mark+0x4b/0x60 fs/notify/dnotify/dnotify.c:112
Code: 80 3c 02 00 75 26 48 83 bd 80 00 00 00 00 75 15 e8 0a d3 a0 ff 48 89 ee 48 8b 3d 68 8c 1d 0b 5d e9 aa 06 e2 ff e8 f5 d2 a0 ff <0f> 0b e8 ae 4d e2 ff eb d3 66 90 66 2e 0f 1f 84 00 00 00 00 00 41
RSP: 0018:ffffc90002f1fc38 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffffff8958ae60 RCX: 1ffff920005e3f95
RDX: ffff888012601a40 RSI: ffffffff81cf5ceb RDI: ffff88801aea2080
RBP: ffff88801aea2000 R08: 0000000000000001 R09: ffffffff8ebb170f
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880171a2000
R13: ffffc90002f1fc98 R14: ffff88801aea2010 R15: ffff88801aea2018
FS: 0000000000000000(0000) GS:ffff8880b9f00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000056045fa95978 CR3: 0000000012121000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
fsnotify_final_mark_destroy+0x71/0xb0 fs/notify/mark.c:205
fsnotify_mark_destroy_workfn+0x1eb/0x340 fs/notify/mark.c:840
process_one_work+0x933/0x15a0 kernel/workqueue.c:2272
worker_thread+0x64c/0x1120 kernel/workqueue.c:2418
kthread+0x3af/0x4a0 kernel/kthread.c:292
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296
Modules linked in:


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Jan Kara]

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?

Honza

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

[Jan Kara]

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?

[Miklos Szeredi]

On Wed, Dec 9, 2020 at 2:59 PM Jan Kara <ja...@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...

Thanks,
Miklos

[Jan Kara]

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

Tested-by: Jan Kara <ja...@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);

[Evgenii Shatokhin]

Hi,

I have checked that the suggested patch fixes the issue in the kernels from RHEL7 too.

Miklos, do you plan to send this fix to mainline?

среда, 9 декабря 2020 г. в 19:15:15 UTC+3, Miklos Szeredi:

Regards, 

Evgenii

[Evgenii Shatokhin]

OK, I see, the fix is in the mainline now:

commit 5d069dbe8aaf2a197142558b6fb2978189ba3454

Author: Miklos Szeredi <msze...@redhat.com>

Date:   Thu Dec 10 15:33:14 2020 +0100

    fuse: fix bad inode

Thanks a lot!

среда, 23 декабря 2020 г. в 17:21:54 UTC+3, Evgenii Shatokhin: