From: Harshad Shirwadkar <[email protected]>
Don't ignore return values of ext2fs_read/write_inode_full() in
e2fsck_rewrite_extent_tree.
Signed-off-by: Harshad Shirwadkar <[email protected]>
---
e2fsck/extents.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index 600dbc97..f48f14ff 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -290,8 +290,10 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
errcode_t err;
memset(&inode, 0, sizeof(inode));
- ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
- sizeof(inode));
+ err = ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
+ sizeof(inode));
+ if (err)
+ return err;
/* Skip deleted inodes and inline data files */
if (inode.i_flags & EXT4_INLINE_DATA_FL)
@@ -306,10 +308,8 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
if (err)
return err;
ext2fs_iblk_set(ctx->fs, EXT2_INODE(&inode), blk_count);
- ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
- sizeof(inode));
-
- return 0;
+ return ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
+ sizeof(inode));
}
errcode_t e2fsck_read_extents(e2fsck_t ctx, struct extent_list *extents)
--
2.30.0.617.g56c4b15f3c-goog
From: Harshad Shirwadkar <[email protected]>
During fast commit replay scan phase, in ext4_fc_replay_scan(), we
want to fallthrough in switch case for EXT4_FC_TAG_ADD_RANGE case. Add
a comment for that.
Signed-off-by: Harshad Shirwadkar <[email protected]>
---
e2fsck/journal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 2708942a..a67ef745 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -325,6 +325,7 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
ret = JBD2_FC_REPLAY_STOP;
else
ret = JBD2_FC_REPLAY_CONTINUE;
+ /* fallthrough */
case EXT4_FC_TAG_DEL_RANGE:
case EXT4_FC_TAG_LINK:
case EXT4_FC_TAG_UNLINK:
--
2.30.0.617.g56c4b15f3c-goog
From: Harshad Shirwadkar <[email protected]>
Don't ignore return value of ext2fs_get_array() in
ext2fs_count_blocks().
Signed-off-by: Harshad Shirwadkar <[email protected]>
---
lib/ext2fs/extent.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 1a87e68b..9e611038 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1824,8 +1824,11 @@ errcode_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino,
if (errcode)
goto out;
- ext2fs_get_array(handle->max_depth, sizeof(blk64_t),
- &intermediate_nodes);
+ errcode = ext2fs_get_array(handle->max_depth, sizeof(blk64_t),
+ &intermediate_nodes);
+ if (errcode)
+ goto out;
+
blkcount = handle->level;
while (!errcode) {
if (extent.e_flags & EXT2_EXTENT_FLAGS_LEAF) {
--
2.30.0.617.g56c4b15f3c-goog
From: Harshad Shirwadkar <[email protected]>
Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
use.
Signed-off-by: Harshad Shirwadkar <[email protected]>
---
e2fsck/journal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a67ef745..8e7ba819 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -289,7 +289,7 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
struct ext4_fc_tail *tail;
__u8 *start, *end;
struct ext4_fc_head *head;
- struct ext2fs_extent ext2fs_ex;
+ struct ext2fs_extent ext2fs_ex = {0};
state = &ctx->fc_replay_state;
--
2.30.0.617.g56c4b15f3c-goog
Hello Harshad,
ext2fs_iblk_set in the same e2fsck_rewrite_extent_tee returns a return code, but code is ignored.
Could you also add check there?
Best regards,
Artem Blagodarenko
> On 20 Feb 2021, at 00:03, Harshad Shirwadkar <[email protected]> wrote:
>
> From: Harshad Shirwadkar <[email protected]>
>
> Don't ignore return values of ext2fs_read/write_inode_full() in
> e2fsck_rewrite_extent_tree.
>
> Signed-off-by: Harshad Shirwadkar <[email protected]>
> ---
> e2fsck/extents.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index 600dbc97..f48f14ff 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -290,8 +290,10 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
> errcode_t err;
>
> memset(&inode, 0, sizeof(inode));
> - ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> - sizeof(inode));
> + err = ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> + sizeof(inode));
> + if (err)
> + return err;
>
> /* Skip deleted inodes and inline data files */
> if (inode.i_flags & EXT4_INLINE_DATA_FL)
> @@ -306,10 +308,8 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
> if (err)
> return err;
> ext2fs_iblk_set(ctx->fs, EXT2_INODE(&inode), blk_count);
> - ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> - sizeof(inode));
> -
> - return 0;
> + return ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> + sizeof(inode));
> }
>
> errcode_t e2fsck_read_extents(e2fsck_t ctx, struct extent_list *extents)
> --
> 2.30.0.617.g56c4b15f3c-goog
>
On Fri, Feb 19, 2021 at 01:03:33PM -0800, Harshad Shirwadkar wrote:
> From: Harshad Shirwadkar <[email protected]>
>
> Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
> use.
>
> Signed-off-by: Harshad Shirwadkar <[email protected]>
I wonder if we should make the following change to
ext2fs_decode_extent(), which will prevent other future bugs to
potential users of the function:
to->e_pblk = ext2fs_le32_to_cpu(from->ee_start) +
((__u64) ext2fs_le16_to_cpu(from->ee_start_hi)
<< 32);
to->e_lblk = ext2fs_le32_to_cpu(from->ee_block);
to->e_len = ext2fs_le16_to_cpu(from->ee_len);
- to->e_flags |= EXT2_EXTENT_FLAGS_LEAF;
+ to->e_flags = EXT2_EXTENT_FLAGS_LEAF;
ext2fs_decode_extent() overwrites all other members of the structure,
so we might as well just initialize e_flags as opposed to depending
the caller to initiaize *to just for the sake of to->e_flags.
Cheers,
- Ted
Thanks Artem, will fix this in V2.
- Harshad
On Sat, Feb 20, 2021 at 12:58 AM Благодаренко Артём
<[email protected]> wrote:
>
> Hello Harshad,
>
> ext2fs_iblk_set in the same e2fsck_rewrite_extent_tee returns a return code, but code is ignored.
> Could you also add check there?
>
> Best regards,
> Artem Blagodarenko
>
> > On 20 Feb 2021, at 00:03, Harshad Shirwadkar <[email protected]> wrote:
> >
> > From: Harshad Shirwadkar <[email protected]>
> >
> > Don't ignore return values of ext2fs_read/write_inode_full() in
> > e2fsck_rewrite_extent_tree.
> >
> > Signed-off-by: Harshad Shirwadkar <[email protected]>
> > ---
> > e2fsck/extents.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> > index 600dbc97..f48f14ff 100644
> > --- a/e2fsck/extents.c
> > +++ b/e2fsck/extents.c
> > @@ -290,8 +290,10 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
> > errcode_t err;
> >
> > memset(&inode, 0, sizeof(inode));
> > - ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> > - sizeof(inode));
> > + err = ext2fs_read_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> > + sizeof(inode));
> > + if (err)
> > + return err;
> >
> > /* Skip deleted inodes and inline data files */
> > if (inode.i_flags & EXT4_INLINE_DATA_FL)
> > @@ -306,10 +308,8 @@ errcode_t e2fsck_rewrite_extent_tree(e2fsck_t ctx, struct extent_list *list)
> > if (err)
> > return err;
> > ext2fs_iblk_set(ctx->fs, EXT2_INODE(&inode), blk_count);
> > - ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> > - sizeof(inode));
> > -
> > - return 0;
> > + return ext2fs_write_inode_full(ctx->fs, list->ino, EXT2_INODE(&inode),
> > + sizeof(inode));
> > }
> >
> > errcode_t e2fsck_read_extents(e2fsck_t ctx, struct extent_list *extents)
> > --
> > 2.30.0.617.g56c4b15f3c-goog
> >
>
Thanks Ted, it makes sense, will fix this in V2.
- Harshad
On Sun, Feb 21, 2021 at 3:14 PM Theodore Ts'o <[email protected]> wrote:
>
> On Fri, Feb 19, 2021 at 01:03:33PM -0800, Harshad Shirwadkar wrote:
> > From: Harshad Shirwadkar <[email protected]>
> >
> > Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
> > use.
> >
> > Signed-off-by: Harshad Shirwadkar <[email protected]>
>
> I wonder if we should make the following change to
> ext2fs_decode_extent(), which will prevent other future bugs to
> potential users of the function:
>
> to->e_pblk = ext2fs_le32_to_cpu(from->ee_start) +
> ((__u64) ext2fs_le16_to_cpu(from->ee_start_hi)
> << 32);
> to->e_lblk = ext2fs_le32_to_cpu(from->ee_block);
> to->e_len = ext2fs_le16_to_cpu(from->ee_len);
> - to->e_flags |= EXT2_EXTENT_FLAGS_LEAF;
> + to->e_flags = EXT2_EXTENT_FLAGS_LEAF;
>
> ext2fs_decode_extent() overwrites all other members of the structure,
> so we might as well just initialize e_flags as opposed to depending
> the caller to initiaize *to just for the sake of to->e_flags.
>
> Cheers,
>
> - Ted