| Message ID | 4b779e7ac657f94f8680a8944bff191f7474db5b.1642044249.git.riteshh@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4/jbd2: inline_data fixes and some cleanups | expand |
On Thu 13-01-22 08:56:26, Ritesh Harjani wrote: > Current code does not fully takes care of krealloc() error case, > which could lead to silent memory corruption or a kernel bug. > This patch fixes that. > > Also it cleans up some duplicated error handling logic from various functions > in fast_commit.c file. > > Reported-by: luo penghao <luo.penghao@zte.com.cn> > Suggested-by: Lukas Czerner <lczerner@redhat.com> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/fast_commit.c | 64 ++++++++++++++++++++----------------------- > 1 file changed, 29 insertions(+), 35 deletions(-) > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index 5ae8026a0c56..4541c8468c01 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -1392,14 +1392,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_size += > - EXT4_FC_REPLAY_REALLOC_INCREMENT; > state->fc_modified_inodes = krealloc( > - state->fc_modified_inodes, sizeof(int) * > - state->fc_modified_inodes_size, > - GFP_KERNEL); > + state->fc_modified_inodes, > + sizeof(int) * (state->fc_modified_inodes_size + > + EXT4_FC_REPLAY_REALLOC_INCREMENT), > + GFP_KERNEL); > if (!state->fc_modified_inodes) > return -ENOMEM; > + state->fc_modified_inodes_size += > + EXT4_FC_REPLAY_REALLOC_INCREMENT; > } > state->fc_modified_inodes[state->fc_modified_inodes_used++] = ino; > return 0; > @@ -1431,7 +1432,9 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl, > } > inode = NULL; > > - ext4_fc_record_modified_inode(sb, ino); > + ret = ext4_fc_record_modified_inode(sb, ino); > + if (ret) > + goto out; > > raw_fc_inode = (struct ext4_inode *) > (val + offsetof(struct ext4_fc_inode, fc_raw_inode)); > @@ -1621,6 +1624,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > } > > ret = ext4_fc_record_modified_inode(sb, inode->i_ino); > + if (ret) > + goto out; > > start = le32_to_cpu(ex->ee_block); > start_pblk = ext4_ext_pblock(ex); > @@ -1638,18 +1643,14 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > map.m_pblk = 0; > ret = ext4_map_blocks(NULL, inode, &map, 0); > > - if (ret < 0) { > - iput(inode); > - return 0; > - } > + if (ret < 0) > + goto out; > > if (ret == 0) { > /* Range is not mapped */ > path = ext4_find_extent(inode, cur, NULL, 0); > - if (IS_ERR(path)) { > - iput(inode); > - return 0; > - } > + if (IS_ERR(path)) > + goto out; > memset(&newex, 0, sizeof(newex)); > newex.ee_block = cpu_to_le32(cur); > ext4_ext_store_pblock( > @@ -1663,10 +1664,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > up_write((&EXT4_I(inode)->i_data_sem)); > ext4_ext_drop_refs(path); > kfree(path); > - if (ret) { > - iput(inode); > - return 0; > - } > + if (ret) > + goto out; > goto next; > } > > @@ -1679,10 +1678,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > ret = ext4_ext_replay_update_ex(inode, cur, map.m_len, > ext4_ext_is_unwritten(ex), > start_pblk + cur - start); > - if (ret) { > - iput(inode); > - return 0; > - } > + if (ret) > + goto out; > /* > * Mark the old blocks as free since they aren't used > * anymore. We maintain an array of all the modified > @@ -1702,10 +1699,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > ext4_ext_is_unwritten(ex), map.m_pblk); > ret = ext4_ext_replay_update_ex(inode, cur, map.m_len, > ext4_ext_is_unwritten(ex), map.m_pblk); > - if (ret) { > - iput(inode); > - return 0; > - } > + if (ret) > + goto out; > /* > * We may have split the extent tree while toggling the state. > * Try to shrink the extent tree now. > @@ -1717,6 +1712,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb, > } > ext4_ext_replay_shrink_inode(inode, i_size_read(inode) >> > sb->s_blocksize_bits); > +out: > iput(inode); > return 0; > } > @@ -1746,6 +1742,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, > } > > ret = ext4_fc_record_modified_inode(sb, inode->i_ino); > + if (ret) > + goto out; > > jbd_debug(1, "DEL_RANGE, inode %ld, lblk %d, len %d\n", > inode->i_ino, le32_to_cpu(lrange.fc_lblk), > @@ -1755,10 +1753,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, > map.m_len = remaining; > > ret = ext4_map_blocks(NULL, inode, &map, 0); > - if (ret < 0) { > - iput(inode); > - return 0; > - } > + if (ret < 0) > + goto out; > if (ret > 0) { > remaining -= ret; > cur += ret; > @@ -1773,15 +1769,13 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, > ret = ext4_ext_remove_space(inode, lrange.fc_lblk, > lrange.fc_lblk + lrange.fc_len - 1); > up_write(&EXT4_I(inode)->i_data_sem); > - if (ret) { > - iput(inode); > - return 0; > - } > + if (ret) > + goto out; > ext4_ext_replay_shrink_inode(inode, > i_size_read(inode) >> sb->s_blocksize_bits); > ext4_mark_inode_dirty(NULL, inode); > +out: > iput(inode); > - > return 0; > } > > -- > 2.31.1 >
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 5ae8026a0c56..4541c8468c01 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -1392,14 +1392,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_size += - EXT4_FC_REPLAY_REALLOC_INCREMENT; state->fc_modified_inodes = krealloc( - state->fc_modified_inodes, sizeof(int) * - state->fc_modified_inodes_size, - GFP_KERNEL); + state->fc_modified_inodes, + sizeof(int) * (state->fc_modified_inodes_size + + EXT4_FC_REPLAY_REALLOC_INCREMENT), + GFP_KERNEL); if (!state->fc_modified_inodes) return -ENOMEM; + state->fc_modified_inodes_size += + EXT4_FC_REPLAY_REALLOC_INCREMENT; } state->fc_modified_inodes[state->fc_modified_inodes_used++] = ino; return 0; @@ -1431,7 +1432,9 @@ static int ext4_fc_replay_inode(struct super_block *sb, struct ext4_fc_tl *tl, } inode = NULL; - ext4_fc_record_modified_inode(sb, ino); + ret = ext4_fc_record_modified_inode(sb, ino); + if (ret) + goto out; raw_fc_inode = (struct ext4_inode *) (val + offsetof(struct ext4_fc_inode, fc_raw_inode)); @@ -1621,6 +1624,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, } ret = ext4_fc_record_modified_inode(sb, inode->i_ino); + if (ret) + goto out; start = le32_to_cpu(ex->ee_block); start_pblk = ext4_ext_pblock(ex); @@ -1638,18 +1643,14 @@ static int ext4_fc_replay_add_range(struct super_block *sb, map.m_pblk = 0; ret = ext4_map_blocks(NULL, inode, &map, 0); - if (ret < 0) { - iput(inode); - return 0; - } + if (ret < 0) + goto out; if (ret == 0) { /* Range is not mapped */ path = ext4_find_extent(inode, cur, NULL, 0); - if (IS_ERR(path)) { - iput(inode); - return 0; - } + if (IS_ERR(path)) + goto out; memset(&newex, 0, sizeof(newex)); newex.ee_block = cpu_to_le32(cur); ext4_ext_store_pblock( @@ -1663,10 +1664,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, up_write((&EXT4_I(inode)->i_data_sem)); ext4_ext_drop_refs(path); kfree(path); - if (ret) { - iput(inode); - return 0; - } + if (ret) + goto out; goto next; } @@ -1679,10 +1678,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, ret = ext4_ext_replay_update_ex(inode, cur, map.m_len, ext4_ext_is_unwritten(ex), start_pblk + cur - start); - if (ret) { - iput(inode); - return 0; - } + if (ret) + goto out; /* * Mark the old blocks as free since they aren't used * anymore. We maintain an array of all the modified @@ -1702,10 +1699,8 @@ static int ext4_fc_replay_add_range(struct super_block *sb, ext4_ext_is_unwritten(ex), map.m_pblk); ret = ext4_ext_replay_update_ex(inode, cur, map.m_len, ext4_ext_is_unwritten(ex), map.m_pblk); - if (ret) { - iput(inode); - return 0; - } + if (ret) + goto out; /* * We may have split the extent tree while toggling the state. * Try to shrink the extent tree now. @@ -1717,6 +1712,7 @@ static int ext4_fc_replay_add_range(struct super_block *sb, } ext4_ext_replay_shrink_inode(inode, i_size_read(inode) >> sb->s_blocksize_bits); +out: iput(inode); return 0; } @@ -1746,6 +1742,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, } ret = ext4_fc_record_modified_inode(sb, inode->i_ino); + if (ret) + goto out; jbd_debug(1, "DEL_RANGE, inode %ld, lblk %d, len %d\n", inode->i_ino, le32_to_cpu(lrange.fc_lblk), @@ -1755,10 +1753,8 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, map.m_len = remaining; ret = ext4_map_blocks(NULL, inode, &map, 0); - if (ret < 0) { - iput(inode); - return 0; - } + if (ret < 0) + goto out; if (ret > 0) { remaining -= ret; cur += ret; @@ -1773,15 +1769,13 @@ ext4_fc_replay_del_range(struct super_block *sb, struct ext4_fc_tl *tl, ret = ext4_ext_remove_space(inode, lrange.fc_lblk, lrange.fc_lblk + lrange.fc_len - 1); up_write(&EXT4_I(inode)->i_data_sem); - if (ret) { - iput(inode); - return 0; - } + if (ret) + goto out; ext4_ext_replay_shrink_inode(inode, i_size_read(inode) >> sb->s_blocksize_bits); ext4_mark_inode_dirty(NULL, inode); +out: iput(inode); - return 0; }
Current code does not fully takes care of krealloc() error case, which could lead to silent memory corruption or a kernel bug. This patch fixes that. Also it cleans up some duplicated error handling logic from various functions in fast_commit.c file. Reported-by: luo penghao <luo.penghao@zte.com.cn> Suggested-by: Lukas Czerner <lczerner@redhat.com> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- fs/ext4/fast_commit.c | 64 ++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 35 deletions(-)