From: Filipe Manana <fdmanana@kernel.org>
To: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: linux-btrfs@vger.kernel.org, clm@fb.com, josef@toxicpanda.com,
dsterba@suse.com
Subject: Re: [PATCH 3/3] btrfs: Fix ulist memory leak in test_multiple_refs()
Date: Tue, 1 Nov 2022 08:15:50 +0000 [thread overview]
Message-ID: <20221101081550.GA3418818@falcondesktop> (raw)
In-Reply-To: <20221101025356.1643836-4-zhangxiaoxu5@huawei.com>
On Tue, Nov 01, 2022 at 10:53:56AM +0800, Zhang Xiaoxu wrote:
> There are some meory leak report when do sanity tests:
>
> unreferenced object 0xffff888296ab40c0 (size 32):
> comm "insmod", pid 76176, jiffies 4307414230 (age 309.541s)
> hex dump (first 32 bytes):
> 01 00 00 00 00 00 00 00 90 8a b1 30 81 88 ff ff ...........0....
> 90 8a b1 30 81 88 ff ff a0 8a b1 30 81 88 ff ff ...0.......0....
> backtrace:
> [<00000000e59989d0>] kmalloc_trace+0x27/0xa0
> [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs]
> [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs]
> [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs]
> [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs]
> [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs]
> [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs]
> [<000000002925cdf3>] do_one_initcall+0x87/0x2a0
> [<00000000c5ed267e>] do_init_module+0xdf/0x320
> [<000000005f972694>] load_module+0x3006/0x3390
> [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0
> [<00000000e7029c2b>] do_syscall_64+0x35/0x80
> [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
> unreferenced object 0xffff888130b18a80 (size 64):
> comm "insmod", pid 76176, jiffies 4307414230 (age 309.542s)
> hex dump (first 32 bytes):
> 05 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> c8 40 ab 96 82 88 ff ff c8 40 ab 96 82 88 ff ff .@.......@......
> backtrace:
> [<00000000e59989d0>] kmalloc_trace+0x27/0xa0
> [<00000000de27bc34>] ulist_add_merge.part.0+0x67/0x1f0 [btrfs]
> [<00000000fbdf3c21>] find_parent_nodes+0xd49/0x2880 [btrfs]
> [<00000000ba4d2155>] btrfs_find_all_roots_safe+0x120/0x1c0 [btrfs]
> [<00000000c7fe3cc7>] test_no_shared_qgroup.constprop.0+0x1f2/0x350 [btrfs]
> [<00000000f6a6b761>] btrfs_test_qgroups+0x2c8/0x300 [btrfs]
> [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs]
> [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs]
> [<000000002925cdf3>] do_one_initcall+0x87/0x2a0
> [<00000000c5ed267e>] do_init_module+0xdf/0x320
> [<000000005f972694>] load_module+0x3006/0x3390
> [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0
> [<00000000e7029c2b>] do_syscall_64+0x35/0x80
> [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
> unreferenced object 0xffff88812ae83100 (size 32):
> comm "insmod", pid 77664, jiffies 4307625383 (age 98.396s)
> hex dump (first 32 bytes):
> 01 00 00 00 00 00 00 00 10 f7 7e 23 81 88 ff ff ..........~#....
> 10 f7 7e 23 81 88 ff ff 20 f7 7e 23 81 88 ff ff ..~#.... .~#....
> backtrace:
> [<00000000e59989d0>] kmalloc_trace+0x27/0xa0
> [<00000000836cc910>] ulist_alloc+0x55/0xe0 [btrfs]
> [<00000000eb3781d2>] btrfs_find_all_roots_safe+0x9d/0x1c0 [btrfs]
> [<00000000cacfcca2>] test_multiple_refs.constprop.0+0x1e0/0x470 [btrfs]
> [<000000008613167f>] btrfs_test_qgroups+0x2da/0x300 [btrfs]
> [<0000000047d4f295>] btrfs_run_sanity_tests.cold+0x5c/0xd5 [btrfs]
> [<00000000b09fac49>] init_btrfs_fs+0xec/0x154 [btrfs]
> [<000000002925cdf3>] do_one_initcall+0x87/0x2a0
> [<00000000c5ed267e>] do_init_module+0xdf/0x320
> [<000000005f972694>] load_module+0x3006/0x3390
> [<0000000070c88d9a>] __do_sys_finit_module+0x113/0x1b0
> [<00000000e7029c2b>] do_syscall_64+0x35/0x80
> [<000000008ffc1dab>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Let's free the ulist memory when the no shared qgroup tests finish.
>
> Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
There's still one ulist leak missing.
I actually had a similar patch here that is part of a much larger patchset
but I hadn't sent it yet. So find the difference:
From: Filipe Manana <fdmanana@suse.com>
Date: Fri, 14 Oct 2022 16:37:10 +0100
Subject: [PATCH 03/18] btrfs: fix ulist leaks in error paths of qgroup self
tests
In the test_no_shared_qgroup() and test_multiple_refs() qgroup self tests,
if we fail to add the tree ref, remove the extent item or remove the
extent ref, we are returning from the test function without freeing the
"old_roots" ulist that was allocated by the previous calls to
btrfs_find_all_roots(). Fix that by calling ulist_free() before returning.
Fixes: 442244c96332 ("btrfs: qgroup: Switch self test to extent-oriented qgroup mechanism.")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/tests/qgroup-tests.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
index 94b04f10f61c..96a70ce36f79 100644
--- a/fs/btrfs/tests/qgroup-tests.c
+++ b/fs/btrfs/tests/qgroup-tests.c
@@ -234,8 +234,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root,
ret = insert_normal_tree_ref(root, nodesize, nodesize, 0,
BTRFS_FS_TREE_OBJECTID);
- if (ret)
+ if (ret) {
+ ulist_free(old_roots);
return ret;
+ }
ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
if (ret) {
@@ -268,8 +270,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root,
}
ret = remove_extent_item(root, nodesize, nodesize);
- if (ret)
+ if (ret) {
+ ulist_free(old_roots);
return -EINVAL;
+ }
ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
if (ret) {
@@ -331,8 +335,10 @@ static int test_multiple_refs(struct btrfs_root *root,
ret = insert_normal_tree_ref(root, nodesize, nodesize, 0,
BTRFS_FS_TREE_OBJECTID);
- if (ret)
+ if (ret) {
+ ulist_free(old_roots);
return ret;
+ }
ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
if (ret) {
@@ -364,8 +370,10 @@ static int test_multiple_refs(struct btrfs_root *root,
ret = add_tree_ref(root, nodesize, nodesize, 0,
BTRFS_FIRST_FREE_OBJECTID);
- if (ret)
+ if (ret) {
+ ulist_free(old_roots);
return ret;
+ }
ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
if (ret) {
@@ -403,8 +411,10 @@ static int test_multiple_refs(struct btrfs_root *root,
ret = remove_extent_ref(root, nodesize, nodesize, 0,
BTRFS_FIRST_FREE_OBJECTID);
- if (ret)
+ if (ret) {
+ ulist_free(old_roots);
return ret;
+ }
ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
if (ret) {
--
2.35.1
> ---
> fs/btrfs/tests/qgroup-tests.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
> index 4172bef5b4a1..043b139cfb8c 100644
> --- a/fs/btrfs/tests/qgroup-tests.c
> +++ b/fs/btrfs/tests/qgroup-tests.c
> @@ -266,8 +266,10 @@ static int test_no_shared_qgroup(struct btrfs_root *root,
> }
>
> ret = remove_extent_item(root, nodesize, nodesize);
> - if (ret)
> + if (ret) {
> + ulist_free(old_roots);
> return -EINVAL;
> + }
>
> ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
> if (ret) {
> @@ -329,8 +331,10 @@ static int test_multiple_refs(struct btrfs_root *root,
>
> ret = insert_normal_tree_ref(root, nodesize, nodesize, 0,
> BTRFS_FS_TREE_OBJECTID);
> - if (ret)
> + if (ret) {
> + ulist_free(old_roots);
> return ret;
> + }
>
> ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
> if (ret) {
> @@ -365,8 +369,10 @@ static int test_multiple_refs(struct btrfs_root *root,
>
> ret = add_tree_ref(root, nodesize, nodesize, 0,
> BTRFS_FIRST_FREE_OBJECTID);
> - if (ret)
> + if (ret) {
> + ulist_free(old_roots);
> return ret;
> + }
>
> ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
> if (ret) {
> @@ -407,8 +413,10 @@ static int test_multiple_refs(struct btrfs_root *root,
>
> ret = remove_extent_ref(root, nodesize, nodesize, 0,
> BTRFS_FIRST_FREE_OBJECTID);
> - if (ret)
> + if (ret) {
> + ulist_free(old_roots);
> return ret;
> + }
>
> ret = btrfs_find_all_roots(&trans, fs_info, nodesize, 0, &new_roots, false);
> if (ret) {
> --
> 2.31.1
>
prev parent reply other threads:[~2022-11-01 8:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-01 2:53 [PATCH 0/3] Fix UAF and kmemleak when sanity test Zhang Xiaoxu
2022-11-01 2:53 ` [PATCH 1/3] btrfs: Fix wrong check in btrfs_free_dummy_root() Zhang Xiaoxu
2022-11-01 4:10 ` Anand Jain
2022-11-03 16:44 ` David Sterba
2022-11-04 1:09 ` zhangxiaoxu (A)
2022-11-01 2:53 ` [PATCH 2/3] btrfs: Fix uaf of the ulist in test_multiple_refs() Zhang Xiaoxu
2022-11-01 8:18 ` Filipe Manana
2022-11-01 2:53 ` [PATCH 3/3] btrfs: Fix ulist memory leak " Zhang Xiaoxu
2022-11-01 8:15 ` Filipe Manana [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221101081550.GA3418818@falcondesktop \
--to=fdmanana@kernel.org \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=zhangxiaoxu5@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).