[syzbot] [ext4?] WARNING in lock_two_nondirectories

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: a39b6ac3781d Linux 6.7-rc5
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=12c3a112e80000
kernel config: https://syzkaller.appspot.com/x/.config?x=e043d554f0a5f852
dashboard link: https://syzkaller.appspot.com/bug?extid=2c4a3b922a860084cc7f
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1687292ee80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16d8adbce80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/67fd20dff9bc/disk-a39b6ac3.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/778677113ec4/vmlinux-a39b6ac3.xz
kernel image: https://storage.googleapis.com/syzbot-assets/fd69b2e7d493/bzImage-a39b6ac3.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/28ab13ef564b/mount_0.gz

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

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1286a2b2e80000
final oops: https://syzkaller.appspot.com/x/report.txt?x=1186a2b2e80000
console output: https://syzkaller.appspot.com/x/log.txt?x=1686a2b2e80000

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

EXT4-fs (loop0): mounted filesystem 00000000-0000-0000-0000-000000000000 r/w without journal. Quota mode: none.
------------[ cut here ]------------
WARNING: CPU: 1 PID: 5067 at fs/inode.c:1148 lock_two_nondirectories+0xca/0x100 fs/inode.c:1148
Modules linked in:
CPU: 1 PID: 5067 Comm: syz-executor207 Not tainted 6.7.0-rc5-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/10/2023
RIP: 0010:lock_two_nondirectories+0xca/0x100 fs/inode.c:1148
Code: ff 66 41 81 fc 00 40 74 1b e8 c2 3d 92 ff 48 89 ee 48 89 df 5b 5d b9 04 00 00 00 31 d2 41 5c e9 5c fd ff ff e8 a7 3d 92 ff 90 <0f> 0b 90 eb da e8 9c 3d 92 ff 90 0f 0b 90 eb 83 48 89 df e8 5e e4
RSP: 0018:ffffc90003a4fc38 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffff88807cf82ab0 RCX: ffffffff81f55031
RDX: ffff888078b21dc0 RSI: ffffffff81f55059 RDI: 0000000000000003
RBP: ffff88807cfe66b0 R08: 0000000000000003 R09: 0000000000004000
R10: 0000000000004000 R11: ffffffff915fc8a0 R12: 0000000000004000
R13: ffff8880298d2c80 R14: ffff88807cfe66b0 R15: ffffffff8d195740
FS: 0000555555d51380(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000056e5a000 CR4: 0000000000350ef0
Call Trace:
<TASK>
swap_inode_boot_loader fs/ext4/ioctl.c:391 [inline]
__ext4_ioctl+0x118d/0x4570 fs/ext4/ioctl.c:1437
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl fs/ioctl.c:857 [inline]
__x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f61c40c4af9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe0eaa3fb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007ffe0eaa4198 RCX: 00007f61c40c4af9
RDX: 0000000000000000 RSI: 0000000000006611 RDI: 0000000000000004
RBP: 00007f61c4138610 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffe0eaa4188 R14: 0000000000000001 R15: 0000000000000001
</TASK>


---
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

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Edward Adam Davis]

please test WARNING in lock_two_nondirectories

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a39b6ac3781d

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 61277f7f8722..692376f3ce0a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4710,6 +4710,7 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
}

