Message-ID: <fb336d1c-ce68-403c-9a7a-556906c05112@gmx.com>
Date: Fri, 26 Dec 2025 18:33:34 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Zilin Guan <zilin@....edu.cn>
Cc: clm@...com, dan.carpenter@...aro.org, dsterba@...e.com,
 jianhao.xu@....edu.cn, linux-btrfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, sunk67188@...il.com
Subject: Re: [PATCH] btrfs: tests: Fix memory leak in btrfs_test_qgroups()



在 2025/12/26 17:59, Zilin Guan 写道:
> On Fri, Dec 26, 2025 at 07:24:45AM +1030, Qu Wenruo wrote:
>>>> @@ -517,11 +517,11 @@ int btrfs_test_qgroups(u32 sectorsize, u32
>>>> nodesize)
>>>>        tmp_root->root_key.objectid = BTRFS_FS_TREE_OBJECTID;
>>>>        root->fs_info->fs_root = tmp_root;
>>>>        ret = btrfs_insert_fs_root(root->fs_info, tmp_root);
>>>> +    btrfs_put_root(tmp_root);
>>>>        if (ret) {
>>>>            test_err("couldn't insert fs root %d", ret);
>>>>            goto out;
>>>
>>> This will lead to double free.
>>>
>>> If btrfs_insert_fs_root() failed, btrfs_put_root() will do the cleaning
>>> and free the root.
>>>
>>> Then btrfs_free_dummy_root() will call btrfs_put_root() again on the
>>> root, cause use-after-free.
>>>
>>> So your analyze is completely wrong.
> 
> Hi Qu,
> 
> Thanks for the review. I apologize for any ambiguity in my previous
> communication.
> 
> I believe there might be a misunderstanding regarding which root is
> being freed at the 'out' label.
> 
> The 'out' label calls:
>      btrfs_free_dummy_root(root);
>      btrfs_free_dummy_fs_info(fs_info);
> 
> It explicitly frees 'root', but it does not free 'tmp_root'.

OK, my bad, indeed it's a different root.

In that case it's correct to move the free_root immediately after 
btrfs_insert_fs_root().


And since you're here, you may also want to modify the error message of 
the subvolume tree, it's copy-pasted from the fs root one, which is not 
correct anymore (it's not fs tree but the first subvolume tree).

> 
> If btrfs_insert_fs_root() fails for 'tmp_root', the 'tmp_root' is not
> added to 'fs_info', so btrfs_free_dummy_fs_info() won't free it either.
> 
> Therefore, if we jump to 'out' on failure without calling
> btrfs_put_root(tmp_root), the reference count of 'tmp_root'
> remains 1 (from allocation), and it is never freed, causing a leak.
> 
> My patch moves btrfs_put_root(tmp_root) before the error check.
> - On success: ref is 2 (alloc + insert), put() drops it to 1
> (held by fs_info).
> - On failure: ref is 1 (alloc), put() drops it to 0, freeing it
> immediately.
> 
> Please let me know if I missed anything or if there are any further
> improvements I can make to the patch.
> 
>> And considering you're sending quite some patches and most of them are
>> rejected, next time if you believe you find some
>> memory-leak/use-after-free, please hit them in the real world with
>> kmemleak/kasan enabled with the call trace.
>>
>> I believe kmemleak/kasan more than you, and that will save us a lot of time.
> 
> Regarding your concern about my previous patches: I admit I may have
> submitted a few incorrect patches early on. In those cases, although
> the proposed fixes were either inappropriate or deemed unnecessary by
> the community, the issues identified were real.
> 
> We have reviewed our previous submissions and found that most of them
> have been accepted. Despite this, we will enforce stricter verification
> processes in the future. Currently, our team enforces a strict
> verification process: we manually confirm there is a clear path missing
> a free before reporting a leak, and every patch undergoes a double-check
> by at least two members.

If that's the case, please add them with proper tags.

> 
> I am dedicated to improving the kernel by detecting potential
> vulnerabilities through static analysis. I hope my efforts contribute
> positively to the community.
> 
> Thanks,
> Zilin Guan
> 
>>>
>>> Thanks,
>>> Qu
>>>
>>>>        }
>>>> -    btrfs_put_root(tmp_root);
>>>>        tmp_root = btrfs_alloc_dummy_root(fs_info);
>>>>        if (IS_ERR(tmp_root)) {
>>>> @@ -532,11 +532,11 @@ int btrfs_test_qgroups(u32 sectorsize, u32
>>>> nodesize)
>>>>        tmp_root->root_key.objectid = BTRFS_FIRST_FREE_OBJECTID;
>>>>        ret = btrfs_insert_fs_root(root->fs_info, tmp_root);
>>>> +    btrfs_put_root(tmp_root);
>>>>        if (ret) {
>>>>            test_err("couldn't insert fs root %d", ret);
>>>>            goto out;
>>>>        }
>>>> -    btrfs_put_root(tmp_root);
>>>>        test_msg("running qgroup tests");
>>>>        ret = test_no_shared_qgroup(root, sectorsize, nodesize);
>>>
>>>
> 

Powered by blists - more mailing lists