[PATCH 1/4] e2fsck: don't ignore return values in e2fsck_rewrite_extent_tree

[PATCH 1/4] e2fsck: don't ignore return values in e2fsck_rewrite_extent_tree

[Harshad Shirwadkar]

From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Don't ignore return values of ext2fs_read/write_inode_full() in
e2fsck_rewrite_extent_tree.

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 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

[PATCH 2/4] ext2fs: don't ignore return value in ext2fs_count_blocks

[Harshad Shirwadkar]

From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Don't ignore return value of ext2fs_get_array() in
ext2fs_count_blocks().

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 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

[PATCH 3/4] e2fsck: add fallthrough comment in fc replay switch case

[Harshad Shirwadkar]

From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

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 <harshadshirwadkar@gmail.com>
---
 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

[PATCH 4/4] e2fsck: initialize variable before first use in fast commit replay

[Harshad Shirwadkar]

From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
use.

Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
---
 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

Re: [PATCH 1/4] e2fsck: don't ignore return values in e2fsck_rewrite_extent_tree

[Благодаренко Артём]

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 <harshadshirwadkar@gmail.com> wrote:
> 
> From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> 
> Don't ignore return values of ext2fs_read/write_inode_full() in
> e2fsck_rewrite_extent_tree.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> ---
> 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
> 

Re: [PATCH 4/4] e2fsck: initialize variable before first use in fast commit replay

[Theodore Ts'o]

On Fri, Feb 19, 2021 at 01:03:33PM -0800, Harshad Shirwadkar wrote:
> From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> 
> Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
> use.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

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

Re: [PATCH 1/4] e2fsck: don't ignore return values in e2fsck_rewrite_extent_tree

[harshad shirwadkar]

Thanks Artem, will fix this in V2.

- Harshad

On Sat, Feb 20, 2021 at 12:58 AM Благодаренко Артём
<artem.blagodarenko@gmail.com> 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 <harshadshirwadkar@gmail.com> wrote:
> >
> > From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> >
> > Don't ignore return values of ext2fs_read/write_inode_full() in
> > e2fsck_rewrite_extent_tree.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> > ---
> > 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
> >
>

Re: [PATCH 4/4] e2fsck: initialize variable before first use in fast commit replay

[harshad shirwadkar]

Thanks Ted, it makes sense, will fix this in V2.

- Harshad

On Sun, Feb 21, 2021 at 3:14 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Feb 19, 2021 at 01:03:33PM -0800, Harshad Shirwadkar wrote:
> > From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> >
> > Initialize ext2fs_ex variable in ext4_fc_replay_scan() before first
> > use.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
>
> 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