| Message ID | 20220919144021.2162295-2-yebin10@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix two potential memory leak | expand |
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 >
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
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 > > . >
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
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 --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;
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(-)