| Message ID | 20200803062011.17291-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: sysfs: fix NULL pointer dereference at btrfs_sysfs_del_qgroups() | expand |
On 3.08.20 г. 9:20 ч., Qu Wenruo wrote: > [BUG] > With next-20200731 tag (079ad2fb4bf9eba8a0aaab014b49705cd7f07c66), > unmounting a btrfs with quota disabled will cause the following NULL > pointer dereference: > > BTRFS info (device dm-5): has skinny extents > BUG: kernel NULL pointer dereference, address: 0000000000000018 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > CPU: 7 PID: 637 Comm: umount Not tainted 5.8.0-rc7-next-20200731-custom #76 > RIP: 0010:kobject_del+0x6/0x20 > Call Trace: > btrfs_sysfs_del_qgroups+0xac/0xf0 [btrfs] > btrfs_free_qgroup_config+0x63/0x70 [btrfs] > close_ctree+0x1f5/0x323 [btrfs] > btrfs_put_super+0x15/0x17 [btrfs] > generic_shutdown_super+0x72/0x110 > kill_anon_super+0x18/0x30 > btrfs_kill_super+0x17/0x30 [btrfs] > deactivate_locked_super+0x3b/0xa0 > deactivate_super+0x40/0x50 > cleanup_mnt+0x135/0x190 > __cleanup_mnt+0x12/0x20 > task_work_run+0x64/0xb0 > exit_to_user_mode_prepare+0x18a/0x190 > syscall_exit_to_user_mode+0x4f/0x270 > do_syscall_64+0x45/0x50 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > ---[ end trace 37b7adca5c1d5c5d ]--- > > [CAUSE] > Commit 079ad2fb4bf9 ("kobject: Avoid premature parent object freeing in > kobject_cleanup()") changed kobject_del() that it no longer accepts NULL > pointer. > > Before that commit, kobject_del() and kobject_put() all accept NULL > pointers and just ignore such NULL pointers. > > But that mentioned commit needs to access the parent node, killing the > old NULL pointer behavior. > > Unfortunately btrfs is relying on that hidden feature thus we will > trigger such NULL pointer dereference. > > [FIX] > Instead of just saving several lines, do proper fs_info->qgroups_kobj > check before calling kobject_del() and kobject_put(). > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/sysfs.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 104c80caaa74..c8df2edafd85 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1565,9 +1565,11 @@ void btrfs_sysfs_del_qgroups(struct btrfs_fs_info *fs_info) > rbtree_postorder_for_each_entry_safe(qgroup, next, > &fs_info->qgroup_tree, node) > btrfs_sysfs_del_one_qgroup(fs_info, qgroup); > - kobject_del(fs_info->qgroups_kobj); > - kobject_put(fs_info->qgroups_kobj); > - fs_info->qgroups_kobj = NULL; > + if (fs_info->qgroups_kobj) { > + kobject_del(fs_info->qgroups_kobj); > + kobject_put(fs_info->qgroups_kobj); > + fs_info->qgroups_kobj = NULL; > + } > } > > /* Called when qgroups get initialized, thus there is no need for locking */ >
On Mon, Aug 03, 2020 at 02:20:11PM +0800, Qu Wenruo wrote: > [BUG] > With next-20200731 tag (079ad2fb4bf9eba8a0aaab014b49705cd7f07c66), I don't think linux-next commit ids are useful, even the tags get removed after a month so the exact commit causing the crash is what we want. > unmounting a btrfs with quota disabled will cause the following NULL > pointer dereference: > > BTRFS info (device dm-5): has skinny extents > BUG: kernel NULL pointer dereference, address: 0000000000000018 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > CPU: 7 PID: 637 Comm: umount Not tainted 5.8.0-rc7-next-20200731-custom #76 > RIP: 0010:kobject_del+0x6/0x20 > Call Trace: > btrfs_sysfs_del_qgroups+0xac/0xf0 [btrfs] > btrfs_free_qgroup_config+0x63/0x70 [btrfs] > close_ctree+0x1f5/0x323 [btrfs] > btrfs_put_super+0x15/0x17 [btrfs] > generic_shutdown_super+0x72/0x110 > kill_anon_super+0x18/0x30 > btrfs_kill_super+0x17/0x30 [btrfs] > deactivate_locked_super+0x3b/0xa0 > deactivate_super+0x40/0x50 > cleanup_mnt+0x135/0x190 > __cleanup_mnt+0x12/0x20 > task_work_run+0x64/0xb0 > exit_to_user_mode_prepare+0x18a/0x190 > syscall_exit_to_user_mode+0x4f/0x270 > do_syscall_64+0x45/0x50 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > ---[ end trace 37b7adca5c1d5c5d ]--- > > [CAUSE] > Commit 079ad2fb4bf9 ("kobject: Avoid premature parent object freeing in > kobject_cleanup()") changed kobject_del() that it no longer accepts NULL > pointer. That commit reference should be sufficient. > Before that commit, kobject_del() and kobject_put() all accept NULL > pointers and just ignore such NULL pointers. > > But that mentioned commit needs to access the parent node, killing the > old NULL pointer behavior. > > Unfortunately btrfs is relying on that hidden feature thus we will > trigger such NULL pointer dereference. > > [FIX] > Instead of just saving several lines, do proper fs_info->qgroups_kobj > check before calling kobject_del() and kobject_put(). > > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to misc-next, thanks.
On 2020/8/11 上午1:02, David Sterba wrote: > On Mon, Aug 03, 2020 at 02:20:11PM +0800, Qu Wenruo wrote: >> [BUG] >> With next-20200731 tag (079ad2fb4bf9eba8a0aaab014b49705cd7f07c66), > > I don't think linux-next commit ids are useful, even the tags get > removed after a month so the exact commit causing the crash is what we > want. > >> unmounting a btrfs with quota disabled will cause the following NULL >> pointer dereference: >> >> BTRFS info (device dm-5): has skinny extents >> BUG: kernel NULL pointer dereference, address: 0000000000000018 >> #PF: supervisor read access in kernel mode >> #PF: error_code(0x0000) - not-present page >> CPU: 7 PID: 637 Comm: umount Not tainted 5.8.0-rc7-next-20200731-custom #76 >> RIP: 0010:kobject_del+0x6/0x20 >> Call Trace: >> btrfs_sysfs_del_qgroups+0xac/0xf0 [btrfs] >> btrfs_free_qgroup_config+0x63/0x70 [btrfs] >> close_ctree+0x1f5/0x323 [btrfs] >> btrfs_put_super+0x15/0x17 [btrfs] >> generic_shutdown_super+0x72/0x110 >> kill_anon_super+0x18/0x30 >> btrfs_kill_super+0x17/0x30 [btrfs] >> deactivate_locked_super+0x3b/0xa0 >> deactivate_super+0x40/0x50 >> cleanup_mnt+0x135/0x190 >> __cleanup_mnt+0x12/0x20 >> task_work_run+0x64/0xb0 >> exit_to_user_mode_prepare+0x18a/0x190 >> syscall_exit_to_user_mode+0x4f/0x270 >> do_syscall_64+0x45/0x50 >> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> ---[ end trace 37b7adca5c1d5c5d ]--- >> >> [CAUSE] >> Commit 079ad2fb4bf9 ("kobject: Avoid premature parent object freeing in >> kobject_cleanup()") changed kobject_del() that it no longer accepts NULL >> pointer. > > That commit reference should be sufficient. > >> Before that commit, kobject_del() and kobject_put() all accept NULL >> pointers and just ignore such NULL pointers. >> >> But that mentioned commit needs to access the parent node, killing the >> old NULL pointer behavior. >> >> Unfortunately btrfs is relying on that hidden feature thus we will >> trigger such NULL pointer dereference. >> >> [FIX] >> Instead of just saving several lines, do proper fs_info->qgroups_kobj >> check before calling kobject_del() and kobject_put(). >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Added to misc-next, thanks. > Just to add a note, kobject guys are going to restore the NULL-awareness behavior, so the patch here is just to be extra safe. Thanks, Qu
On Tue, Aug 11, 2020 at 10:06:31AM +0800, Qu Wenruo wrote: > > Added to misc-next, thanks. > Just to add a note, kobject guys are going to restore the NULL-awareness > behavior, so the patch here is just to be extra safe. We have the additional pointer checks in other locations too, so adding it here is fine.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 104c80caaa74..c8df2edafd85 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1565,9 +1565,11 @@ void btrfs_sysfs_del_qgroups(struct btrfs_fs_info *fs_info) rbtree_postorder_for_each_entry_safe(qgroup, next, &fs_info->qgroup_tree, node) btrfs_sysfs_del_one_qgroup(fs_info, qgroup); - kobject_del(fs_info->qgroups_kobj); - kobject_put(fs_info->qgroups_kobj); - fs_info->qgroups_kobj = NULL; + if (fs_info->qgroups_kobj) { + kobject_del(fs_info->qgroups_kobj); + kobject_put(fs_info->qgroups_kobj); + fs_info->qgroups_kobj = NULL; + } } /* Called when qgroups get initialized, thus there is no need for locking */
[BUG] With next-20200731 tag (079ad2fb4bf9eba8a0aaab014b49705cd7f07c66), unmounting a btrfs with quota disabled will cause the following NULL pointer dereference: BTRFS info (device dm-5): has skinny extents BUG: kernel NULL pointer dereference, address: 0000000000000018 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page CPU: 7 PID: 637 Comm: umount Not tainted 5.8.0-rc7-next-20200731-custom #76 RIP: 0010:kobject_del+0x6/0x20 Call Trace: btrfs_sysfs_del_qgroups+0xac/0xf0 [btrfs] btrfs_free_qgroup_config+0x63/0x70 [btrfs] close_ctree+0x1f5/0x323 [btrfs] btrfs_put_super+0x15/0x17 [btrfs] generic_shutdown_super+0x72/0x110 kill_anon_super+0x18/0x30 btrfs_kill_super+0x17/0x30 [btrfs] deactivate_locked_super+0x3b/0xa0 deactivate_super+0x40/0x50 cleanup_mnt+0x135/0x190 __cleanup_mnt+0x12/0x20 task_work_run+0x64/0xb0 exit_to_user_mode_prepare+0x18a/0x190 syscall_exit_to_user_mode+0x4f/0x270 do_syscall_64+0x45/0x50 entry_SYSCALL_64_after_hwframe+0x44/0xa9 ---[ end trace 37b7adca5c1d5c5d ]--- [CAUSE] Commit 079ad2fb4bf9 ("kobject: Avoid premature parent object freeing in kobject_cleanup()") changed kobject_del() that it no longer accepts NULL pointer. Before that commit, kobject_del() and kobject_put() all accept NULL pointers and just ignore such NULL pointers. But that mentioned commit needs to access the parent node, killing the old NULL pointer behavior. Unfortunately btrfs is relying on that hidden feature thus we will trigger such NULL pointer dereference. [FIX] Instead of just saving several lines, do proper fs_info->qgroups_kobj check before calling kobject_del() and kobject_put(). Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/sysfs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)