| Message ID | 20241112073421.GD1458936@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: possible circular locking dependency at ext4_xattr_inode_create | expand |
On Tue, Nov 12, 2024 at 04:34:21PM +0900, Sergey Senozhatsky wrote: > > I've a following syzkaller report (no reproducer); the report is > against 5.15, but the same call-chain seems possible in current > upstream as well. So I suspect that maybe ext4_xattr_inode_create() > should take nested inode_lock (I_MUTEX_XATTR) instead. Does the > patch below make any sense? These syzkaller reports result from mounting a corrupted (fuzzed) file system typically when an inode is used in multiple contexts (e.g., as a directory and an EA inode, etc.) at the same time. I'd have to take a closer look to see if it makes sense, but in general, very often whenever we try to fix one of these it ends up triggering some other syzkaller failure. And, these sorts of things don't actually result in actual security problems (at worst, a hang / denial of service attack), and the right thing to do is to just run fsck on the !@#?!? file system before mounting the thing. The best way to protect systems against threat model of users picking up a random USB stick dropped in a parking lot that contains a maliciously fuzzed file system is to either (a) run fsck before allowing the file system to be mounted, (b) enable the enterprise policy that prohibits USB thumb drives from being automounted, or (c) mount USB stick in some kind of VM (e.g., CrosVM) and then use a reverse virtiofs / 9pfs / fuse to make the file system be available in the host system. The last would be best solution, but it would require development work. So I mention it in the hopes that at some point I can convince some company to pick it up, since it would significantly improve security for all desktops, laptops, and mobile systems that want to support mounting removeable storage. In any case, trying to fix these sorts of syzkaller warnings is essentially playing whack-a-mole, and so while I don't have objections to these sorts of fixes, if it causes any kind of regression or worse, *two* new syzkaller failures, it just makes life harder for overworked ext4 developers. :-) Cheers, - Ted
Hi Ted, On (24/11/12 10:29), Theodore Ts'o wrote: > > I've a following syzkaller report (no reproducer); the report is > > against 5.15, but the same call-chain seems possible in current > > upstream as well. So I suspect that maybe ext4_xattr_inode_create() > > should take nested inode_lock (I_MUTEX_XATTR) instead. Does the > > patch below make any sense? > > These syzkaller reports result from mounting a corrupted (fuzzed) file > system typically when an inode is used in multiple contexts (e.g., as > a directory and an EA inode, etc.) at the same time. I certainly see your point, and I don't argue. > I'd have to take a closer look to see if it makes sense, but in > general, very often whenever we try to fix one of these it ends up > triggering some other syzkaller failure. I see, the one-liner that I posted sort of looks like an addition to d1bc560e9a9c7 which landed in ext4 recently. > And, these sorts of things don't actually result in actual security > problems (at worst, a hang / denial of service attack), and the right > thing to do is to just run fsck on the !@#?!? file system before > mounting the thing. So in our particular case reboot is a bad scenario. Looking at reports from the fleet I see a bunch of hung-task reboots with ext4 frames, e.g. ext4_update_i_disksize()->down_write()->schedule() /* forever */, but I can't claim that this is the deadlock that syzkaller has reported, it very well might not be.
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 7647e9f6e190..db3c68fbbadf 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -1511,7 +1511,7 @@ static struct inode *ext4_xattr_inode_create(handle_t *handle, */ dquot_free_inode(ea_inode); dquot_drop(ea_inode); - inode_lock(ea_inode); + inode_lock_nested(inode, I_MUTEX_XATTR); ea_inode->i_flags |= S_NOQUOTA; inode_unlock(ea_inode); }
Hi, I've a following syzkaller report (no reproducer); the report is against 5.15, but the same call-chain seems possible in current upstream as well. So I suspect that maybe ext4_xattr_inode_create() should take nested inode_lock (I_MUTEX_XATTR) instead. Does the patch below make any sense? ====================================================== WARNING: possible circular locking dependency detected 5.15.168-syzkaller-23766-g3f37c55c6291 #0 Not tainted ------------------------------------------------------ syz-executor297/1452 is trying to acquire lock: ffff888120b5e750 (&ea_inode->i_rwsem#8/1){+.+.}-{3:3}, at: inode_lock ffff888120b5e750 (&ea_inode->i_rwsem#8/1){+.+.}-{3:3}, at: ext4_xattr_inode_create ffff888120b5e750 (&ea_inode->i_rwsem#8/1){+.+.}-{3:3}, at: ext4_xattr_inode_lookup_create ffff888120b5e750 (&ea_inode->i_rwsem#8/1){+.+.}-{3:3}, at: ext4_xattr_set_entry+0x2aeb/0x3200 but task is already holding lock: ffff888120b58c68 (&ei->i_data_sem/3){++++}-{3:3}, at: ext4_setattr+0x12b5/0x1950 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&ei->i_data_sem/3){++++}-{3:3}: down_write+0x38/0x60 ext4_update_i_disksize ext4_xattr_inode_write ext4_xattr_inode_lookup_create ext4_xattr_set_entry+0x2839/0x3200 ext4_xattr_ibody_set+0x113/0x320 ext4_xattr_set_handle+0xa31/0x1440 ext4_xattr_set+0x266/0x3d0 __vfs_setxattr+0x15e/0x1c0 __vfs_setxattr_noperm+0x128/0x5e0 vfs_setxattr+0x1c6/0x410 setxattr+0x1d6/0x270 path_setxattr+0x1cc/0x2b0 __do_sys_lsetxattr __se_sys_lsetxattr __x64_sys_lsetxattr+0xb4/0xd0 do_syscall_x64 do_syscall_64+0x69/0xc0 entry_SYSCALL_64_after_hwframe+0x66/0xd0 -> #0 (&ea_inode->i_rwsem#8/1){+.+.}-{3:3}: check_prev_add check_prevs_add validate_chain __lock_acquire+0x2c95/0x7850 lock_acquire+0x1d2/0x4e0 down_write+0x38/0x60 inode_lock ext4_xattr_inode_create ext4_xattr_inode_lookup_create ext4_xattr_set_entry+0x2aeb/0x3200 ext4_xattr_block_set+0xdc1/0x2de0 ext4_xattr_move_to_block ext4_xattr_make_inode_space ext4_expand_extra_isize_ea+0xe58/0x19c0 __ext4_expand_extra_isize+0x2fd/0x400 ext4_try_to_expand_extra_isize __ext4_mark_inode_dirty+0x58b/0x840 ext4_setattr+0x1341/0x1950 notify_change+0xafb/0xd80 do_truncate+0x218/0x2f0 handle_truncate do_open path_openat+0x27d3/0x2e10 do_filp_open+0x23a/0x360 do_sys_openat2+0x188/0x720 do_sys_open+0x1d1/0x220 do_syscall_x64 do_syscall_64+0x69/0xc0 entry_SYSCALL_64_after_hwframe+0x66/0xd0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&ei->i_data_sem/3); lock(&ea_inode->i_rwsem#8/1); lock(&ei->i_data_sem/3); lock(&ea_inode->i_rwsem#8/1); *** DEADLOCK *** 5 locks held by syz-executor297/1452: #0: ffff88811231c460 (sb_writers#5){.+.+}-{0:0}, at: mnt_want_write+0x3b/0x80 #1: ffff888120b58de0 (&sb->s_type->i_mutex_key#8){++++}-{3:3}, at: inode_lock #1: ffff888120b58de0 (&sb->s_type->i_mutex_key#8){++++}-{3:3}, at: do_truncate+0x204/0x2f0 #2: ffff888120b58f80 (mapping.invalidate_lock){++++}-{3:3}, at: filemap_invalidate_lock #2: ffff888120b58f80 (mapping.invalidate_lock){++++}-{3:3}, at: ext4_setattr+0xd49/0x1950 #3: ffff888120b58c68 (&ei->i_data_sem/3){++++}-{3:3}, at: ext4_setattr+0x12b5/0x1950 #4: ffff888120b58ab8 (&ei->xattr_sem){++++}-{3:3}, at: ext4_write_trylock_xattr #4: ffff888120b58ab8 (&ei->xattr_sem){++++}-{3:3}, at: ext4_try_to_expand_extra_isize #4: ffff888120b58ab8 (&ei->xattr_sem){++++}-{3:3}, at: __ext4_mark_inode_dirty+0x4f7/0x840 stack backtrace: Call Trace: <TASK> __dump_stack dump_stack_lvl+0x1e3/0x2d0 check_noncircular+0x2f3/0x3a0 check_prev_add check_prevs_add validate_chain __lock_acquire+0x2c95/0x7850 lock_acquire+0x1d2/0x4e0 down_write+0x38/0x60 inode_lock ext4_xattr_inode_create ext4_xattr_inode_lookup_create ext4_xattr_set_entry+0x2aeb/0x3200 ext4_xattr_block_set+0xdc1/0x2de0 ext4_xattr_move_to_block ext4_xattr_make_inode_space ext4_expand_extra_isize_ea+0xe58/0x19c0 __ext4_expand_extra_isize+0x2fd/0x400 ext4_try_to_expand_extra_isize __ext4_mark_inode_dirty+0x58b/0x840 ext4_setattr+0x1341/0x1950 notify_change+0xafb/0xd80 do_truncate+0x218/0x2f0 handle_truncate do_open path_openat+0x27d3/0x2e10 do_filp_open+0x23a/0x360 do_sys_openat2+0x188/0x720 do_sys_open+0x1d1/0x220 do_syscall_x64 do_syscall_64+0x69/0xc0 entry_SYSCALL_64_after_hwframe+0x66/0xd0 ---