Re: [PATCH 1/2] btrfs-progs: fix a memory leak when starting a transaction on fs with error

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/4/19 13:09, Andrei Borzenkov wrote:
> On 18.04.2022 16:10, Qu Wenruo wrote:
>> Function btrfs_start_transaction() will allocate the memory
>> unconditionally, but if the fs has an aborted transaction we don't free
>> the allocated memory but return error directly.
>>
>> Fix it by only allocate the new memory after the transaction_aborted
>> check.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   kernel-shared/transaction.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c
>> index 0201226678ba..799520a0ef71 100644
>> --- a/kernel-shared/transaction.c
>> +++ b/kernel-shared/transaction.c
>> @@ -25,13 +25,15 @@ struct btrfs_trans_handle* btrfs_start_transaction(struct btrfs_root *root,
>>   		int num_blocks)
>>   {
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>> -	struct btrfs_trans_handle *h = kzalloc(sizeof(*h), GFP_NOFS);
>> -
>> +	struct btrfs_trans_handle *h;
>> +
>>   	if (fs_info->transaction_aborted)
>>   		return ERR_PTR(-EROFS);
>>
>> +	h = kzalloc(sizeof(*h), GFP_NOFS);
>>   	if (!h)
>>   		return ERR_PTR(-ENOMEM);
>> +
>>   	if (root->commit_root) {
>>   		error("commit_root already set when starting transaction");
>>   		kfree(h);
>
> If you are moving allocation of h anyway, why not move it beyond all
> checks and delete redundant kfree(h)?

Good idea.

Thanks,
Qu