diff mbox series

[-v2] e2fsck: fix portability problems caused by unaligned accesses

Message ID 20210506183017.208802-1-tytso@mit.edu
State Accepted
Headers show
Series [-v2] e2fsck: fix portability problems caused by unaligned accesses | expand

Commit Message

Theodore Ts'o May 6, 2021, 6:30 p.m. UTC 
The on-disk format for the ext4 journal can have unaigned 32-bit
integers.  This can happen when replaying a journal using a obsolete
checksum format (which was never popularly used, since the v3 format
replaced v2 while the metadata checksum feature was being stablized),
and in the fast commit feature (which landed in the 5.10 kernel,
although it is not enabled by default).

This commit fixes the following regression tests on some platforms
(such as running 32-bit arm architectures on a 64-bit arm kernel):
j_recover_csum2_32bit, j_recover_csum2_64bit, j_recover_fast_commit.

https://github.com/tytso/e2fsprogs/issues/65

Addresses-Debian-Bug: #987641
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---

Changes from v1:
  - Copy the structures to assure alignment instead of using a
    byte-by-byte access because taking a pointer of a structure
    member since that could trigger UBSAN complaints.

 e2fsck/journal.c                   | 83 ++++++++++++++++--------------
 e2fsck/recovery.c                  | 22 ++++----
 tests/j_recover_fast_commit/script |  1 -
 3 files changed, 56 insertions(+), 50 deletions(-)

Comments

harshad shirwadkar May 6, 2021, 11:30 p.m. UTC | #1 
Hi Ted,

Thanks for working on this. The patch looks good, but there are some
more places where we need to apply the fix for alignment. I am working
on creating another patch on top of this patch to handle that. So,
I'll send out your patch + extra patch that fixes remaining alignment
issues soon. I have inlined places that also need memcpy fixes:

