diff mbox series

btrfs: Fix missing close devices

Message ID 20200921082637.26009-1-qiang.zhang@windriver.com
State New, archived
Headers show
Series btrfs: Fix missing close devices | expand

Commit Message

Zqiang Sept. 21, 2020, 8:26 a.m. UTC
From: Zqiang <qiang.zhang@windriver.com>

When the btrfs fill super error, we should first close devices and
then call deactivate_locked_super func to free fs_info.

Signed-off-by: Zqiang <qiang.zhang@windriver.com>
---
 fs/btrfs/super.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Johannes Thumshirn Sept. 21, 2020, 8:52 a.m. UTC | #1
On 21/09/2020 10:27, qiang.zhang@windriver.com wrote:
> From: Zqiang <qiang.zhang@windriver.com>
> 
> When the btrfs fill super error, we should first close devices and
> then call deactivate_locked_super func to free fs_info.
> 
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> ---
>  fs/btrfs/super.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 8840a4fa81eb..3bfd54e8f388 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1675,6 +1675,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>  		error = security_sb_set_mnt_opts(s, new_sec_opts, 0, NULL);
>  	security_free_mnt_opts(&new_sec_opts);
>  	if (error) {
> +		btrfs_close_devices(fs_devices);
>  		deactivate_locked_super(s);
>  		return ERR_PTR(error);
>  	}
> 

I think this is the fix for the syzkaller issue: 
Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
Johannes Thumshirn Sept. 21, 2020, 9:17 a.m. UTC | #2
On 21/09/2020 11:14, Zhang, Qiang wrote:
> 
> 
> ________________________________________
> 发件人: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
> 发送时间: 2020年9月21日 16:52
> 收件人: Zhang, Qiang; clm@fb.com; josef@toxicpanda.com; dsterba@suse.com
> 抄送: linux-btrfs@vger.kernel.org; linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] btrfs: Fix missing close devices
> 
> On 21/09/2020 10:27, qiang.zhang@windriver.com wrote:
>> From: Zqiang <qiang.zhang@windriver.com>
>>
>> When the btrfs fill super error, we should first close devices and
>> then call deactivate_locked_super func to free fs_info.
>>
>> Signed-off-by: Zqiang <qiang.zhang@windriver.com>
>> ---
>>  fs/btrfs/super.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 8840a4fa81eb..3bfd54e8f388 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -1675,6 +1675,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>>               error = security_sb_set_mnt_opts(s, new_sec_opts, 0, NULL);
>>       security_free_mnt_opts(&new_sec_opts);
>>       if (error) {
>> +             btrfs_close_devices(fs_devices);
>>               deactivate_locked_super(s);
>>               return ERR_PTR(error);
>>       }
>>
> 
>> I think this is the fix for the syzkaller issue:
>> Reported-by: syzbot+582e66e5edf36a22c7b0@syzkaller.appspotmail.com
> 
> Please  try this patch.
> 

