* [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging
@ 2020-07-29 9:17 fdmanana
2020-07-29 10:09 ` Johannes Thumshirn
2020-07-29 18:49 ` David Sterba
0 siblings, 2 replies; 5+ messages in thread
From: fdmanana @ 2020-07-29 9:17 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
While logging an inode, at copy_items(), if we fail to lookup the checksums
for an extent we release the destination path, free the ins_data array and
then return immediately. However a previous iteration of the for loop may
have added checksums to the ordered_sums list, in which case we leak the
memory used by them.
So fix this by making sure we iterate the ordered_sums list and free all
its checksums before returning.
Fixes: 3650860b90cc2a ("Btrfs: remove almost all of the BUG()'s from tree-log.c")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/tree-log.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 20334bebcaf2..f1812f5baec4 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4035,11 +4035,8 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
fs_info->csum_root,
ds + cs, ds + cs + cl - 1,
&ordered_sums, 0);
- if (ret) {
- btrfs_release_path(dst_path);
- kfree(ins_data);
- return ret;
- }
+ if (ret)
+ break;
}
}
}
@@ -4052,7 +4049,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
* we have to do this after the loop above to avoid changing the
* log tree while trying to change the log tree.
*/
- ret = 0;
while (!list_empty(&ordered_sums)) {
struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
struct btrfs_ordered_sum,
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging
2020-07-29 9:17 [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging fdmanana
@ 2020-07-29 10:09 ` Johannes Thumshirn
2020-07-29 10:18 ` Filipe Manana
2020-07-29 18:49 ` David Sterba
1 sibling, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2020-07-29 10:09 UTC (permalink / raw)
To: fdmanana@kernel.org, linux-btrfs@vger.kernel.org
On 29/07/2020 11:18, fdmanana@kernel.org wrote:
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 20334bebcaf2..f1812f5baec4 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -4035,11 +4035,8 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
> fs_info->csum_root,
> ds + cs, ds + cs + cl - 1,
> &ordered_sums, 0);
> - if (ret) {
> - btrfs_release_path(dst_path);
> - kfree(ins_data);
> - return ret;
> - }
> + if (ret)
> + break;
> }
> }
> }
> @@ -4052,7 +4049,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
> * we have to do this after the loop above to avoid changing the
> * log tree while trying to change the log tree.
> */
> - ret = 0;
> while (!list_empty(&ordered_sums)) {
> struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
Isn't there a potential that ret get overridden by this hunk:
while (!list_empty(&ordered_sums)) {
struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
struct btrfs_ordered_sum,
list);
if (!ret)
ret = log_csums(trans, inode, log, sums);
list_del(&sums->list);
kfree(sums);
}
return ret;
and we're potentially returning 0 instead of the -ENOMEM from 'btrfs_lookup_csums_range'?
Maybe we should do something like this:
while (!list_empty(&ordered_sums)) {
struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
struct btrfs_ordered_sum,
list);
if (!ret)
ret2 = log_csums(trans, inode, log, sums);
list_del(&sums->list);
kfree(sums);
}
return ret2 ? ret2 : ret;
Thanks,
Johannes
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging
2020-07-29 10:09 ` Johannes Thumshirn
@ 2020-07-29 10:18 ` Filipe Manana
2020-07-29 10:21 ` Johannes Thumshirn
0 siblings, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2020-07-29 10:18 UTC (permalink / raw)
To: Johannes Thumshirn; +Cc: linux-btrfs@vger.kernel.org
On Wed, Jul 29, 2020 at 11:09 AM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 29/07/2020 11:18, fdmanana@kernel.org wrote:
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 20334bebcaf2..f1812f5baec4 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -4035,11 +4035,8 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
> > fs_info->csum_root,
> > ds + cs, ds + cs + cl - 1,
> > &ordered_sums, 0);
> > - if (ret) {
> > - btrfs_release_path(dst_path);
> > - kfree(ins_data);
> > - return ret;
> > - }
> > + if (ret)
> > + break;
> > }
> > }
> > }
> > @@ -4052,7 +4049,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
> > * we have to do this after the loop above to avoid changing the
> > * log tree while trying to change the log tree.
> > */
> > - ret = 0;
> > while (!list_empty(&ordered_sums)) {
> > struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
>
> Isn't there a potential that ret get overridden by this hunk:
>
> while (!list_empty(&ordered_sums)) {
> struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
> struct btrfs_ordered_sum,
> list);
> if (!ret)
> ret = log_csums(trans, inode, log, sums);
> list_del(&sums->list);
> kfree(sums);
> }
Nop, when ret is non-zero it never gets overwritten.
>
> return ret;
>
> and we're potentially returning 0 instead of the -ENOMEM from 'btrfs_lookup_csums_range'?
Nop, for the same reason mentioned before.
Thanks.
>
> Maybe we should do something like this:
>
> while (!list_empty(&ordered_sums)) {
> struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
> struct btrfs_ordered_sum,
> list);
> if (!ret)
> ret2 = log_csums(trans, inode, log, sums);
> list_del(&sums->list);
> kfree(sums);
> }
>
> return ret2 ? ret2 : ret;
>
> Thanks,
> Johannes
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging
2020-07-29 10:18 ` Filipe Manana
@ 2020-07-29 10:21 ` Johannes Thumshirn
0 siblings, 0 replies; 5+ messages in thread
From: Johannes Thumshirn @ 2020-07-29 10:21 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs@vger.kernel.org
On 29/07/2020 12:18, Filipe Manana wrote:
> On Wed, Jul 29, 2020 at 11:09 AM Johannes Thumshirn
> <Johannes.Thumshirn@wdc.com> wrote:
>>
>> On 29/07/2020 11:18, fdmanana@kernel.org wrote:
>>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>>> index 20334bebcaf2..f1812f5baec4 100644
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -4035,11 +4035,8 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
>>> fs_info->csum_root,
>>> ds + cs, ds + cs + cl - 1,
>>> &ordered_sums, 0);
>>> - if (ret) {
>>> - btrfs_release_path(dst_path);
>>> - kfree(ins_data);
>>> - return ret;
>>> - }
>>> + if (ret)
>>> + break;
>>> }
>>> }
>>> }
>>> @@ -4052,7 +4049,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
>>> * we have to do this after the loop above to avoid changing the
>>> * log tree while trying to change the log tree.
>>> */
>>> - ret = 0;
>>> while (!list_empty(&ordered_sums)) {
>>> struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
>>
>> Isn't there a potential that ret get overridden by this hunk:
>>
>> while (!list_empty(&ordered_sums)) {
>> struct btrfs_ordered_sum *sums = list_entry(ordered_sums.next,
>> struct btrfs_ordered_sum,
>> list);
>> if (!ret)
>> ret = log_csums(trans, inode, log, sums);
>> list_del(&sums->list);
>> kfree(sums);
>> }
>
> Nop, when ret is non-zero it never gets overwritten.
You're absolutely right, I'm sorry.
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging
2020-07-29 9:17 [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging fdmanana
2020-07-29 10:09 ` Johannes Thumshirn
@ 2020-07-29 18:49 ` David Sterba
1 sibling, 0 replies; 5+ messages in thread
From: David Sterba @ 2020-07-29 18:49 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Wed, Jul 29, 2020 at 10:17:50AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> While logging an inode, at copy_items(), if we fail to lookup the checksums
> for an extent we release the destination path, free the ins_data array and
> then return immediately. However a previous iteration of the for loop may
> have added checksums to the ordered_sums list, in which case we leak the
> memory used by them.
>
> So fix this by making sure we iterate the ordered_sums list and free all
> its checksums before returning.
>
> Fixes: 3650860b90cc2a ("Btrfs: remove almost all of the BUG()'s from tree-log.c")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Added to misc-next, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-29 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-29 9:17 [PATCH] btrfs: fix memory leaks after failure to lookup checksums during inode logging fdmanana
2020-07-29 10:09 ` Johannes Thumshirn
2020-07-29 10:18 ` Filipe Manana
2020-07-29 10:21 ` Johannes Thumshirn
2020-07-29 18:49 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox