linux-kernel - Re: [PATCH] btrfs: tests: Fix memory leak in btrfs_test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20251226072956.1734175-1-zilin@seu.edu.cn>
Date: Fri, 26 Dec 2025 07:29:56 +0000
From: Zilin Guan <zilin@....edu.cn>
To: quwenruo.btrfs@....com
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,
	zilin@....edu.cn
Subject: Re: [PATCH] btrfs: tests: Fix memory leak in btrfs_test_qgroups()

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