* [PATCH -next 0/2] Fix two potential memory leak @ 2022-09-19 14:40 Ye Bin 2022-09-19 14:40 ` [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() Ye Bin 2022-09-19 14:40 ` [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() Ye Bin 0 siblings, 2 replies; 9+ messages in thread From: Ye Bin @ 2022-09-19 14:40 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin Ye Bin (2): ext4: fix potential memory leak in ext4_fc_record_regions() ext4: fix potential memory leak in ext4_fc_record_modified_inode() fs/ext4/fast_commit.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-19 14:40 [PATCH -next 0/2] Fix two potential memory leak Ye Bin @ 2022-09-19 14:40 ` Ye Bin 2022-09-19 15:26 ` Jan Kara 2022-09-19 18:40 ` Damien Guibouret 2022-09-19 14:40 ` [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() Ye Bin 1 sibling, 2 replies; 9+ messages in thread From: Ye Bin @ 2022-09-19 14:40 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin 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) return -ENOMEM; + state->fc_regions = fc_regions; } region = &state->fc_regions[state->fc_regions_used++]; region->ino = ino; -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-19 14:40 ` [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() Ye Bin @ 2022-09-19 15:26 ` Jan Kara 2022-09-19 18:40 ` Damien Guibouret 1 sibling, 0 replies; 9+ messages in thread From: Jan Kara @ 2022-09-19 15:26 UTC (permalink / raw) To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack 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 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-19 14:40 ` [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() Ye Bin 2022-09-19 15:26 ` Jan Kara @ 2022-09-19 18:40 ` Damien Guibouret 2022-09-20 1:07 ` yebin 1 sibling, 1 reply; 9+ messages in thread From: Damien Guibouret @ 2022-09-19 18:40 UTC (permalink / raw) To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-19 18:40 ` Damien Guibouret @ 2022-09-20 1:07 ` yebin 2022-09-20 18:25 ` Damien Guibouret 0 siblings, 1 reply; 9+ messages in thread From: yebin @ 2022-09-20 1:07 UTC (permalink / raw) To: Damien Guibouret, tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-20 1:07 ` yebin @ 2022-09-20 18:25 ` Damien Guibouret 2022-09-21 1:26 ` yebin 0 siblings, 1 reply; 9+ messages in thread From: Damien Guibouret @ 2022-09-20 18:25 UTC (permalink / raw) To: yebin, tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() 2022-09-20 18:25 ` Damien Guibouret @ 2022-09-21 1:26 ` yebin 0 siblings, 0 replies; 9+ messages in thread From: yebin @ 2022-09-21 1:26 UTC (permalink / raw) To: Damien Guibouret, tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack 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 > > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() 2022-09-19 14:40 [PATCH -next 0/2] Fix two potential memory leak Ye Bin 2022-09-19 14:40 ` [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() Ye Bin @ 2022-09-19 14:40 ` Ye Bin 2022-09-19 15:26 ` Jan Kara 1 sibling, 1 reply; 9+ messages in thread From: Ye Bin @ 2022-09-19 14:40 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin As krealloc may return NULL, in this case 'state->fc_modified_inodes' may not be freed by krealloc, but 'state->fc_modified_inodes' already set NULL. Then will lead to 'state->fc_modified_inodes' memory leak. Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/ext4/fast_commit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index cc8c8db075ba..5ab58cb4ce8d 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -1486,13 +1486,15 @@ static int ext4_fc_record_modified_inode(struct super_block *sb, int ino) if (state->fc_modified_inodes[i] == ino) return 0; if (state->fc_modified_inodes_used == state->fc_modified_inodes_size) { - state->fc_modified_inodes = krealloc( - state->fc_modified_inodes, + int *fc_modified_inodes; + + fc_modified_inodes = krealloc(state->fc_modified_inodes, sizeof(int) * (state->fc_modified_inodes_size + EXT4_FC_REPLAY_REALLOC_INCREMENT), GFP_KERNEL); - if (!state->fc_modified_inodes) + if (!fc_modified_inodes) return -ENOMEM; + state->fc_modified_inodes = fc_modified_inodes; state->fc_modified_inodes_size += EXT4_FC_REPLAY_REALLOC_INCREMENT; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() 2022-09-19 14:40 ` [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() Ye Bin @ 2022-09-19 15:26 ` Jan Kara 0 siblings, 0 replies; 9+ messages in thread From: Jan Kara @ 2022-09-19 15:26 UTC (permalink / raw) To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack On Mon 19-09-22 22:40:21, Ye Bin wrote: > As krealloc may return NULL, in this case 'state->fc_modified_inodes' > may not be freed by krealloc, but 'state->fc_modified_inodes' already > set NULL. Then will lead to 'state->fc_modified_inodes' 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 | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index cc8c8db075ba..5ab58cb4ce8d 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -1486,13 +1486,15 @@ static int ext4_fc_record_modified_inode(struct super_block *sb, int ino) > if (state->fc_modified_inodes[i] == ino) > return 0; > if (state->fc_modified_inodes_used == state->fc_modified_inodes_size) { > - state->fc_modified_inodes = krealloc( > - state->fc_modified_inodes, > + int *fc_modified_inodes; > + > + fc_modified_inodes = krealloc(state->fc_modified_inodes, > sizeof(int) * (state->fc_modified_inodes_size + > EXT4_FC_REPLAY_REALLOC_INCREMENT), > GFP_KERNEL); > - if (!state->fc_modified_inodes) > + if (!fc_modified_inodes) > return -ENOMEM; > + state->fc_modified_inodes = fc_modified_inodes; > state->fc_modified_inodes_size += > EXT4_FC_REPLAY_REALLOC_INCREMENT; > } > -- > 2.31.1 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-21 1:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-19 14:40 [PATCH -next 0/2] Fix two potential memory leak Ye Bin 2022-09-19 14:40 ` [PATCH -next 1/2] ext4: fix potential memory leak in ext4_fc_record_regions() Ye Bin 2022-09-19 15:26 ` Jan Kara 2022-09-19 18:40 ` Damien Guibouret 2022-09-20 1:07 ` yebin 2022-09-20 18:25 ` Damien Guibouret 2022-09-21 1:26 ` yebin 2022-09-19 14:40 ` [PATCH -next 2/2] ext4: fix potential memory leak in ext4_fc_record_modified_inode() Ye Bin 2022-09-19 15:26 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).