Re: [PATCH] btrfs: tests: Fix memory leak in btrfs_test_qgroups()

[Qu Wenruo]

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