Re: [PATCH 3/6] ext4: Fix error handling in ext4_fc_record_modified_inode()

[Jan Kara]

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@xxxxxxxxxx>
> Suggested-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> Signed-off-by: Ritesh Harjani <riteshh@xxxxxxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								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
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR