diff mbox series

btrfs: fix memory leaks after failure to lookup checksums during inode logging

Message ID 20200729091750.2538306-1-fdmanana@kernel.org (mailing list archive)
State New, archived
Headers show
Series btrfs: fix memory leaks after failure to lookup checksums during inode logging | expand

Commit Message

Filipe Manana July 29, 2020, 9:17 a.m. UTC 
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(-)

Comments

Johannes Thumshirn July 29, 2020, 10:09 a.m. UTC | #1 
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
Filipe Manana July 29, 2020, 10:18 a.m. UTC | #2 
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
Johannes Thumshirn July 29, 2020, 10:21 a.m. UTC | #3 
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>
David Sterba July 29, 2020, 6:49 p.m. UTC | #4 
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.
diff mbox series

Patch

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,