| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <063b1e52-0769-403b-ae05-7b999223a1f2@gmx.com>
Date: Wed, 25 Jun 2025 20:19:06 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Hillf Danton <hdanton@...a.com>
Cc: Edward Adam Davis <eadavis@...com>,
syzbot+fa90fcaa28f5cd4b1fc1@...kaller.appspotmail.com, clm@...com,
josef@...icpanda.com, linux-btrfs@...r.kernel.org,
linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH next] btrfs: fix deadlock in btrfs_read_chunk_tree
在 2025/6/25 09:26, Hillf Danton 写道:
> On Wed, 25 Jun 2025 06:00:09 +0930 Qu Wenruo wrote:
>> =E5=9C=A8 2025/6/25 00:00, Edward Adam Davis =E5=86=99=E9=81=93:
>>> Remove the lock uuid_mutex outside of sget_fc() to avoid the deadlock
>>> reported by [1].
>>> =20
>>> [1]
>>> -> #1 (&type->s_umount_key#41/1){+.+.}-{4:4}:
>>> lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5871
>>> down_write_nested+0x9d/0x200 kernel/locking/rwsem.c:1693
>>> alloc_super+0x204/0x970 fs/super.c:345
>
> Given kzalloc [3], the syzbot report is false positive (a known lockdep
> issue) as nobody else should acquire s->s_umount lock.
>
> [3] https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/super.c?id=7aacdf6feed1#n319
Not a false alert either.
sget_fc() can return an existing super block, we can race between a
mount and an unmount on the same super block.
In that case it's going to cause problem.
This is already fixed in the v4 (and later v5) patchset:
https://lore.kernel.org/linux-btrfs/cover.1750724841.git.wqu@suse.com/
Thanks,
Qu
>
>>> sget_fc+0x329/0xa40 fs/super.c:761
>>> btrfs_get_tree_super fs/btrfs/super.c:1867 [inline]
>>> btrfs_get_tree_subvol fs/btrfs/super.c:2059 [inline]
>>> btrfs_get_tree+0x4c6/0x12d0 fs/btrfs/super.c:2093
>>> vfs_get_tree+0x8f/0x2b0 fs/super.c:1804
>>> do_new_mount+0x24a/0xa40 fs/namespace.c:3902
>>> do_mount fs/namespace.c:4239 [inline]
>>> __do_sys_mount fs/namespace.c:4450 [inline]
>>> __se_sys_mount+0x317/0x410 fs/namespace.c:4427
>>> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>>> do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
>>> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>> =20
>>> -> #0 (uuid_mutex){+.+.}-{4:4}:
>>> check_prev_add kernel/locking/lockdep.c:3168 [inline]
>>> check_prevs_add kernel/locking/lockdep.c:3287 [inline]
>>> validate_chain+0xb9b/0x2140 kernel/locking/lockdep.c:3911
>>> __lock_acquire+0xab9/0xd20 kernel/locking/lockdep.c:5240
>>> lock_acquire+0x120/0x360 kernel/locking/lockdep.c:5871
>>> __mutex_lock_common kernel/locking/mutex.c:602 [inline]
>>> __mutex_lock+0x182/0xe80 kernel/locking/mutex.c:747
>>> btrfs_read_chunk_tree+0xef/0x2170 fs/btrfs/volumes.c:7462
>>> open_ctree+0x17f2/0x3a10 fs/btrfs/disk-io.c:3458
>>> btrfs_fill_super fs/btrfs/super.c:984 [inline]
>>> btrfs_get_tree_super fs/btrfs/super.c:1922 [inline]
>>> btrfs_get_tree_subvol fs/btrfs/super.c:2059 [inline]
>>> btrfs_get_tree+0xc6f/0x12d0 fs/btrfs/super.c:2093
>>> vfs_get_tree+0x8f/0x2b0 fs/super.c:1804
>>> do_new_mount+0x24a/0xa40 fs/namespace.c:3902
>>> do_mount fs/namespace.c:4239 [inline]
>>> __do_sys_mount fs/namespace.c:4450 [inline]
>>> __se_sys_mount+0x317/0x410 fs/namespace.c:4427
>>> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>>> do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
>>> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>> =20
>>> other info that might help us debug this:
>>> =20
>>> Possible unsafe locking scenario:
>>> =20
>>> CPU0 CPU1
>>> ---- ----
>>> lock(&type->s_umount_key#41/1);
>>> lock(uuid_mutex);
>>> lock(&type->s_umount_key#41/1);
>>> lock(uuid_mutex);
>>> =20
>>> *** DEADLOCK ***
>>> =20
>>> Fixes: 7aacdf6feed1 ("btrfs: delay btrfs_open_devices() until super bloc=
>> k is created")
>>> Reported-by: syzbot+fa90fcaa28f5cd4b1fc1@...kaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=3Dfa90fcaa28f5cd4b1fc1
>>> Tested-by: syzbot+fa90fcaa28f5cd4b1fc1@...kaller.appspotmail.com
>>> Signed-off-by: Edward Adam Davis <eadavis@...com>
>>> ---
>>> fs/btrfs/super.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>> =20
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 237e60b53192..c2ce1eb53ad7 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -1864,11 +1864,10 @@ static int btrfs_get_tree_super(struct fs_contex=
>> t *fc)
>>> fs_devices =3D device->fs_devices;
>>> fs_info->fs_devices =3D fs_devices;
>>> =20
>>> + mutex_unlock(&uuid_mutex);
>>
>> No, you can not unlock uuid_mutex without opening the devices.
>>
>> Just run the test case generic/604.
>>
>>> sb =3D sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
>>> - if (IS_ERR(sb)) {
>>> - mutex_unlock(&uuid_mutex);
>>> + if (IS_ERR(sb))
>>> return PTR_ERR(sb);
>>> - }
>>> =20
>>> set_device_specific_options(fs_info);
>>> =20
>>> @@ -1887,6 +1886,7 @@ static int btrfs_get_tree_super(struct fs_context =
>> *fc)
>>> * But the fs_info->fs_devices is not opened, we should not let
>>> * btrfs_free_fs_context() to close them.
>>> */
>>> + mutex_lock(&uuid_mutex);
>>> fs_info->fs_devices =3D NULL;
>>> mutex_unlock(&uuid_mutex);
>>> =20
>>> @@ -1906,6 +1906,7 @@ static int btrfs_get_tree_super(struct fs_context =
>> *fc)
>>> */
>>> ASSERT(fc->s_fs_info =3D=3D NULL);
>>> =20
>>> + mutex_lock(&uuid_mutex);
>>> ret =3D btrfs_open_devices(fs_devices, mode, sb);
>>> mutex_unlock(&uuid_mutex);
>>> if (ret < 0) {
>
Powered by blists - more mailing lists