btrfs_alloc_dummy_root() allocates a root with a reference count of 1.
Then btrfs_insert_fs_root() is used to insert the root into the fs_info.
On success, it increments the reference count. On failure, it does not.

Currently, if btrfs_insert_fs_root() fails, the error handling path
jumps to the out label immediately without decrementing the reference
count of tmp_root, leading to a memory leak.

Fix this by calling btrfs_put_root() unconditionally after
btrfs_insert_fs_root(). This correctly handles both cases: on success,
it drops the local reference, leaving the root with the reference held
by fs_info; on failure, it drops the sole reference, freeing the root.

Fixes: 4785e24fa5d23 ("btrfs: don't take an extra root ref at allocation time")
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 fs/btrfs/tests/qgroup-tests.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
index e9124605974b..0d51e0abaeac 100644
--- a/fs/btrfs/tests/qgroup-tests.c
+++ b/fs/btrfs/tests/qgroup-tests.c
@@ -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;
 	}
-	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);
-- 
2.34.1

在 2025/12/25 20:57, Zilin Guan 写道:
> btrfs_alloc_dummy_root() allocates a root with a reference count of 1.
> Then btrfs_insert_fs_root() is used to insert the root into the fs_info.
> On success, it increments the reference count. On failure, it does not.
> 
> Currently, if btrfs_insert_fs_root() fails, the error handling path
> jumps to the out label immediately without decrementing the reference
> count of tmp_root, leading to a memory leak.
> 
> Fix this by calling btrfs_put_root() unconditionally after
> btrfs_insert_fs_root(). This correctly handles both cases: on success,
> it drops the local reference, leaving the root with the reference held
> by fs_info; on failure, it drops the sole reference, freeing the root.
> 
> Fixes: 4785e24fa5d23 ("btrfs: don't take an extra root ref at allocation time")
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>   fs/btrfs/tests/qgroup-tests.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
> index e9124605974b..0d51e0abaeac 100644
> --- a/fs/btrfs/tests/qgroup-tests.c
> +++ b/fs/btrfs/tests/qgroup-tests.c
> @@ -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.

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);

在 2025/12/26 07:19, Qu Wenruo 写道:
> 
> 
> 在 2025/12/25 20:57, Zilin Guan 写道:
>> btrfs_alloc_dummy_root() allocates a root with a reference count of 1.
>> Then btrfs_insert_fs_root() is used to insert the root into the fs_info.
>> On success, it increments the reference count. On failure, it does not.
>>
>> Currently, if btrfs_insert_fs_root() fails, the error handling path
>> jumps to the out label immediately without decrementing the reference
>> count of tmp_root, leading to a memory leak.
>>
>> Fix this by calling btrfs_put_root() unconditionally after
>> btrfs_insert_fs_root(). This correctly handles both cases: on success,
>> it drops the local reference, leaving the root with the reference held
>> by fs_info; on failure, it drops the sole reference, freeing the root.
>>
>> Fixes: 4785e24fa5d23 ("btrfs: don't take an extra root ref at 
>> allocation time")
>> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
>> ---
>>   fs/btrfs/tests/qgroup-tests.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup- 
>> tests.c
>> index e9124605974b..0d51e0abaeac 100644
>> --- a/fs/btrfs/tests/qgroup-tests.c
>> +++ b/fs/btrfs/tests/qgroup-tests.c
>> @@ -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.

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.

> 
> 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);
> 
> 

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'.

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.

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);
> > 
> >

在 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);
>>>
>>>
> 

Hi Qu,

On Fri, Dec 26, 2025 at 06:33:34PM+1030, Qu Wenruo wrote:
> 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).

Thanks for the confirmation. I will update the error message accordingly in v2.

> > 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.

Sure, I will include the related tags in v2.

Thanks,
Zilin Guan