diff mbox series

[-next,1/2] ext4: fix potential memory leak in ext4_fc_record_regions()

Message ID 20220919144021.2162295-2-yebin10@huawei.com
State Superseded
Headers show
Series Fix two potential memory leak | expand

Commit Message

yebin (H) Sept. 19, 2022, 2:40 p.m. UTC
As krealloc may return NULL, in this case 'state->fc_regions' may not be
freed by krealloc, but 'state->fc_regions' already set NULL. Then will
lead to 'state->fc_regions' memory leak.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/fast_commit.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Jan Kara Sept. 19, 2022, 3:26 p.m. UTC | #1
On Mon 19-09-22 22:40:20, Ye Bin wrote:
> As krealloc may return NULL, in this case 'state->fc_regions' may not be
> freed by krealloc, but 'state->fc_regions' already set NULL. Then will
> lead to 'state->fc_regions' memory leak.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/fast_commit.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 9217a588afd1..cc8c8db075ba 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1677,15 +1677,17 @@ int ext4_fc_record_regions(struct super_block *sb, int ino,
>  	if (replay && state->fc_regions_used != state->fc_regions_valid)
>  		state->fc_regions_used = state->fc_regions_valid;
>  	if (state->fc_regions_used == state->fc_regions_size) {
> +		struct ext4_fc_alloc_region *fc_regions;
> +
>  		state->fc_regions_size +=
>  			EXT4_FC_REPLAY_REALLOC_INCREMENT;
> -		state->fc_regions = krealloc(
> -					state->fc_regions,
> -					state->fc_regions_size *
> -					sizeof(struct ext4_fc_alloc_region),
> -					GFP_KERNEL);
> -		if (!state->fc_regions)
> +		fc_regions = krealloc(state->fc_regions,
> +				      state->fc_regions_size *
> +				      sizeof(struct ext4_fc_alloc_region),
> +				      GFP_KERNEL);
> +		if (!fc_regions)
>  			return -ENOMEM;
> +		state->fc_regions = fc_regions;
>  	}
>  	region = &state->fc_regions[state->fc_regions_used++];
>  	region->ino = ino;
> -- 
> 2.31.1
>
Damien Guibouret Sept. 19, 2022, 6:40 p.m. UTC | #2
Hello,

Le 19/09/2022 à 16:40, Ye Bin a écrit :
> As krealloc may return NULL, in this case 'state->fc_regions' may not be
> freed by krealloc, but 'state->fc_regions' already set NULL. Then will
> lead to 'state->fc_regions' memory leak.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>   fs/ext4/fast_commit.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 9217a588afd1..cc8c8db075ba 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -1677,15 +1677,17 @@ int ext4_fc_record_regions(struct super_block *sb, int ino,
>   	if (replay && state->fc_regions_used != state->fc_regions_valid)
>   		state->fc_regions_used = state->fc_regions_valid;
>   	if (state->fc_regions_used == state->fc_regions_size) {
> +		struct ext4_fc_alloc_region *fc_regions;
> +
>   		state->fc_regions_size +=
>   			EXT4_FC_REPLAY_REALLOC_INCREMENT;
> -		state->fc_regions = krealloc(
> -					state->fc_regions,
> -					state->fc_regions_size *
> -					sizeof(struct ext4_fc_alloc_region),
> -					GFP_KERNEL);
> -		if (!state->fc_regions)
> +		fc_regions = krealloc(state->fc_regions,
> +				      state->fc_regions_size *
> +				      sizeof(struct ext4_fc_alloc_region),
> +				      GFP_KERNEL);
> +		if (!fc_regions)

Would it not be safer to restore state->fc_regions_size to its previous 
value in that case to keep consistency between size value and allocated 
size (or to update state->fc_regions_size only after allocation as it is 
done in second part of this patch) ?

>   			return -ENOMEM;
> +		state->fc_regions = fc_regions;
>   	}
>   	region = &state->fc_regions[state->fc_regions_used++];
>   	region->ino = ino;

Regards,

Damien
yebin (H) Sept. 20, 2022, 1:07 a.m. UTC | #3
On 2022/9/20 2:40, Damien Guibouret wrote:
> Hello,
>
> Le 19/09/2022 à 16:40, Ye Bin a écrit :
>> As krealloc may return NULL, in this case 'state->fc_regions' may not be
>> freed by krealloc, but 'state->fc_regions' already set NULL. Then will
>> lead to 'state->fc_regions' memory leak.
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>>   fs/ext4/fast_commit.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
>> index 9217a588afd1..cc8c8db075ba 100644
>> --- a/fs/ext4/fast_commit.c
>> +++ b/fs/ext4/fast_commit.c
>> @@ -1677,15 +1677,17 @@ int ext4_fc_record_regions(struct super_block 
>> *sb, int ino,
>>       if (replay && state->fc_regions_used != state->fc_regions_valid)
>>           state->fc_regions_used = state->fc_regions_valid;
>>       if (state->fc_regions_used == state->fc_regions_size) {
>> +        struct ext4_fc_alloc_region *fc_regions;
>> +
>>           state->fc_regions_size +=
>>               EXT4_FC_REPLAY_REALLOC_INCREMENT;
>> -        state->fc_regions = krealloc(
>> -                    state->fc_regions,
>> -                    state->fc_regions_size *
>> -                    sizeof(struct ext4_fc_alloc_region),
>> -                    GFP_KERNEL);
>> -        if (!state->fc_regions)
>> +        fc_regions = krealloc(state->fc_regions,
>> +                      state->fc_regions_size *
>> +                      sizeof(struct ext4_fc_alloc_region),
>> +                      GFP_KERNEL);
>> +        if (!fc_regions)
>
> Would it not be safer to restore state->fc_regions_size to its 
> previous value in that case to keep consistency between size value and 
> allocated size (or to update state->fc_regions_size only after 
> allocation as it is done in second part of this patch) ?
>
Actually, If   'ext4_fc_record_regions()' return -ENOMEM, then will stop 
replay journal.
'state->fc_regions_size' will not be used any more, so it's safe.
>>               return -ENOMEM;
>> +        state->fc_regions = fc_regions;
>>       }
>>       region = &state->fc_regions[state->fc_regions_used++];
>>       region->ino = ino;
>
> Regards,
>
> Damien
>
> .
>
Damien Guibouret Sept. 20, 2022, 6:25 p.m. UTC | #4
Hello,

Le 20/09/2022 à 03:07, yebin a écrit :
> 
> 
> On 2022/9/20 2:40, Damien Guibouret wrote:
>> Hello,
>>
>> Le 19/09/2022 à 16:40, Ye Bin a écrit :
>>> As krealloc may return NULL, in this case 'state->fc_regions' may not be
>>> freed by krealloc, but 'state->fc_regions' already set NULL. Then will
>>> lead to 'state->fc_regions' memory leak.
>>>
>>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>>> ---
>>>   fs/ext4/fast_commit.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
>>> index 9217a588afd1..cc8c8db075ba 100644
>>> --- a/fs/ext4/fast_commit.c
>>> +++ b/fs/ext4/fast_commit.c
>>> @@ -1677,15 +1677,17 @@ int ext4_fc_record_regions(struct super_block 
>>> *sb, int ino,
>>>       if (replay && state->fc_regions_used != state->fc_regions_valid)
>>>           state->fc_regions_used = state->fc_regions_valid;
>>>       if (state->fc_regions_used == state->fc_regions_size) {
>>> +        struct ext4_fc_alloc_region *fc_regions;
>>> +
>>>           state->fc_regions_size +=
>>>               EXT4_FC_REPLAY_REALLOC_INCREMENT;
>>> -        state->fc_regions = krealloc(
>>> -                    state->fc_regions,
>>> -                    state->fc_regions_size *
>>> -                    sizeof(struct ext4_fc_alloc_region),
>>> -                    GFP_KERNEL);
>>> -        if (!state->fc_regions)
>>> +        fc_regions = krealloc(state->fc_regions,
>>> +                      state->fc_regions_size *
>>> +                      sizeof(struct ext4_fc_alloc_region),
>>> +                      GFP_KERNEL);
>>> +        if (!fc_regions)
>>
>> Would it not be safer to restore state->fc_regions_size to its 
>> previous value in that case to keep consistency between size value and 
>> allocated size (or to update state->fc_regions_size only after 
>> allocation as it is done in second part of this patch) ?
>>
> Actually, If   'ext4_fc_record_regions()' return -ENOMEM, then will stop 
> replay journal.
> 'state->fc_regions_size' will not be used any more, so it's safe.

There are at least two calls in ext4_ext_clear_bb (ext4/extents.c) that 
do not check for return code of ext4_fc_record_regions. But perhaps 
these are these calls that should be fixed.

>>>               return -ENOMEM;
>>> +        state->fc_regions = fc_regions;
>>>       }
>>>       region = &state->fc_regions[state->fc_regions_used++];
>>>       region->ino = ino;
>>

Regards,

Damien
yebin (H) Sept. 21, 2022, 1:26 a.m. UTC | #5
On 2022/9/21 2:25, Damien Guibouret wrote:
> Hello,
>
> Le 20/09/2022 à 03:07, yebin a écrit :
>>
>>
>> On 2022/9/20 2:40, Damien Guibouret wrote:
>>> Hello,
>>>
>>> Le 19/09/2022 à 16:40, Ye Bin a écrit :
>>>> As krealloc may return NULL, in this case 'state->fc_regions' may 
>>>> not be
>>>> freed by krealloc, but 'state->fc_regions' already set NULL. Then will
>>>> lead to 'state->fc_regions' memory leak.
>>>>
>>>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>>>> ---
>>>>   fs/ext4/fast_commit.c | 14 ++++++++------
>>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
>>>> index 9217a588afd1..cc8c8db075ba 100644
>>>> --- a/fs/ext4/fast_commit.c
>>>> +++ b/fs/ext4/fast_commit.c
>>>> @@ -1677,15 +1677,17 @@ int ext4_fc_record_regions(struct 
>>>> super_block *sb, int ino,
>>>>       if (replay && state->fc_regions_used != state->fc_regions_valid)
>>>>           state->fc_regions_used = state->fc_regions_valid;
>>>>       if (state->fc_regions_used == state->fc_regions_size) {
>>>> +        struct ext4_fc_alloc_region *fc_regions;
>>>> +
>>>>           state->fc_regions_size +=
>>>>               EXT4_FC_REPLAY_REALLOC_INCREMENT;
>>>> -        state->fc_regions = krealloc(
>>>> -                    state->fc_regions,
>>>> -                    state->fc_regions_size *
>>>> -                    sizeof(struct ext4_fc_alloc_region),
>>>> -                    GFP_KERNEL);
>>>> -        if (!state->fc_regions)
>>>> +        fc_regions = krealloc(state->fc_regions,
>>>> +                      state->fc_regions_size *
>>>> +                      sizeof(struct ext4_fc_alloc_region),
>>>> +                      GFP_KERNEL);
>>>> +        if (!fc_regions)
>>>
>>> Would it not be safer to restore state->fc_regions_size to its 
>>> previous value in that case to keep consistency between size value 
>>> and allocated size (or to update state->fc_regions_size only after 
>>> allocation as it is done in second part of this patch) ?
>>>
>> Actually, If   'ext4_fc_record_regions()' return -ENOMEM, then will 
>> stop replay journal.
>> 'state->fc_regions_size' will not be used any more, so it's safe.
>
> There are at least two calls in ext4_ext_clear_bb (ext4/extents.c) 
> that do not check for return code of ext4_fc_record_regions. But 
> perhaps these are these calls that should be fixed.
>
Thanks very much,
Indeed, it's better to repair it here.
>>>>               return -ENOMEM;
>>>> +        state->fc_regions = fc_regions;
>>>>       }
>>>>       region = &state->fc_regions[state->fc_regions_used++];
>>>>       region->ino = ino;
>>>
>
> Regards,
>
> Damien
>
>
> .
>
diff mbox series

Patch

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 9217a588afd1..cc8c8db075ba 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -1677,15 +1677,17 @@  int ext4_fc_record_regions(struct super_block *sb, int ino,
 	if (replay && state->fc_regions_used != state->fc_regions_valid)
 		state->fc_regions_used = state->fc_regions_valid;
 	if (state->fc_regions_used == state->fc_regions_size) {
+		struct ext4_fc_alloc_region *fc_regions;
+
 		state->fc_regions_size +=
 			EXT4_FC_REPLAY_REALLOC_INCREMENT;
-		state->fc_regions = krealloc(
-					state->fc_regions,
-					state->fc_regions_size *
-					sizeof(struct ext4_fc_alloc_region),
-					GFP_KERNEL);
-		if (!state->fc_regions)
+		fc_regions = krealloc(state->fc_regions,
+				      state->fc_regions_size *
+				      sizeof(struct ext4_fc_alloc_region),
+				      GFP_KERNEL);
+		if (!fc_regions)
 			return -ENOMEM;
+		state->fc_regions = fc_regions;
 	}
 	region = &state->fc_regions[state->fc_regions_used++];
 	region->ino = ino;