* [PATCH] btrfs: ref-verify: fix memory leaks
@ 2023-11-12 16:56 Bragatheswaran Manickavel
2023-11-13 13:20 ` Filipe Manana
0 siblings, 1 reply; 5+ messages in thread
From: Bragatheswaran Manickavel @ 2023-11-12 16:56 UTC (permalink / raw)
To: clm, josef, dsterba
Cc: Bragatheswaran Manickavel, linux-btrfs, linux-kernel,
syzbot+d66de4cbf532749df35f
In btrfs_ref_tree_mod(), when !parent 're' was allocated
through kmalloc(). In the following code, if an error occurs,
the execution will be redirected to 'out' or 'out_unlock' and
the function will be exited. However, on some of the paths,
're' are not deallocated and may lead to memory leaks.
For example : lookup_block_entry() for 'be' returns null, the
out label will be invoked. During that flow ref and ra was
freed but not re, which can potentially lead to memleak
Reported-and-tested-by: syzbot+d66de4cbf532749df35f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f
Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
---
fs/btrfs/ref-verify.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index 95d28497de7c..50b59b3dc474 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -791,6 +791,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
dump_ref_action(fs_info, ra);
kfree(ref);
kfree(ra);
+ kfree(re);
goto out_unlock;
} else if (be->num_refs == 0) {
btrfs_err(fs_info,
@@ -800,6 +801,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
dump_ref_action(fs_info, ra);
kfree(ref);
kfree(ra);
+ kfree(re);
goto out_unlock;
}
@@ -822,6 +824,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
dump_ref_action(fs_info, ra);
kfree(ref);
kfree(ra);
+ kfree(re);
goto out_unlock;
}
exist->num_refs--;
@@ -838,6 +841,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
dump_ref_action(fs_info, ra);
kfree(ref);
kfree(ra);
+ kfree(re);
goto out_unlock;
}
kfree(ref);
@@ -849,6 +853,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
dump_ref_action(fs_info, ra);
kfree(ref);
kfree(ra);
+ kfree(re);
goto out_unlock;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: ref-verify: fix memory leaks
2023-11-12 16:56 [PATCH] btrfs: ref-verify: fix memory leaks Bragatheswaran Manickavel
@ 2023-11-13 13:20 ` Filipe Manana
2023-11-13 13:49 ` Bragatheswaran Manickavel
0 siblings, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2023-11-13 13:20 UTC (permalink / raw)
To: Bragatheswaran Manickavel
Cc: clm, josef, dsterba, linux-btrfs, linux-kernel,
syzbot+d66de4cbf532749df35f
On Sun, Nov 12, 2023 at 4:57 PM Bragatheswaran Manickavel
<bragathemanick0908@gmail.com> wrote:
>
> In btrfs_ref_tree_mod(), when !parent 're' was allocated
> through kmalloc(). In the following code, if an error occurs,
> the execution will be redirected to 'out' or 'out_unlock' and
> the function will be exited. However, on some of the paths,
> 're' are not deallocated and may lead to memory leaks.
>
> For example : lookup_block_entry() for 'be' returns null, the
> out label will be invoked. During that flow ref and ra was
> freed but not re, which can potentially lead to memleak
>
> Reported-and-tested-by: syzbot+d66de4cbf532749df35f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f
> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
> ---
> fs/btrfs/ref-verify.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
> index 95d28497de7c..50b59b3dc474 100644
> --- a/fs/btrfs/ref-verify.c
> +++ b/fs/btrfs/ref-verify.c
> @@ -791,6 +791,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
> dump_ref_action(fs_info, ra);
> kfree(ref);
> kfree(ra);
> + kfree(re);
Here it's fine, 're' was not yet added to the rbtree (be->roots).
> goto out_unlock;
> } else if (be->num_refs == 0) {
> btrfs_err(fs_info,
> @@ -800,6 +801,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
> dump_ref_action(fs_info, ra);
> kfree(ref);
> kfree(ra);
> + kfree(re);
Same here.
> goto out_unlock;
> }
>
> @@ -822,6 +824,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
> dump_ref_action(fs_info, ra);
> kfree(ref);
> kfree(ra);
> + kfree(re);
Here it's not ok. 're' was added to the rbtree, so you can't free it,
as later when accessing the tree, it will trigger a use-after-free
bug.
> goto out_unlock;
> }
> exist->num_refs--;
> @@ -838,6 +841,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
> dump_ref_action(fs_info, ra);
> kfree(ref);
> kfree(ra);
> + kfree(re);
Same here, it will lead to a use-after-free.
> goto out_unlock;
> }
> kfree(ref);
> @@ -849,6 +853,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
> dump_ref_action(fs_info, ra);
> kfree(ref);
> kfree(ra);
> + kfree(re);
Same here, it will lead to a use-after-free.
Thanks.
> goto out_unlock;
> }
> }
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: ref-verify: fix memory leaks
2023-11-13 13:20 ` Filipe Manana
@ 2023-11-13 13:49 ` Bragatheswaran Manickavel
0 siblings, 0 replies; 5+ messages in thread
From: Bragatheswaran Manickavel @ 2023-11-13 13:49 UTC (permalink / raw)
To: Filipe Manana
Cc: clm, josef, dsterba, linux-btrfs, linux-kernel,
syzbot+d66de4cbf532749df35f
On 13/11/23 18:50, Filipe Manana wrote:
> On Sun, Nov 12, 2023 at 4:57 PM Bragatheswaran Manickavel
> <bragathemanick0908@gmail.com> wrote:
>> In btrfs_ref_tree_mod(), when !parent 're' was allocated
>> through kmalloc(). In the following code, if an error occurs,
>> the execution will be redirected to 'out' or 'out_unlock' and
>> the function will be exited. However, on some of the paths,
>> 're' are not deallocated and may lead to memory leaks.
>>
>> For example : lookup_block_entry() for 'be' returns null, the
>> out label will be invoked. During that flow ref and ra was
>> freed but not re, which can potentially lead to memleak
>>
>> Reported-and-tested-by: syzbot+d66de4cbf532749df35f@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=d66de4cbf532749df35f
>> Signed-off-by: Bragatheswaran Manickavel <bragathemanick0908@gmail.com>
>> ---
>> fs/btrfs/ref-verify.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
>> index 95d28497de7c..50b59b3dc474 100644
>> --- a/fs/btrfs/ref-verify.c
>> +++ b/fs/btrfs/ref-verify.c
>> @@ -791,6 +791,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
>> dump_ref_action(fs_info, ra);
>> kfree(ref);
>> kfree(ra);
>> + kfree(re);
> Here it's fine, 're' was not yet added to the rbtree (be->roots).
>
>> goto out_unlock;
>> } else if (be->num_refs == 0) {
>> btrfs_err(fs_info,
>> @@ -800,6 +801,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
>> dump_ref_action(fs_info, ra);
>> kfree(ref);
>> kfree(ra);
>> + kfree(re);
> Same here.
>
>> goto out_unlock;
>> }
>>
>> @@ -822,6 +824,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
>> dump_ref_action(fs_info, ra);
>> kfree(ref);
>> kfree(ra);
>> + kfree(re);
> Here it's not ok. 're' was added to the rbtree, so you can't free it,
> as later when accessing the tree, it will trigger a use-after-free
> bug.
>
>> goto out_unlock;
>> }
>> exist->num_refs--;
>> @@ -838,6 +841,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
>> dump_ref_action(fs_info, ra);
>> kfree(ref);
>> kfree(ra);
>> + kfree(re);
> Same here, it will lead to a use-after-free.
>
>> goto out_unlock;
>> }
>> kfree(ref);
>> @@ -849,6 +853,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
>> dump_ref_action(fs_info, ra);
>> kfree(ref);
>> kfree(ra);
>> + kfree(re);
> Same here, it will lead to a use-after-free.
>
> Thanks.
>> goto out_unlock;
>> }
>> }
>> --
>> 2.34.1
Thanks Filipe for reviewing this!
Now, I understood why we shouldn't free 're' after it was added to the
tree.
In that case, can I send a new patch with first two changes.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] btrfs: ref-verify: fix memory leaks
@ 2020-02-01 20:38 Wenwen Wang
2020-02-04 16:14 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Wenwen Wang @ 2020-02-01 20:38 UTC (permalink / raw)
To: Wenwen Wang
Cc: Chris Mason, Josef Bacik, David Sterba,
open list:BTRFS FILE SYSTEM, open list
In btrfs_ref_tree_mod(), 'ref' and 'ra' are allocated through kzalloc() and
kmalloc(), respectively. In the following code, if an error occurs, the
execution will be redirected to 'out' or 'out_unlock' and the function will
be exited. However, on some of the paths, 'ref' and 'ra' are not
deallocated, leading to memory leaks. For example, if 'action' is
BTRFS_ADD_DELAYED_EXTENT, add_block_entry() will be invoked. If the return
value indicates an error, the execution will be redirected to 'out'. But,
'ref' is not deallocated on this path, causing a memory leak.
To fix the above issues, deallocate both 'ref' and 'ra' before exiting from
the function when an error is encountered.
Signed-off-by: Wenwen Wang <wenwen@cs.uga.edu>
---
fs/btrfs/ref-verify.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index b57f3618e58e..454a1015d026 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -744,6 +744,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
*/
be = add_block_entry(fs_info, bytenr, num_bytes, ref_root);
if (IS_ERR(be)) {
+ kfree(ref);
kfree(ra);
ret = PTR_ERR(be);
goto out;
@@ -757,6 +758,8 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
"re-allocated a block that still has references to it!");
dump_block_entry(fs_info, be);
dump_ref_action(fs_info, ra);
+ kfree(ref);
+ kfree(ra);
goto out_unlock;
}
@@ -819,6 +822,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
"dropping a ref for a existing root that doesn't have a ref on the block");
dump_block_entry(fs_info, be);
dump_ref_action(fs_info, ra);
+ kfree(ref);
kfree(ra);
goto out_unlock;
}
@@ -834,6 +838,7 @@ int btrfs_ref_tree_mod(struct btrfs_fs_info *fs_info,
"attempting to add another ref for an existing ref on a tree block");
dump_block_entry(fs_info, be);
dump_ref_action(fs_info, ra);
+ kfree(ref);
kfree(ra);
goto out_unlock;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: ref-verify: fix memory leaks
2020-02-01 20:38 Wenwen Wang
@ 2020-02-04 16:14 ` David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2020-02-04 16:14 UTC (permalink / raw)
To: Wenwen Wang
Cc: Chris Mason, Josef Bacik, David Sterba,
open list:BTRFS FILE SYSTEM, open list
On Sat, Feb 01, 2020 at 08:38:38PM +0000, Wenwen Wang wrote:
> In btrfs_ref_tree_mod(), 'ref' and 'ra' are allocated through kzalloc() and
> kmalloc(), respectively. In the following code, if an error occurs, the
> execution will be redirected to 'out' or 'out_unlock' and the function will
> be exited. However, on some of the paths, 'ref' and 'ra' are not
> deallocated, leading to memory leaks. For example, if 'action' is
> BTRFS_ADD_DELAYED_EXTENT, add_block_entry() will be invoked. If the return
> value indicates an error, the execution will be redirected to 'out'. But,
> 'ref' is not deallocated on this path, causing a memory leak.
>
> To fix the above issues, deallocate both 'ref' and 'ra' before exiting from
> the function when an error is encountered.
Right, the kfrees are missing. The whole function needs to be
restructured otherwise it's too easy to get lost in the conditions and
what needs to be cleaned up after errors but that's for a separate
patch. Added to devel queue, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-13 13:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-12 16:56 [PATCH] btrfs: ref-verify: fix memory leaks Bragatheswaran Manickavel
2023-11-13 13:20 ` Filipe Manana
2023-11-13 13:49 ` Bragatheswaran Manickavel
-- strict thread matches above, loose matches on Subject: below --
2020-02-01 20:38 Wenwen Wang
2020-02-04 16:14 ` David Sterba
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).