| Message ID | tencent_46F42EDFC0F504046B160E939DB76255EE08@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: propagate errors from fast commit i_blocks replay | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] The patch fails to fix error handling in the second loop of ext4_ext_replay_set_iblocks(), leading to partial block counts being applied to the inode and masked errors. -- commit 561c6a78544da466ba6d1525d42b474f54d2d473 Author: lty <781735889@qq.com> ext4: propagate errors from fast commit i_blocks replay This patch stops ext4_ext_replay_set_iblocks() from calculating partial block counts when ext4_map_blocks() fails. It leaves i_blocks unchanged and aborts inode replay on errors. > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 76038b6c36552..d67e30cf44adb 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c [ ... ] > @@ -6329,11 +6329,11 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > out: > inode->i_blocks = numblks << (inode->i_sb->s_blocksize_bits - 9); > ext4_mark_inode_dirty(NULL, inode); [Severity: High] This isn't a bug introduced by this patch, but does the second loop in ext4_ext_replay_set_iblocks() still suffer from the same issue? While this patch correctly captures errors from ext4_map_blocks() in the first loop, failures during the extent tree block counting phase still jump to the out label: while (cur < end) { path = ext4_find_extent(inode, cur, path, 0); if (IS_ERR(path)) break; ex = path[path->p_depth].p_ext; ... ret = skip_hole(inode, &cur); if (ret < 0) break; If any of these break conditions occur, we hit the out label shown in the diff context above and blindly overwrite inode->i_blocks with an incomplete numblks tally. Additionally, if ext4_find_extent() fails, we don't propagate the error code because ret might still be 0 from a previous skip_hole() success. This allows fast commit replay to silently corrupt metadata while returning success. Should the error paths in the second loop also jump to cleanup instead of out, and ensure ret is updated to PTR_ERR(path) on lookup failures? > cleanup: > ext4_free_ext_path(path); > ext4_free_ext_path(path2); > - return 0; > + return ret; > }
On Mon 14-09-26 15:16:45, lty wrote: > ext4_ext_replay_set_iblocks() breaks out of its mapping loop when > ext4_map_blocks() fails, but then continues with a partial block count > and unconditionally returns success. The fast commit replay caller also > ignores the helper return value. > > Stop the count immediately on mapping errors, leave i_blocks unchanged, > and propagate the failure to abort inode replay. > > Fixes: 8016e29f4362 ("ext4: fast commit recovery path") > > Signed-off-by: lty <781735889@qq.com> I guess this is fair but please propagate all errors from ext4_ext_replay_set_iblocks(). There are several other places in that function doing things like: path = ext4_find_extent(inode, cur, path, 0); if (IS_ERR(path)) goto out; which can return errors we should be propagating up. Honza > --- > fs/ext4/extents.c | 4 ++-- > fs/ext4/fast_commit.c | 7 +++++-- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 76038b6c3655..d67e30cf44ad 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > map.m_len = end - cur; > ret = ext4_map_blocks(NULL, inode, &map, 0); > if (ret < 0) > - break; > + goto cleanup; > if (ret > 0) > numblks += ret; > cur = cur + map.m_len; > @@ -6333,7 +6333,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) > cleanup: > ext4_free_ext_path(path); > ext4_free_ext_path(path2); > - return 0; > + return ret; > } > > int ext4_ext_clear_bb(struct inode *inode) > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index 0cac890cf370..8110b01e36c1 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb, > * crashing. This should be fixed but until then, we calculate > * the number of blocks the inode. > */ > - if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) > - ext4_ext_replay_set_iblocks(inode); > + if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) { > + ret = ext4_ext_replay_set_iblocks(inode); > + if (ret) > + goto out_brelse; > + } > > inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation); > ext4_reset_inode_seed(inode); > > base-commit: 704340f1cd0dcef829eb62f5b48ae95a2ce17bdf > -- > 2.34.1 >
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 76038b6c3655..d67e30cf44ad 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -6275,7 +6275,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) map.m_len = end - cur; ret = ext4_map_blocks(NULL, inode, &map, 0); if (ret < 0) - break; + goto cleanup; if (ret > 0) numblks += ret; cur = cur + map.m_len; @@ -6333,7 +6333,7 @@ int ext4_ext_replay_set_iblocks(struct inode *inode) cleanup: ext4_free_ext_path(path); ext4_free_ext_path(path2); - return 0; + return ret; } int ext4_ext_clear_bb(struct inode *inode) diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c index 0cac890cf370..8110b01e36c1 100644 --- a/fs/ext4/fast_commit.c +++ b/fs/ext4/fast_commit.c @@ -2002,8 +2002,11 @@ static int ext4_fc_replay_inode(struct super_block *sb, * crashing. This should be fixed but until then, we calculate * the number of blocks the inode. */ - if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) - ext4_ext_replay_set_iblocks(inode); + if (!ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) { + ret = ext4_ext_replay_set_iblocks(inode); + if (ret) + goto out_brelse; + } inode->i_generation = le32_to_cpu(ext4_raw_inode(&iloc)->i_generation); ext4_reset_inode_seed(inode);
ext4_ext_replay_set_iblocks() breaks out of its mapping loop when ext4_map_blocks() fails, but then continues with a partial block count and unconditionally returns success. The fast commit replay caller also ignores the helper return value. Stop the count immediately on mapping errors, leave i_blocks unchanged, and propagate the failure to abort inode replay. Fixes: 8016e29f4362 ("ext4: fast commit recovery path") Signed-off-by: lty <781735889@qq.com> --- fs/ext4/extents.c | 4 ++-- fs/ext4/fast_commit.c | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) base-commit: 704340f1cd0dcef829eb62f5b48ae95a2ce17bdf