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 |
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
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
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>
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 --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,