Nope, with this patch I get the following Null-ptr-deref:
[   39.065209] ==================================================================    
[   39.066318] BUG: KASAN: null-ptr-deref in bdev_name.constprop.0+0xd4/0x240   
[   39.067307] Read of size 4 at addr 00000000000003ac by task syz-repro/273    
[   39.068289]                                                                  
[   39.069602] ==================================================================
[   39.070837] BUG: kernel NULL pointer dereference, address: 00000000000003ac  
[   39.071837] #PF: supervisor read access in kernel mode                                                                                                                             
[   39.072580] #PF: error_code(0x0000) - not-present page
[   39.073318] PGD 80000001cd3b1067 P4D 80000001cd3b1067 PUD 1c6de7067 PMD 0 
[   39.074306] Oops: 0000 [#1] SMP KASAN PTI             
[   39.074887] CPU: 0 PID: 273 Comm: syz-repro Tainted: G    B             5.9.0-rc5+ #772                                                                                                                                                                                                                                                                                  
[   39.076031] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
[   39.077638] RIP: 0010:bdev_name.constprop.0+0xd4/0x240                       
[   39.078387] Code: ca 4c 89 4c 24 08 e8 0b e9 ff ff 48 89 df 49 89 c6 e8 40 42 c6 ff 49 8b ac 24 e0 00 00 00 48 8d bd ac 03 00 00 e8 2c 41 c6 ff <8b> 85 ac 03 00 00 4c 8b 4c 24 08 85 c0 0f 84 fe 00 00 00 4c 89 cf
[   39.080991] RSP: 0018:ffff8881f1a97878 EFLAGS: 00010286                      
[   39.081728] RAX: 0000000000000001 RBX: ffff8881c9fb80e0 RCX: dffffc0000000000
[   39.082725] RDX: 0000000000000007 RSI: 0000000000000004 RDI: ffffffff81acd784
[   39.083717] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000           
[   39.084722] R10: fffffbfff0539591 R11: 0000000000000001 R12: ffff8881c9fb8000
[   39.085711] R13: ffff8881ef6e2698 R14: ffff8881ef6e2680 R15: 0000000000000000
[   39.086704] FS:  00007f5d36eb9540(0000) GS:ffff8881f7600000(0000) knlGS:0000000000000000
[   39.087827] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                
[   39.088623] CR2: 00000000000003ac CR3: 00000001ef552000 CR4: 00000000000006b0
[   39.089607] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   39.090603] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   39.091583] Call Trace:
[   39.091943]  ? mac_address_string+0x380/0x380
[   39.092559]  ? mark_held_locks+0x65/0x90
[   39.093116]  pointer+0x21c/0x650
[   39.093578]  ? format_decode+0x1cf/0x4e0
[   39.094139]  ? resource_string.isra.0+0xc10/0xc10
[   39.094809]  vsnprintf+0x2e0/0x820
[   39.095292]  ? pointer+0x650/0x650
[   39.095785]  snprintf+0x88/0xa0
[   39.096234]  ? vsprintf+0x10/0x10
[   39.096708]  ? rcu_read_lock_sched_held+0x3a/0x70
[   39.097378]  ? sget+0x200/0x240
[   39.097908]  ? btrfs_kill_super+0x30/0x30 [btrfs]
[   39.098644]  btrfs_mount_root+0x442/0x5d0 [btrfs]
[   39.099377]  ? parse_rescue_options+0x150/0x150 [btrfs]
[   39.100103]  ? rcu_read_lock_sched_held+0x3a/0x70
[   39.100759]  ? vfs_parse_fs_string+0xbc/0xf0
[   39.101355]  ? kfree+0x1e0/0x310
[   39.101816]  ? vfs_parse_fs_string+0xbc/0xf0
[   39.102493]  ? parse_rescue_options+0x150/0x150 [btrfs]
[   39.103219]  legacy_get_tree+0x7d/0xc0
[   39.103750]  vfs_get_tree+0x48/0x100
[   39.104263]  ? parse_monolithic_mount_data+0x1c/0x40
[   39.104960]  vfs_kern_mount.part.0+0x70/0xd0
[   39.105649]  btrfs_mount+0x187/0x550 [btrfs]
[   39.106268]  ? mark_held_locks+0x24/0x90
[   39.106918]  ? btrfs_show_options+0x730/0x730 [btrfs]
[   39.107641]  ? lockdep_hardirqs_on_prepare+0x146/0x240
[   39.108368]  ? strcmp+0x2e/0x50
[   39.108810]  ? rcu_read_lock_sched_held+0x3a/0x70
[   39.109461]  ? vfs_parse_fs_string+0xbc/0xf0
[   39.110054]  ? kfree+0x1e0/0x310
[   39.110519]  ? vfs_parse_fs_string+0xbc/0xf0
[   39.111197]  ? btrfs_show_options+0x730/0x730 [btrfs]
[   39.111898]  legacy_get_tree+0x7d/0xc0
[   39.112427]  vfs_get_tree+0x48/0x100
[   39.112928]  path_mount+0xa37/0xfc0
[   39.113422]  ? strncpy_from_user+0xde/0x1f0
[   39.114011]  ? copy_mount_string+0x20/0x20
[   39.114588]  ? getname_flags+0xa7/0x220
[   39.115132]  ? _copy_from_user+0x8e/0xd0
[   39.115686]  __x64_sys_mount+0x16b/0x1a0
[   39.116242]  ? copy_mnt_ns+0x540/0x540
[   39.116770]  ? trace_hardirqs_on+0x34/0x130
[   39.117370]  do_syscall_64+0x33/0x40
[   39.117886]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   39.118604] RIP: 0033:0x7f5d36df1eda
Johannes Thumshirn Sept. 23, 2020, 8:17 a.m. UTC | #3
On 23/09/2020 08:03, Zhang, Qiang wrote:
> Hello Johannes Thumshirn
> 
> the crash happend in "snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev)" in btrfs_mount_root func,  the  "bdev" may be destroyed in btrfs_close_devices.
> I think add  btrfs_close_devices func before deactivate_locked_super is reasonable.
>  I'm not sure if that's another problem .
>  What's your point of view ?
> 

I think this is generally unneeded.
Think of this call chain:
deactivate_locked_super()
`-> fs->kill_sb()
    `-> btrfs_kill_super()
        `-> kill_anon_super()
            `-> generic_shutdown_super()
                `-> sop->put_super()
                    `-> btrfs_put_super()
                        `-> close_ctree()
                            `-> btrfs_close_devices()
diff mbox series

Patch

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8840a4fa81eb..3bfd54e8f388 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1675,6 +1675,7 @@  static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		error = security_sb_set_mnt_opts(s, new_sec_opts, 0, NULL);
 	security_free_mnt_opts(&new_sec_opts);
 	if (error) {
+		btrfs_close_devices(fs_devices);
 		deactivate_locked_super(s);
 		return ERR_PTR(error);
 	}