Re: [PATCH] btrfs: qgroup: fix sleep from invalid context bug in update_qgroup_limit_item()

[ChenXiaoSong <chenxiaosong2@huawei.com>]

Yes, at least two places will sleep in btrfs_search_slot():

   update_qgroup_limit_item
     btrfs_search_slot
       setup_nodes_for_search
         reada_for_balance
           btrfs_readahead_node_child
             btrfs_readahead_tree_block
               btrfs_find_create_tree_block
                 alloc_extent_buffer
                   kmem_cache_zalloc
                     /* will sleep */
                     kmem_cache_alloc(GFP_NOFS|__GFP_NOFAIL|__GFP_ZERO)
               read_extent_buffer_pages
                 submit_extent_page
                   submit_one_bio /* disk IO, will sleep */

在 2022/11/11 6:58, Qu Wenruo 写道:
> 
> 
> On 2022/11/10 22:13, ChenXiaoSong wrote:
>> Syzkaller reported BUG as follows:
>>
>>    BUG: sleeping function called from invalid context at
>>         include/linux/sched/mm.h:274
>>    Call Trace:
>>     <TASK>
>>     dump_stack_lvl+0xcd/0x134
>>     __might_resched.cold+0x222/0x26b
>>     kmem_cache_alloc+0x2e7/0x3c0
>>     update_qgroup_limit_item+0xe1/0x390
>>     btrfs_qgroup_inherit+0x147b/0x1ee0
>>     create_subvol+0x4eb/0x1710
>>     btrfs_mksubvol+0xfe5/0x13f0
>>     __btrfs_ioctl_snap_create+0x2b0/0x430
>>     btrfs_ioctl_snap_create_v2+0x25a/0x520
>>     btrfs_ioctl+0x2a1c/0x5ce0
>>     __x64_sys_ioctl+0x193/0x200
>>     do_syscall_64+0x35/0x80
>>
>> Fix this by introducing __update_qgroup_limit_item() helper, allocate
>> memory outside of the spin lock.
>>
>> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> 
> Unfortunately, __update_qgroup_limit_item() can still sleep.
> 
> As it calls btrfs_search_slot(), which can lead to disk IO if the qgroup 
> tree is not cached.
> 
> 
> I believe the proper way is to either unlock the spinlock inside 
> btrfs_qgroup_inherit() (which needs extra scrutiny on the qgroup lock), 
> or delayed the limit item updates until we have unlocked the spinlock.
> 
> To me, the latter one seems more reasonable, as it's just one qgroup 
> (@dstgroup), and we're doing the same delayed work for sysfs interface 
> creation.
> 
> Thanks,
> Qu
> 
>> ---
>>   fs/btrfs/qgroup.c | 35 ++++++++++++++++++++++++++---------
>>   1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 9334c3157c22..99a61cc04b68 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -768,11 +768,11 @@ static int del_qgroup_item(struct 
>> btrfs_trans_handle *trans, u64 qgroupid)
>>       return ret;
>>   }
>> -static int update_qgroup_limit_item(struct btrfs_trans_handle *trans,
>> -                    struct btrfs_qgroup *qgroup)
>> +static int __update_qgroup_limit_item(struct btrfs_trans_handle *trans,
>> +                      struct btrfs_qgroup *qgroup,
>> +                      struct btrfs_path *path) >   {
>>       struct btrfs_root *quota_root = trans->fs_info->quota_root;
>> -    struct btrfs_path *path;
>>       struct btrfs_key key;
>>       struct extent_buffer *l;
>>       struct btrfs_qgroup_limit_item *qgroup_limit;
>> @@ -783,10 +783,6 @@ static int update_qgroup_limit_item(struct 
>> btrfs_trans_handle *trans,
>>       key.type = BTRFS_QGROUP_LIMIT_KEY;
>>       key.offset = qgroup->qgroupid;
>> -    path = btrfs_alloc_path();
>> -    if (!path)
>> -        return -ENOMEM;
>> -
>>       ret = btrfs_search_slot(trans, quota_root, &key, path, 0, 1);
>>       if (ret > 0)
>>           ret = -ENOENT;
>> @@ -806,6 +802,21 @@ static int update_qgroup_limit_item(struct 
>> btrfs_trans_handle *trans,
>>       btrfs_mark_buffer_dirty(l);
>>   out:
>> +    return ret;
>> +}
>> +
>> +static int update_qgroup_limit_item(struct btrfs_trans_handle *trans,
>> +                    struct btrfs_qgroup *qgroup)
>> +{
>> +    struct btrfs_path *path;
>> +    int ret;
>> +
>> +    path = btrfs_alloc_path();
>> +    if (!path)
>> +        return -ENOMEM;
>> +
>> +    ret = __update_qgroup_limit_item(trans, qgroup, path);
>> +
>>       btrfs_free_path(path);
>>       return ret;
>>   }
>> @@ -2860,6 +2871,7 @@ int btrfs_qgroup_inherit(struct 
>> btrfs_trans_handle *trans, u64 srcid,
>>       bool need_rescan = false;
>>       u32 level_size = 0;
>>       u64 nums;
>> +    struct btrfs_path *path;
>>       /*
>>        * There are only two callers of this function.
>> @@ -2935,6 +2947,11 @@ int btrfs_qgroup_inherit(struct 
>> btrfs_trans_handle *trans, u64 srcid,
>>           ret = 0;
>>       }
>> +    path = btrfs_alloc_path();
>> +    if (!path) {
>> +        ret = -ENOMEM;
>> +        goto out;
>> +    }
>>       spin_lock(&fs_info->qgroup_lock);
>> @@ -2950,8 +2967,7 @@ int btrfs_qgroup_inherit(struct 
>> btrfs_trans_handle *trans, u64 srcid,
>>           dstgroup->max_excl = inherit->lim.max_excl;
>>           dstgroup->rsv_rfer = inherit->lim.rsv_rfer;
>>           dstgroup->rsv_excl = inherit->lim.rsv_excl;
>> -
>> -        ret = update_qgroup_limit_item(trans, dstgroup);
>> +        ret = __update_qgroup_limit_item(trans, dstgroup, path);
>>           if (ret) {
>>               qgroup_mark_inconsistent(fs_info);
>>               btrfs_info(fs_info,
>> @@ -3053,6 +3069,7 @@ int btrfs_qgroup_inherit(struct 
>> btrfs_trans_handle *trans, u64 srcid,
>>   unlock:
>>       spin_unlock(&fs_info->qgroup_lock);
>> +    btrfs_free_path(path);
>>       if (!ret)
>>           ret = btrfs_sysfs_add_one_qgroup(fs_info, dstgroup);
>>   out:
> 
> .