inode = iget_locked(sb, ino);
+ printk("ino: %u, in: %p, %s\n", ino, inode, __func__);
if (!inode)
return ERR_PTR(-ENOMEM);
if (!(inode->i_state & I_NEW)) {
@@ -4944,8 +4945,13 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
inode->i_fop = &ext4_file_operations;
ext4_set_aops(inode);
} else if (S_ISDIR(inode->i_mode)) {
- inode->i_op = &ext4_dir_inode_operations;
- inode->i_fop = &ext4_dir_operations;
+ printk("i: %p, %s\n", inode, __func__);
+ if (ino == EXT4_BOOT_LOADER_INO)
+ make_bad_inode(inode);
+ else {
+ inode->i_op = &ext4_dir_inode_operations;
+ inode->i_fop = &ext4_dir_operations;
+ }
} else if (S_ISLNK(inode->i_mode)) {
/* VFS does not allow setting these so must be corruption */
if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+2c4a3b...@syzkaller.appspotmail.com

Tested on:

commit: a39b6ac3 Linux 6.7-rc5
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=156be7a5e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=e043d554f0a5f852
dashboard link: https://syzkaller.appspot.com/bug?extid=2c4a3b922a860084cc7f
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=13c773d6e80000

Note: testing is done by a robot and is best-effort only.

[Edward Adam Davis]

If inode is the ext4 boot loader inode, then when it is a directory, the inode
should also be set to bad inode.

Reported-and-tested-by: syzbot+2c4a3b...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/ext4/inode.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 61277f7f8722..b311f610f008 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,

inode->i_fop = &ext4_file_operations;
ext4_set_aops(inode);
} else if (S_ISDIR(inode->i_mode)) {
- inode->i_op = &ext4_dir_inode_operations;
- inode->i_fop = &ext4_dir_operations;

+ if (ino == EXT4_BOOT_LOADER_INO)
+ make_bad_inode(inode);
+ else {
+ inode->i_op = &ext4_dir_inode_operations;
+ inode->i_fop = &ext4_dir_operations;
+ }
} else if (S_ISLNK(inode->i_mode)) {
/* VFS does not allow setting these so must be corruption */
if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {

--
2.43.0

[Matthew Wilcox]

On Sun, Dec 24, 2023 at 07:53:00PM +0800, Edward Adam Davis wrote:
> If inode is the ext4 boot loader inode, then when it is a directory, the inode
> should also be set to bad inode.

... what? Are you saying that syzbot has randomly created a filesystem
where the boot loader inode is a directory? If so, I'd suggest just
rejecting the filesystem with EFSCORRUPT.

[Baokun Li]

On 2023/12/24 19:53, Edward Adam Davis wrote:
> If inode is the ext4 boot loader inode, then when it is a directory, the inode
> should also be set to bad inode.
>
> Reported-and-tested-by: syzbot+2c4a3b...@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> fs/ext4/inode.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 61277f7f8722..b311f610f008 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
> inode->i_fop = &ext4_file_operations;
> ext4_set_aops(inode);
> } else if (S_ISDIR(inode->i_mode)) {
> - inode->i_op = &ext4_dir_inode_operations;
> - inode->i_fop = &ext4_dir_operations;
> + if (ino == EXT4_BOOT_LOADER_INO)
> + make_bad_inode(inode);

Marking the boot loader inode as a bad inode here is useless,
EXT4_IGET_BAD allows us to get a bad boot loader inode.
In my opinion, it doesn't make sense to call lock_two_nondirectories()
here to determine if the inode is a regular file or not, since the logic
for dealing with non-regular files comes after the locking, so calling
lock_two_inodes() directly here will suffice.

Merry Christmas!
Baokun

[Al Viro]

On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote:

> In my opinion, it doesn't make sense to call lock_two_nondirectories()
> here to determine if the inode is a regular file or not, since the logic
> for dealing with non-regular files comes after the locking, so calling
> lock_two_inodes() directly here will suffice.

No. First of all, lock_two_inodes() is a mistake that is going to be
removed in the coming cycle.

What's more, why the hell do you need to lock *anything* to check the
inode type? Inode type never changes, period.

Just take that check prior to lock_two_nondirectories() and be done with
that.

[Theodore Ts'o]

On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote:

> Marking the boot loader inode as a bad inode here is useless,
> EXT4_IGET_BAD allows us to get a bad boot loader inode.
> In my opinion, it doesn't make sense to call lock_two_nondirectories()
> here to determine if the inode is a regular file or not, since the logic
> for dealing with non-regular files comes after the locking, so calling
> lock_two_inodes() directly here will suffice.

This is all very silly, and why I consider this sort of thing pure
syzkaller noise. It really doesn't protect against any real threat,
and it encourages people to put all sorts of random crud in kernel
code, all in the name of trying to shut up syzbot.

If we *are* going to care about shutting up syzkaller, the right
approach is to simply add a check in swap_inode_boot_loader() which
causes it to call ext4_error() and declare the file system corrupted
if the bootloader inode is not a regular file, and then return
-EFSCORRUPTED.

We don't need to add random hacks to ext4_iget(), or in other places...

- Ted

[Al Viro]

Just check the inode type before anything else and be done with that -
if an in-core inode of a regular file manages to become a directory
right under us, we have a much worse problem.

IOW, the bug is real, but suggested patch is just plain wrong.

[Baokun Li]

On 2023/12/25 10:07, Al Viro wrote:
> On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote:
>
>> In my opinion, it doesn't make sense to call lock_two_nondirectories()
>> here to determine if the inode is a regular file or not, since the logic
>> for dealing with non-regular files comes after the locking, so calling
>> lock_two_inodes() directly here will suffice.
> No. First of all, lock_two_inodes() is a mistake that is going to be
> removed in the coming cycle.

Okay, I didn't know about this.

> What's more, why the hell do you need to lock *anything* to check the
> inode type? Inode type never changes, period.
>
> Just take that check prior to lock_two_nondirectories() and be done with
> that.

Since in the current logic we update the boot loader file via
swap_inode_boot_loader(), however the boot loader inode on disk
may be uninitialized and may be garbage data, so we allow to get a
bad boot loader inode and then initialize it and swap it with the boot
loader file to be set.
When reinitializing the bad boot loader inode, something like an
inode type conversion may occur.

Cheers,
Baokun

[Baokun Li]

On 2023/12/25 10:11, Theodore Ts'o wrote:
> On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote:
>> Marking the boot loader inode as a bad inode here is useless,
>> EXT4_IGET_BAD allows us to get a bad boot loader inode.
>> In my opinion, it doesn't make sense to call lock_two_nondirectories()
>> here to determine if the inode is a regular file or not, since the logic
>> for dealing with non-regular files comes after the locking, so calling
>> lock_two_inodes() directly here will suffice.
> This is all very silly, and why I consider this sort of thing pure
> syzkaller noise. It really doesn't protect against any real threat,
> and it encourages people to put all sorts of random crud in kernel
> code, all in the name of trying to shut up syzbot.

Indeed, the warning is meaningless, but it is undeniable that if the
user can easily trigger the warning, something is wrong with the code.

> If we *are* going to care about shutting up syzkaller, the right
> approach is to simply add a check in swap_inode_boot_loader() which
> causes it to call ext4_error() and declare the file system corrupted
> if the bootloader inode is not a regular file, and then return
> -EFSCORRUPTED.
>
> We don't need to add random hacks to ext4_iget(), or in other places...
>
> - Ted

Without considering the case where the boot loader inode is
uninitialized, I think this is fine and the logic to determine if the boot
loader inode is initialized and to initialize it can be removed.

Merry Christmas!
--
With Best Regards,
Baokun Li
.

[Theodore Ts'o]

On Mon, Dec 25, 2023 at 10:33:20AM +0800, Baokun Li wrote:
> Since in the current logic we update the boot loader file via
> swap_inode_boot_loader(), however the boot loader inode on disk
> may be uninitialized and may be garbage data, so we allow to get a
> bad boot loader inode and then initialize it and swap it with the boot
> loader file to be set.
> When reinitializing the bad boot loader inode, something like an
> inode type conversion may occur.

Yes, but the boot laoder inode is *either* all zeros, or a regular
file. If it's a directory, then it's a malicious syzbot trying to
mess with our minds.

Aside from the warning, it's pretty harmless, but it will very likely
result in a corrupted file system --- but the file system was
corrupted in the first place. So who cares?

Just check to make sure that i_mode is either 0, or regular file, and
return EFSCORRUPTEd, and we're done.

- Ted

[Baokun Li]

Yes, this seems to work, but for that matter, when i_mode is 0, we
still trigger the WARN_ON_ONCE in lock_two_nondirectories().

[syzbot]

syzbot suspects this issue was fixed by commit:

commit 6f861765464f43a71462d52026fbddfc858239a5
Author: Jan Kara <ja...@suse.cz>
Date: Wed Nov 1 17:43:10 2023 +0000

fs: Block writes to mounted block devices

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15477434180000
start commit: a39b6ac3781d Linux 6.7-rc5
git tree: upstream

kernel config: https://syzkaller.appspot.com/x/.config?x=e043d554f0a5f852
dashboard link: https://syzkaller.appspot.com/bug?extid=2c4a3b922a860084cc7f

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1687292ee80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16d8adbce80000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: fs: Block writes to mounted block devices

[Jan Kara]

On Sun 11-02-24 16:00:04, syzbot wrote:
> syzbot suspects this issue was fixed by commit:
>
> commit 6f861765464f43a71462d52026fbddfc858239a5
> Author: Jan Kara <ja...@suse.cz>
> Date: Wed Nov 1 17:43:10 2023 +0000
>
> fs: Block writes to mounted block devices
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15477434180000
> start commit: a39b6ac3781d Linux 6.7-rc5
> git tree: upstream
> kernel config: https://syzkaller.appspot.com/x/.config?x=e043d554f0a5f852
> dashboard link: https://syzkaller.appspot.com/bug?extid=2c4a3b922a860084cc7f
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1687292ee80000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16d8adbce80000
>
> If the result looks correct, please mark the issue as fixed by replying with:

Another repro that seems to be corrupting the fs metadata:

#syz fix: fs: Block writes to mounted block devices

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