On Thu, May 6, 2021 at 11:30 AM Theodore Ts'o <tytso@mit.edu> wrote:
>
> The on-disk format for the ext4 journal can have unaigned 32-bit
> integers.  This can happen when replaying a journal using a obsolete
> checksum format (which was never popularly used, since the v3 format
> replaced v2 while the metadata checksum feature was being stablized),
> and in the fast commit feature (which landed in the 5.10 kernel,
> although it is not enabled by default).
>
> This commit fixes the following regression tests on some platforms
> (such as running 32-bit arm architectures on a 64-bit arm kernel):
> j_recover_csum2_32bit, j_recover_csum2_64bit, j_recover_fast_commit.
>
> https://github.com/tytso/e2fsprogs/issues/65
>
> Addresses-Debian-Bug: #987641
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>
> Changes from v1:
>   - Copy the structures to assure alignment instead of using a
>     byte-by-byte access because taking a pointer of a structure
>     member since that could trigger UBSAN complaints.
>
>  e2fsck/journal.c                   | 83 ++++++++++++++++--------------
>  e2fsck/recovery.c                  | 22 ++++----
>  tests/j_recover_fast_commit/script |  1 -
>  3 files changed, 56 insertions(+), 50 deletions(-)
>
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index a425bbd1..bd0e4f31 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -286,9 +286,9 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
>         int ret = JBD2_FC_REPLAY_CONTINUE;
>         struct ext4_fc_add_range *ext;
>         struct ext4_fc_tl *tl;
> -       struct ext4_fc_tail *tail;
> +       struct ext4_fc_tail tail;
>         __u8 *start, *end;
> -       struct ext4_fc_head *head;
> +       struct ext4_fc_head head;
>         struct ext2fs_extent ext2fs_ex = {0};
>
>         state = &ctx->fc_replay_state;
> @@ -338,16 +338,15 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
>                         break;
>                 case EXT4_FC_TAG_TAIL:
>                         state->fc_cur_tag++;
> -                       tail = (struct ext4_fc_tail *)ext4_fc_tag_val(tl);
> +                       memcpy(&tail, ext4_fc_tag_val(tl), sizeof(tail));
>                         state->fc_crc = jbd2_chksum(j, state->fc_crc, tl,
>                                                 sizeof(*tl) +
>                                                 offsetof(struct ext4_fc_tail,
>                                                 fc_crc));
>                         jbd_debug(1, "tail tid %d, expected %d\n",
> -                                       le32_to_cpu(tail->fc_tid),
> -                                       expected_tid);
> -                       if (le32_to_cpu(tail->fc_tid) == expected_tid &&
> -                               le32_to_cpu(tail->fc_crc) == state->fc_crc) {
> +                                 le32_to_cpu(tail.fc_tid), expected_tid);
> +                       if (le32_to_cpu(tail.fc_tid) == expected_tid &&
> +                           le32_to_cpu(tail.fc_crc) == state->fc_crc) {
>                                 state->fc_replay_num_tags = state->fc_cur_tag;
>                         } else {
>                                 ret = state->fc_replay_num_tags ?
> @@ -356,13 +355,13 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
>                         state->fc_crc = 0;
>                         break;
>                 case EXT4_FC_TAG_HEAD:
> -                       head = (struct ext4_fc_head *)ext4_fc_tag_val(tl);
> -                       if (le32_to_cpu(head->fc_features) &
> -                               ~EXT4_FC_SUPPORTED_FEATURES) {
> +                       memcpy(&head, ext4_fc_tag_val(tl), sizeof(head));
> +                       if (le32_to_cpu(head.fc_features) &
> +                           ~EXT4_FC_SUPPORTED_FEATURES) {
>                                 ret = -EOPNOTSUPP;
>                                 break;
>                         }
> -                       if (le32_to_cpu(head->fc_tid) != expected_tid) {
> +                       if (le32_to_cpu(head.fc_tid) != expected_tid) {
>                                 ret = -EINVAL;
>                                 break;
>                         }
> @@ -612,27 +611,31 @@ struct dentry_info_args {
>         char *dname;
>  };
>
> -static inline void tl_to_darg(struct dentry_info_args *darg,
> +static inline int tl_to_darg(struct dentry_info_args *darg,
>                                 struct  ext4_fc_tl *tl)
>  {
> -       struct ext4_fc_dentry_info *fcd;
> +       struct ext4_fc_dentry_info fcd;
>         int tag = le16_to_cpu(tl->fc_tag);
The above line where we dereference tl, this can also result in
unaligned accesses. So, we need to do memcpy stuff for "tl" too.
Changing all access of tl to a memcpy-ed local variable is itself a
big change which I'll send along with your patch.

>
> -       fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl);
> +       memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));

If we do the memcpy fix here, ext4_fc_tag_val macro becomes unusable -
since at this point that macro just does (tl + 1), which will fail on
a memcpy-ed version of "tl".

Interesting bit is that even the kernel does these kinds of accesses
in the recovery code. I have a suspicion that these unaligned accesses
are the reason why you see failures on sparc?

Thanks,
Harshad
>
> -       darg->parent_ino = le32_to_cpu(fcd->fc_parent_ino);
> -       darg->ino = le32_to_cpu(fcd->fc_ino);
> -       darg->dname = (char *) fcd->fc_dname;
> +       darg->parent_ino = le32_to_cpu(fcd.fc_parent_ino);
> +       darg->ino = le32_to_cpu(fcd.fc_ino);
>         darg->dname_len = ext4_fc_tag_len(tl) -
>                         sizeof(struct ext4_fc_dentry_info);
>         darg->dname = malloc(darg->dname_len + 1);
> -       memcpy(darg->dname, fcd->fc_dname, darg->dname_len);
> +       if (!darg->dname)
> +               return -ENOMEM;
> +       memcpy(darg->dname,
> +              ext4_fc_tag_val(tl) + sizeof(struct ext4_fc_dentry_info),
> +              darg->dname_len);
>         darg->dname[darg->dname_len] = 0;
>         jbd_debug(1, "%s: %s, ino %d, parent %d\n",
>                 tag == EXT4_FC_TAG_CREAT ? "create" :
>                 (tag == EXT4_FC_TAG_LINK ? "link" :
>                 (tag == EXT4_FC_TAG_UNLINK ? "unlink" : "error")),
>                 darg->dname, darg->ino, darg->parent_ino);
> +       return 0;
>  }
>
>  static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
> @@ -642,7 +645,9 @@ static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
>         ext2_filsys fs = ctx->fs;
>         int ret;
>
> -       tl_to_darg(&darg, tl);
> +       ret = tl_to_darg(&darg, tl);
> +       if (ret)
> +               return ret;
>         ext4_fc_flush_extents(ctx, darg.ino);
>         ret = errcode_to_errno(
>                        ext2fs_unlink(ctx->fs, darg.parent_ino,
> @@ -659,7 +664,9 @@ static int ext4_fc_handle_link_and_create(e2fsck_t ctx, struct ext4_fc_tl *tl)
>         struct ext2_inode_large inode_large;
>         int ret, filetype, mode;
>
> -       tl_to_darg(&darg, tl);
> +       ret = tl_to_darg(&darg, tl);
> +       if (ret)
> +               return ret;
>         ext4_fc_flush_extents(ctx, 0);
>         ret = errcode_to_errno(ext2fs_read_inode(fs, darg.ino,
>                                                  (struct ext2_inode *)&inode_large));
> @@ -730,17 +737,18 @@ static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
>         struct e2fsck_fc_replay_state *state = &ctx->fc_replay_state;
>         int ino, inode_len = EXT2_GOOD_OLD_INODE_SIZE;
>         struct ext2_inode_large *inode = NULL, *fc_inode = NULL;
> -       struct ext4_fc_inode *fc_inode_val;
> +       __le32 fc_ino;
> +       __u8 *fc_raw_inode;
>         errcode_t err;
>         blk64_t blks;
>
> -       fc_inode_val = (struct ext4_fc_inode *)ext4_fc_tag_val(tl);
> -       ino = le32_to_cpu(fc_inode_val->fc_ino);
> +       memcpy(&fc_ino, ext4_fc_tag_val(tl), sizeof(fc_ino));
> +       fc_raw_inode = ext4_fc_tag_val(tl) + sizeof(fc_ino);
> +       ino = le32_to_cpu(fc_ino);
>
>         if (EXT2_INODE_SIZE(ctx->fs->super) > EXT2_GOOD_OLD_INODE_SIZE)
>                 inode_len += ext2fs_le16_to_cpu(
> -                       ((struct ext2_inode_large *)fc_inode_val->fc_raw_inode)
> -                               ->i_extra_isize);
> +                       ((struct ext2_inode_large *)fc_raw_inode)->i_extra_isize);
>         err = ext2fs_get_mem(inode_len, &inode);
>         if (err)
>                 goto out;
> @@ -755,10 +763,10 @@ static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
>                 goto out;
>  #ifdef WORDS_BIGENDIAN
>         ext2fs_swap_inode_full(ctx->fs, fc_inode,
> -                              (struct ext2_inode_large *)fc_inode_val->fc_raw_inode,
> +                              (struct ext2_inode_large *)fc_raw_inode,
>                                0, sizeof(*inode));
>  #else
> -       memcpy(fc_inode, fc_inode_val->fc_raw_inode, inode_len);
> +       memcpy(fc_inode, fc_raw_inode, inode_len);
>  #endif
>         memcpy(inode, fc_inode, offsetof(struct ext2_inode_large, i_block));
>         memcpy(&inode->i_generation, &fc_inode->i_generation,
> @@ -792,12 +800,11 @@ out:
>  static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
>  {
>         struct ext2fs_extent extent;
> -       struct ext4_fc_add_range *add_range;
> -       struct ext4_fc_del_range *del_range;
> +       struct ext4_fc_add_range add_range;
>         int ret = 0, ino;
>
> -       add_range = (struct ext4_fc_add_range *)ext4_fc_tag_val(tl);
> -       ino = le32_to_cpu(add_range->fc_ino);
> +       memcpy(&add_range, ext4_fc_tag_val(tl), sizeof(add_range));
> +       ino = le32_to_cpu(add_range.fc_ino);
>         ext4_fc_flush_extents(ctx, ino);
>
>         ret = ext4_fc_read_extents(ctx, ino);
> @@ -805,8 +812,8 @@ static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
>                 return ret;
>         memset(&extent, 0, sizeof(extent));
>         ret = errcode_to_errno(ext2fs_decode_extent(
> -                       &extent, (void *)(add_range->fc_ex),
> -                       sizeof(add_range->fc_ex)));
> +                       &extent, (void *)add_range.fc_ex,
> +                       sizeof(add_range.fc_ex)));
>         if (ret)
>                 return ret;
>         return ext4_add_extent_to_list(ctx,
> @@ -819,16 +826,16 @@ static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
>  static int ext4_fc_handle_del_range(e2fsck_t ctx, struct ext4_fc_tl *tl)
>  {
>         struct ext2fs_extent extent;
> -       struct ext4_fc_del_range *del_range;
> +       struct ext4_fc_del_range del_range;
>         int ret, ino;
>
> -       del_range = (struct ext4_fc_del_range *)ext4_fc_tag_val(tl);
> -       ino = le32_to_cpu(del_range->fc_ino);
> +       memcpy(&del_range, ext4_fc_tag_val(tl), sizeof(del_range));
> +       ino = le32_to_cpu(del_range.fc_ino);
>         ext4_fc_flush_extents(ctx, ino);
>
>         memset(&extent, 0, sizeof(extent));
> -       extent.e_lblk = ext2fs_le32_to_cpu(del_range->fc_lblk);
> -       extent.e_len = ext2fs_le32_to_cpu(del_range->fc_len);
> +       extent.e_lblk = le32_to_cpu(del_range.fc_lblk);
> +       extent.e_len = le32_to_cpu(del_range.fc_len);
>         ret = ext4_fc_read_extents(ctx, ino);
>         if (ret)
>                 return ret;
> diff --git a/e2fsck/recovery.c b/e2fsck/recovery.c
> index dc0694fc..02694d2c 100644
> --- a/e2fsck/recovery.c
> +++ b/e2fsck/recovery.c
> @@ -196,7 +196,7 @@ static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
>  static int count_tags(journal_t *journal, struct buffer_head *bh)
>  {
>         char *                  tagp;
> -       journal_block_tag_t *   tag;
> +       journal_block_tag_t     tag;
>         int                     nr = 0, size = journal->j_blocksize;
>         int                     tag_bytes = journal_tag_bytes(journal);
>
> @@ -206,14 +206,14 @@ static int count_tags(journal_t *journal, struct buffer_head *bh)
>         tagp = &bh->b_data[sizeof(journal_header_t)];
>
>         while ((tagp - bh->b_data + tag_bytes) <= size) {
> -               tag = (journal_block_tag_t *) tagp;
> +               memcpy(&tag, tagp, sizeof(tag));
>
>                 nr++;
>                 tagp += tag_bytes;
> -               if (!(tag->t_flags & cpu_to_be16(JBD2_FLAG_SAME_UUID)))
> +               if (!(tag.t_flags & cpu_to_be16(JBD2_FLAG_SAME_UUID)))
>                         tagp += 16;
>
> -               if (tag->t_flags & cpu_to_be16(JBD2_FLAG_LAST_TAG))
> +               if (tag.t_flags & cpu_to_be16(JBD2_FLAG_LAST_TAG))
>                         break;
>         }
>
> @@ -434,9 +434,9 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
>  }
>
>  static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
> +                                     journal_block_tag3_t *tag3,
>                                       void *buf, __u32 sequence)
>  {
> -       journal_block_tag3_t *tag3 = (journal_block_tag3_t *)tag;
>         __u32 csum32;
>         __be32 seq;
>
> @@ -497,7 +497,7 @@ static int do_one_pass(journal_t *journal,
>         while (1) {
>                 int                     flags;
>                 char *                  tagp;
> -               journal_block_tag_t *   tag;
> +               journal_block_tag_t     tag;
>                 struct buffer_head *    obh;
>                 struct buffer_head *    nbh;
>
> @@ -614,8 +614,8 @@ static int do_one_pass(journal_t *journal,
>                                <= journal->j_blocksize - descr_csum_size) {
>                                 unsigned long io_block;
>
> -                               tag = (journal_block_tag_t *) tagp;
> -                               flags = be16_to_cpu(tag->t_flags);
> +                               memcpy(&tag, tagp, sizeof(tag));
> +                               flags = be16_to_cpu(tag.t_flags);
>
>                                 io_block = next_log_block++;
>                                 wrap(journal, next_log_block);
> @@ -633,7 +633,7 @@ static int do_one_pass(journal_t *journal,
>
>                                         J_ASSERT(obh != NULL);
>                                         blocknr = read_tag_block(journal,
> -                                                                tag);
> +                                                                &tag);
>
>                                         /* If the block has been
>                                          * revoked, then we're all done
> @@ -648,8 +648,8 @@ static int do_one_pass(journal_t *journal,
>
>                                         /* Look for block corruption */
>                                         if (!jbd2_block_tag_csum_verify(
> -                                               journal, tag, obh->b_data,
> -                                               be32_to_cpu(tmp->h_sequence))) {
> +                       journal, &tag, (journal_block_tag3_t *)tagp,
> +                       obh->b_data, be32_to_cpu(tmp->h_sequence))) {
>                                                 brelse(obh);
>                                                 success = -EFSBADCRC;
>                                                 printk(KERN_ERR "JBD2: Invalid "
> diff --git a/tests/j_recover_fast_commit/script b/tests/j_recover_fast_commit/script
> index 22ef6325..05c40cc5 100755
> --- a/tests/j_recover_fast_commit/script
> +++ b/tests/j_recover_fast_commit/script
> @@ -10,7 +10,6 @@ gunzip < $IMAGE > $TMPFILE
>  EXP=$test_dir/expect
>  OUT=$test_name.log
>
> -cp $TMPFILE /tmp/debugthis
>  $FSCK $FSCK_OPT -E journal_only -N test_filesys $TMPFILE 2>/dev/null | head -n 1000 | tail -n +2 > $OUT
>  echo "Exit status is $?" >> $OUT
>
> --
> 2.31.0
>
Theodore Ts'o May 7, 2021, 1:50 a.m. UTC | #2 
On Thu, May 06, 2021 at 04:30:39PM -0700, harshad shirwadkar wrote:
> > -static inline void tl_to_darg(struct dentry_info_args *darg,
> > +static inline int tl_to_darg(struct dentry_info_args *darg,
> >                                 struct  ext4_fc_tl *tl)
> >  {
> > -       struct ext4_fc_dentry_info *fcd;
> > +       struct ext4_fc_dentry_info fcd;
> >         int tag = le16_to_cpu(tl->fc_tag);
> The above line where we dereference tl, this can also result in
> unaligned accesses. So, we need to do memcpy stuff for "tl" too.
> Changing all access of tl to a memcpy-ed local variable is itself a
> big change which I'll send along with your patch.

Ah, I didn't realize that 16-bit shorts could be misaligned.  With the
jbd2 checksum v2, that wasn't an issue, since the entries were always
an even number of bytes, so it was only the 32-bit accesses that were
problematic.  But yeah, if the dentry is an odd number of bytes, we're
not padding that out.

> >
> > -       fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl);
> > +       memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));
> 
> If we do the memcpy fix here, ext4_fc_tag_val macro becomes unusable -
> since at this point that macro just does (tl + 1), which will fail on
> a memcpy-ed version of "tl".

Well, we can make define them as:

/* Get length of a particular tlv */
static inline int ext4_fc_tag_len(struct ext4_fc_tl *tl)
{
	__u8 *p = (__u8 *) tl;
	
	return *cp + (*(cp+1) << 8);
}

/* Get a pointer to "value" of a tlv */
static inline __u8 *ext4_fc_tag_val(struct ext4_fc_tl *tl)
{
	__u8 *p = ((__u8 *) tl) + 2;

	return *cp + (*(cp+1) << 8);
}

> Interesting bit is that even the kernel does these kinds of accesses
> in the recovery code. I have a suspicion that these unaligned accesses
> are the reason why you see failures on sparc?

Yeah, it could be that arm allows unaligned 16-bit dereferences, which
is why this isn't blowing up on armhf and armel.

But at least with this patch, armhf and armel builds aren't blowing
up, and UBSAN is happy.  (Although I wonder why UBSAN isn't
complaining about the unaligned 16-bit dereferences.)

					- Ted
diff mbox series

Patch

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a425bbd1..bd0e4f31 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -286,9 +286,9 @@  static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
 	int ret = JBD2_FC_REPLAY_CONTINUE;
 	struct ext4_fc_add_range *ext;
 	struct ext4_fc_tl *tl;
-	struct ext4_fc_tail *tail;
+	struct ext4_fc_tail tail;
 	__u8 *start, *end;
-	struct ext4_fc_head *head;
+	struct ext4_fc_head head;
 	struct ext2fs_extent ext2fs_ex = {0};
 
 	state = &ctx->fc_replay_state;
@@ -338,16 +338,15 @@  static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
 			break;
 		case EXT4_FC_TAG_TAIL:
 			state->fc_cur_tag++;
-			tail = (struct ext4_fc_tail *)ext4_fc_tag_val(tl);
+			memcpy(&tail, ext4_fc_tag_val(tl), sizeof(tail));
 			state->fc_crc = jbd2_chksum(j, state->fc_crc, tl,
 						sizeof(*tl) +
 						offsetof(struct ext4_fc_tail,
 						fc_crc));
 			jbd_debug(1, "tail tid %d, expected %d\n",
-					le32_to_cpu(tail->fc_tid),
-					expected_tid);
-			if (le32_to_cpu(tail->fc_tid) == expected_tid &&
-				le32_to_cpu(tail->fc_crc) == state->fc_crc) {
+				  le32_to_cpu(tail.fc_tid), expected_tid);
+			if (le32_to_cpu(tail.fc_tid) == expected_tid &&
+			    le32_to_cpu(tail.fc_crc) == state->fc_crc) {
 				state->fc_replay_num_tags = state->fc_cur_tag;
 			} else {
 				ret = state->fc_replay_num_tags ?
@@ -356,13 +355,13 @@  static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
 			state->fc_crc = 0;
 			break;
 		case EXT4_FC_TAG_HEAD:
-			head = (struct ext4_fc_head *)ext4_fc_tag_val(tl);
-			if (le32_to_cpu(head->fc_features) &
-				~EXT4_FC_SUPPORTED_FEATURES) {
+			memcpy(&head, ext4_fc_tag_val(tl), sizeof(head));
+			if (le32_to_cpu(head.fc_features) &
+			    ~EXT4_FC_SUPPORTED_FEATURES) {
 				ret = -EOPNOTSUPP;
 				break;
 			}
-			if (le32_to_cpu(head->fc_tid) != expected_tid) {
+			if (le32_to_cpu(head.fc_tid) != expected_tid) {
 				ret = -EINVAL;
 				break;
 			}
@@ -612,27 +611,31 @@  struct dentry_info_args {
 	char *dname;
 };
 
-static inline void tl_to_darg(struct dentry_info_args *darg,
+static inline int tl_to_darg(struct dentry_info_args *darg,
 				struct  ext4_fc_tl *tl)
 {
-	struct ext4_fc_dentry_info *fcd;
+	struct ext4_fc_dentry_info fcd;
 	int tag = le16_to_cpu(tl->fc_tag);
 
-	fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl);
+	memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));
 
-	darg->parent_ino = le32_to_cpu(fcd->fc_parent_ino);
-	darg->ino = le32_to_cpu(fcd->fc_ino);
-	darg->dname = (char *) fcd->fc_dname;
+	darg->parent_ino = le32_to_cpu(fcd.fc_parent_ino);
+	darg->ino = le32_to_cpu(fcd.fc_ino);
 	darg->dname_len = ext4_fc_tag_len(tl) -
 			sizeof(struct ext4_fc_dentry_info);
 	darg->dname = malloc(darg->dname_len + 1);
-	memcpy(darg->dname, fcd->fc_dname, darg->dname_len);
+	if (!darg->dname)
+		return -ENOMEM;
+	memcpy(darg->dname,
+	       ext4_fc_tag_val(tl) + sizeof(struct ext4_fc_dentry_info),
+	       darg->dname_len);
 	darg->dname[darg->dname_len] = 0;
 	jbd_debug(1, "%s: %s, ino %d, parent %d\n",
 		tag == EXT4_FC_TAG_CREAT ? "create" :
 		(tag == EXT4_FC_TAG_LINK ? "link" :
 		(tag == EXT4_FC_TAG_UNLINK ? "unlink" : "error")),
 		darg->dname, darg->ino, darg->parent_ino);
+	return 0;
 }
 
 static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
@@ -642,7 +645,9 @@  static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
 	ext2_filsys fs = ctx->fs;
 	int ret;
 
-	tl_to_darg(&darg, tl);
+	ret = tl_to_darg(&darg, tl);
+	if (ret)
+		return ret;
 	ext4_fc_flush_extents(ctx, darg.ino);
 	ret = errcode_to_errno(
 		       ext2fs_unlink(ctx->fs, darg.parent_ino,
@@ -659,7 +664,9 @@  static int ext4_fc_handle_link_and_create(e2fsck_t ctx, struct ext4_fc_tl *tl)
 	struct ext2_inode_large inode_large;
 	int ret, filetype, mode;
 
-	tl_to_darg(&darg, tl);
+	ret = tl_to_darg(&darg, tl);
+	if (ret)
+		return ret;
 	ext4_fc_flush_extents(ctx, 0);
 	ret = errcode_to_errno(ext2fs_read_inode(fs, darg.ino,
 						 (struct ext2_inode *)&inode_large));
@@ -730,17 +737,18 @@  static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
 	struct e2fsck_fc_replay_state *state = &ctx->fc_replay_state;
 	int ino, inode_len = EXT2_GOOD_OLD_INODE_SIZE;
 	struct ext2_inode_large *inode = NULL, *fc_inode = NULL;
-	struct ext4_fc_inode *fc_inode_val;
+	__le32 fc_ino;
+	__u8 *fc_raw_inode;
 	errcode_t err;
 	blk64_t blks;
 
-	fc_inode_val = (struct ext4_fc_inode *)ext4_fc_tag_val(tl);
-	ino = le32_to_cpu(fc_inode_val->fc_ino);
+	memcpy(&fc_ino, ext4_fc_tag_val(tl), sizeof(fc_ino));
+	fc_raw_inode = ext4_fc_tag_val(tl) + sizeof(fc_ino);
+	ino = le32_to_cpu(fc_ino);
 
 	if (EXT2_INODE_SIZE(ctx->fs->super) > EXT2_GOOD_OLD_INODE_SIZE)
 		inode_len += ext2fs_le16_to_cpu(
-			((struct ext2_inode_large *)fc_inode_val->fc_raw_inode)
-				->i_extra_isize);
+			((struct ext2_inode_large *)fc_raw_inode)->i_extra_isize);
 	err = ext2fs_get_mem(inode_len, &inode);
 	if (err)
 		goto out;
@@ -755,10 +763,10 @@  static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
 		goto out;
 #ifdef WORDS_BIGENDIAN
 	ext2fs_swap_inode_full(ctx->fs, fc_inode,
-			       (struct ext2_inode_large *)fc_inode_val->fc_raw_inode,
+			       (struct ext2_inode_large *)fc_raw_inode,
 			       0, sizeof(*inode));
 #else
-	memcpy(fc_inode, fc_inode_val->fc_raw_inode, inode_len);
+	memcpy(fc_inode, fc_raw_inode, inode_len);
 #endif
 	memcpy(inode, fc_inode, offsetof(struct ext2_inode_large, i_block));
 	memcpy(&inode->i_generation, &fc_inode->i_generation,
@@ -792,12 +800,11 @@  out:
 static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
 {
 	struct ext2fs_extent extent;
-	struct ext4_fc_add_range *add_range;
-	struct ext4_fc_del_range *del_range;
+	struct ext4_fc_add_range add_range;
 	int ret = 0, ino;
 
-	add_range = (struct ext4_fc_add_range *)ext4_fc_tag_val(tl);
-	ino = le32_to_cpu(add_range->fc_ino);
+	memcpy(&add_range, ext4_fc_tag_val(tl), sizeof(add_range));
+	ino = le32_to_cpu(add_range.fc_ino);
 	ext4_fc_flush_extents(ctx, ino);
 
 	ret = ext4_fc_read_extents(ctx, ino);
@@ -805,8 +812,8 @@  static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
 		return ret;
 	memset(&extent, 0, sizeof(extent));
 	ret = errcode_to_errno(ext2fs_decode_extent(
-			&extent, (void *)(add_range->fc_ex),
-			sizeof(add_range->fc_ex)));
+			&extent, (void *)add_range.fc_ex,
+			sizeof(add_range.fc_ex)));
 	if (ret)
 		return ret;
 	return ext4_add_extent_to_list(ctx,
@@ -819,16 +826,16 @@  static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
 static int ext4_fc_handle_del_range(e2fsck_t ctx, struct ext4_fc_tl *tl)
 {
 	struct ext2fs_extent extent;
-	struct ext4_fc_del_range *del_range;
+	struct ext4_fc_del_range del_range;
 	int ret, ino;
 
-	del_range = (struct ext4_fc_del_range *)ext4_fc_tag_val(tl);
-	ino = le32_to_cpu(del_range->fc_ino);
+	memcpy(&del_range, ext4_fc_tag_val(tl), sizeof(del_range));
+	ino = le32_to_cpu(del_range.fc_ino);
 	ext4_fc_flush_extents(ctx, ino);
 
 	memset(&extent, 0, sizeof(extent));
-	extent.e_lblk = ext2fs_le32_to_cpu(del_range->fc_lblk);
-	extent.e_len = ext2fs_le32_to_cpu(del_range->fc_len);
+	extent.e_lblk = le32_to_cpu(del_range.fc_lblk);
+	extent.e_len = le32_to_cpu(del_range.fc_len);
 	ret = ext4_fc_read_extents(ctx, ino);
 	if (ret)
 		return ret;
diff --git a/e2fsck/recovery.c b/e2fsck/recovery.c
index dc0694fc..02694d2c 100644
--- a/e2fsck/recovery.c
+++ b/e2fsck/recovery.c
@@ -196,7 +196,7 @@  static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
 static int count_tags(journal_t *journal, struct buffer_head *bh)
 {
 	char *			tagp;
-	journal_block_tag_t *	tag;
+	journal_block_tag_t	tag;
 	int			nr = 0, size = journal->j_blocksize;
 	int			tag_bytes = journal_tag_bytes(journal);
 
@@ -206,14 +206,14 @@  static int count_tags(journal_t *journal, struct buffer_head *bh)
 	tagp = &bh->b_data[sizeof(journal_header_t)];
 
 	while ((tagp - bh->b_data + tag_bytes) <= size) {
-		tag = (journal_block_tag_t *) tagp;
+		memcpy(&tag, tagp, sizeof(tag));
 
 		nr++;
 		tagp += tag_bytes;
-		if (!(tag->t_flags & cpu_to_be16(JBD2_FLAG_SAME_UUID)))
+		if (!(tag.t_flags & cpu_to_be16(JBD2_FLAG_SAME_UUID)))
 			tagp += 16;
 
-		if (tag->t_flags & cpu_to_be16(JBD2_FLAG_LAST_TAG))
+		if (tag.t_flags & cpu_to_be16(JBD2_FLAG_LAST_TAG))
 			break;
 	}
 
@@ -434,9 +434,9 @@  static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
 }
 
 static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
+				      journal_block_tag3_t *tag3,
 				      void *buf, __u32 sequence)
 {
-	journal_block_tag3_t *tag3 = (journal_block_tag3_t *)tag;
 	__u32 csum32;
 	__be32 seq;
 
@@ -497,7 +497,7 @@  static int do_one_pass(journal_t *journal,
 	while (1) {
 		int			flags;
 		char *			tagp;
-		journal_block_tag_t *	tag;
+		journal_block_tag_t	tag;
 		struct buffer_head *	obh;
 		struct buffer_head *	nbh;
 
@@ -614,8 +614,8 @@  static int do_one_pass(journal_t *journal,
 			       <= journal->j_blocksize - descr_csum_size) {
 				unsigned long io_block;
 
-				tag = (journal_block_tag_t *) tagp;
-				flags = be16_to_cpu(tag->t_flags);
+				memcpy(&tag, tagp, sizeof(tag));
+				flags = be16_to_cpu(tag.t_flags);
 
 				io_block = next_log_block++;
 				wrap(journal, next_log_block);
@@ -633,7 +633,7 @@  static int do_one_pass(journal_t *journal,
 
 					J_ASSERT(obh != NULL);
 					blocknr = read_tag_block(journal,
-								 tag);
+								 &tag);
 
 					/* If the block has been
 					 * revoked, then we're all done
@@ -648,8 +648,8 @@  static int do_one_pass(journal_t *journal,
 
 					/* Look for block corruption */
 					if (!jbd2_block_tag_csum_verify(
-						journal, tag, obh->b_data,
-						be32_to_cpu(tmp->h_sequence))) {
+			journal, &tag, (journal_block_tag3_t *)tagp,
+			obh->b_data, be32_to_cpu(tmp->h_sequence))) {
 						brelse(obh);
 						success = -EFSBADCRC;
 						printk(KERN_ERR "JBD2: Invalid "
diff --git a/tests/j_recover_fast_commit/script b/tests/j_recover_fast_commit/script
index 22ef6325..05c40cc5 100755
--- a/tests/j_recover_fast_commit/script
+++ b/tests/j_recover_fast_commit/script
@@ -10,7 +10,6 @@  gunzip < $IMAGE > $TMPFILE
 EXP=$test_dir/expect
 OUT=$test_name.log
 
-cp $TMPFILE /tmp/debugthis
 $FSCK $FSCK_OPT -E journal_only -N test_filesys $TMPFILE 2>/dev/null | head -n 1000 | tail -n +2 > $OUT
 echo "Exit status is $?" >> $OUT