From: Theodore Ts'o <[email protected]>
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 <[email protected]>
---
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);
- 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
--
2.31.1.607.g51e8a6a459-goog
From: Harshad Shirwadkar <[email protected]>
Fast commit related struct ext4_fc_tl can be unaligned on disk. So,
while accessing that we should ensure that the pointers are
aligned. This patch fixes unaligned accesses to ext4_fc_tl and also
gets rid of macros fc_for_each_tl and ext4_fc_tag_val that may result
in unaligned accesses to struct ext4_fc_tl.
Signed-off-by: Harshad Shirwadkar <[email protected]>
---
debugfs/logdump.c | 42 ++++++++++----------
e2fsck/journal.c | 82 +++++++++++++++++++++-------------------
lib/ext2fs/fast_commit.h | 13 -------
3 files changed, 65 insertions(+), 72 deletions(-)
diff --git a/debugfs/logdump.c b/debugfs/logdump.c
index 27e2e72d..6aee1a12 100644
--- a/debugfs/logdump.c
+++ b/debugfs/logdump.c
@@ -551,24 +551,28 @@ static inline size_t journal_super_tag_bytes(journal_superblock_t *jsb)
static void dump_fc_block(FILE *out_file, char *buf, int blocksize,
int transaction, int *fc_done, int dump_old)
{
- struct ext4_fc_tl *tl;
+ struct ext4_fc_tl tl;
struct ext4_fc_head *head;
struct ext4_fc_add_range *add_range;
struct ext4_fc_del_range *del_range;
struct ext4_fc_dentry_info *dentry_info;
struct ext4_fc_tail *tail;
struct ext3_extent *ex;
+ __u8 *cur, *val;
*fc_done = 0;
- fc_for_each_tl(buf, buf + blocksize, tl) {
- switch (le16_to_cpu(tl->fc_tag)) {
+ for (cur = (__u8 *)buf; cur < (__u8 *)buf + blocksize;
+ cur = cur + sizeof(tl) + le16_to_cpu(tl.fc_len)) {
+ memcpy(&tl, cur, sizeof(tl));
+ val = cur + sizeof(tl);
+
+ switch (le16_to_cpu(tl.fc_tag)) {
case EXT4_FC_TAG_ADD_RANGE:
- add_range =
- (struct ext4_fc_add_range *)ext4_fc_tag_val(tl);
+ add_range = (struct ext4_fc_add_range *)val;
ex = (struct ext3_extent *)add_range->fc_ex;
fprintf(out_file,
"tag %s, inode %d, lblk %u, pblk %llu, len %lu\n",
- tag2str(tl->fc_tag),
+ tag2str(tl.fc_tag),
le32_to_cpu(add_range->fc_ino),
le32_to_cpu(ex->ee_block),
le32_to_cpu(ex->ee_start) +
@@ -578,10 +582,9 @@ static void dump_fc_block(FILE *out_file, char *buf, int blocksize,
le16_to_cpu(ex->ee_len));
break;
case EXT4_FC_TAG_DEL_RANGE:
- del_range =
- (struct ext4_fc_del_range *)ext4_fc_tag_val(tl);
+ del_range = (struct ext4_fc_del_range *)val;
fprintf(out_file, "tag %s, inode %d, lblk %d, len %d\n",
- tag2str(tl->fc_tag),
+ tag2str(tl.fc_tag),
le32_to_cpu(del_range->fc_ino),
le32_to_cpu(del_range->fc_lblk),
le32_to_cpu(del_range->fc_len));
@@ -589,29 +592,26 @@ static void dump_fc_block(FILE *out_file, char *buf, int blocksize,
case EXT4_FC_TAG_LINK:
case EXT4_FC_TAG_UNLINK:
case EXT4_FC_TAG_CREAT:
- dentry_info =
- (struct ext4_fc_dentry_info *)
- ext4_fc_tag_val(tl);
+ dentry_info = (struct ext4_fc_dentry_info *)val;
fprintf(out_file,
"tag %s, parent %d, ino %d, name \"%s\"\n",
- tag2str(tl->fc_tag),
+ tag2str(tl.fc_tag),
le32_to_cpu(dentry_info->fc_parent_ino),
le32_to_cpu(dentry_info->fc_ino),
dentry_info->fc_dname);
break;
case EXT4_FC_TAG_INODE:
fprintf(out_file, "tag %s, inode %d\n",
- tag2str(tl->fc_tag),
- le32_to_cpu(((struct ext4_fc_inode *)
- ext4_fc_tag_val(tl))->fc_ino));
+ tag2str(tl.fc_tag),
+ le32_to_cpu(((struct ext4_fc_inode *)val)->fc_ino));
break;
case EXT4_FC_TAG_PAD:
- fprintf(out_file, "tag %s\n", tag2str(tl->fc_tag));
+ fprintf(out_file, "tag %s\n", tag2str(tl.fc_tag));
break;
case EXT4_FC_TAG_TAIL:
- tail = (struct ext4_fc_tail *)ext4_fc_tag_val(tl);
+ tail = (struct ext4_fc_tail *)val;
fprintf(out_file, "tag %s, tid %d\n",
- tag2str(tl->fc_tag),
+ tag2str(tl.fc_tag),
le32_to_cpu(tail->fc_tid));
if (!dump_old &&
le32_to_cpu(tail->fc_tid) < transaction) {
@@ -621,9 +621,9 @@ static void dump_fc_block(FILE *out_file, char *buf, int blocksize,
break;
case EXT4_FC_TAG_HEAD:
fprintf(out_file, "\n*** Fast Commit Area ***\n");
- head = (struct ext4_fc_head *)ext4_fc_tag_val(tl);
+ head = (struct ext4_fc_head *)val;
fprintf(out_file, "tag %s, features 0x%x, tid %d\n",
- tag2str(tl->fc_tag),
+ tag2str(tl.fc_tag),
le32_to_cpu(head->fc_features),
le32_to_cpu(head->fc_tid));
if (!dump_old &&
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index bd0e4f31..ae3df800 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -285,9 +285,9 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
struct e2fsck_fc_replay_state *state;
int ret = JBD2_FC_REPLAY_CONTINUE;
struct ext4_fc_add_range *ext;
- struct ext4_fc_tl *tl;
+ struct ext4_fc_tl tl;
struct ext4_fc_tail tail;
- __u8 *start, *end;
+ __u8 *start, *cur, *end, *val;
struct ext4_fc_head head;
struct ext2fs_extent ext2fs_ex = {0};
@@ -313,12 +313,15 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
}
state->fc_replay_expected_off++;
- fc_for_each_tl(start, end, tl) {
+ for (cur = start; cur < end; cur = cur + le16_to_cpu(tl.fc_len) + sizeof(tl)) {
+ memcpy(&tl, cur, sizeof(tl));
+ val = cur + sizeof(tl);
+
jbd_debug(3, "Scan phase, tag:%s, blk %lld\n",
- tag2str(le16_to_cpu(tl->fc_tag)), bh->b_blocknr);
- switch (le16_to_cpu(tl->fc_tag)) {
+ tag2str(le16_to_cpu(tl.fc_tag)), bh->b_blocknr);
+ switch (le16_to_cpu(tl.fc_tag)) {
case EXT4_FC_TAG_ADD_RANGE:
- ext = (struct ext4_fc_add_range *)ext4_fc_tag_val(tl);
+ ext = (struct ext4_fc_add_range *)val;
ret = ext2fs_decode_extent(&ext2fs_ex, (void *)&ext->fc_ex,
sizeof(ext->fc_ex));
if (ret)
@@ -333,14 +336,14 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
case EXT4_FC_TAG_INODE:
case EXT4_FC_TAG_PAD:
state->fc_cur_tag++;
- state->fc_crc = jbd2_chksum(j, state->fc_crc, tl,
- sizeof(*tl) + ext4_fc_tag_len(tl));
+ state->fc_crc = jbd2_chksum(j, state->fc_crc, cur,
+ sizeof(tl) + ext4_fc_tag_len(&tl));
break;
case EXT4_FC_TAG_TAIL:
state->fc_cur_tag++;
- memcpy(&tail, ext4_fc_tag_val(tl), sizeof(tail));
- state->fc_crc = jbd2_chksum(j, state->fc_crc, tl,
- sizeof(*tl) +
+ memcpy(&tail, val, sizeof(tail));
+ state->fc_crc = jbd2_chksum(j, state->fc_crc, cur,
+ sizeof(tl) +
offsetof(struct ext4_fc_tail,
fc_crc));
jbd_debug(1, "tail tid %d, expected %d\n",
@@ -355,7 +358,7 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
state->fc_crc = 0;
break;
case EXT4_FC_TAG_HEAD:
- memcpy(&head, ext4_fc_tag_val(tl), sizeof(head));
+ memcpy(&head, val, sizeof(head));
if (le32_to_cpu(head.fc_features) &
~EXT4_FC_SUPPORTED_FEATURES) {
ret = -EOPNOTSUPP;
@@ -366,8 +369,8 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
break;
}
state->fc_cur_tag++;
- state->fc_crc = jbd2_chksum(j, state->fc_crc, tl,
- sizeof(*tl) + ext4_fc_tag_len(tl));
+ state->fc_crc = jbd2_chksum(j, state->fc_crc, cur,
+ sizeof(tl) + ext4_fc_tag_len(&tl));
break;
default:
ret = state->fc_replay_num_tags ?
@@ -612,12 +615,12 @@ struct dentry_info_args {
};
static inline int tl_to_darg(struct dentry_info_args *darg,
- struct ext4_fc_tl *tl)
+ struct ext4_fc_tl *tl, __u8 *val)
{
struct ext4_fc_dentry_info fcd;
int tag = le16_to_cpu(tl->fc_tag);
- memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));
+ memcpy(&fcd, val, sizeof(fcd));
darg->parent_ino = le32_to_cpu(fcd.fc_parent_ino);
darg->ino = le32_to_cpu(fcd.fc_ino);
@@ -627,7 +630,7 @@ static inline int tl_to_darg(struct dentry_info_args *darg,
if (!darg->dname)
return -ENOMEM;
memcpy(darg->dname,
- ext4_fc_tag_val(tl) + sizeof(struct ext4_fc_dentry_info),
+ val + 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",
@@ -638,14 +641,14 @@ static inline int tl_to_darg(struct dentry_info_args *darg,
return 0;
}
-static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
+static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl, __u8 *val)
{
struct ext2_inode inode;
struct dentry_info_args darg;
ext2_filsys fs = ctx->fs;
int ret;
- ret = tl_to_darg(&darg, tl);
+ ret = tl_to_darg(&darg, tl, val);
if (ret)
return ret;
ext4_fc_flush_extents(ctx, darg.ino);
@@ -657,14 +660,14 @@ static int ext4_fc_handle_unlink(e2fsck_t ctx, struct ext4_fc_tl *tl)
return ret;
}
-static int ext4_fc_handle_link_and_create(e2fsck_t ctx, struct ext4_fc_tl *tl)
+static int ext4_fc_handle_link_and_create(e2fsck_t ctx, struct ext4_fc_tl *tl, __u8 *val)
{
struct dentry_info_args darg;
ext2_filsys fs = ctx->fs;
struct ext2_inode_large inode_large;
int ret, filetype, mode;
- ret = tl_to_darg(&darg, tl);
+ ret = tl_to_darg(&darg, tl, val);
if (ret)
return ret;
ext4_fc_flush_extents(ctx, 0);
@@ -732,7 +735,7 @@ static void ext4_fc_replay_fixup_iblocks(struct ext2_inode_large *ondisk_inode,
}
}
-static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
+static int ext4_fc_handle_inode(e2fsck_t ctx, __u8 *val)
{
struct e2fsck_fc_replay_state *state = &ctx->fc_replay_state;
int ino, inode_len = EXT2_GOOD_OLD_INODE_SIZE;
@@ -742,8 +745,8 @@ static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl)
errcode_t err;
blk64_t blks;
- memcpy(&fc_ino, ext4_fc_tag_val(tl), sizeof(fc_ino));
- fc_raw_inode = ext4_fc_tag_val(tl) + sizeof(fc_ino);
+ memcpy(&fc_ino, val, sizeof(fc_ino));
+ fc_raw_inode = val + sizeof(fc_ino);
ino = le32_to_cpu(fc_ino);
if (EXT2_INODE_SIZE(ctx->fs->super) > EXT2_GOOD_OLD_INODE_SIZE)
@@ -797,13 +800,13 @@ out:
/*
* Handle add extent replay tag.
*/
-static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
+static int ext4_fc_handle_add_extent(e2fsck_t ctx, __u8 *val)
{
struct ext2fs_extent extent;
struct ext4_fc_add_range add_range;
int ret = 0, ino;
- memcpy(&add_range, ext4_fc_tag_val(tl), sizeof(add_range));
+ memcpy(&add_range, val, sizeof(add_range));
ino = le32_to_cpu(add_range.fc_ino);
ext4_fc_flush_extents(ctx, ino);
@@ -823,13 +826,13 @@ static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl)
/*
* Handle delete logical range replay tag.
*/
-static int ext4_fc_handle_del_range(e2fsck_t ctx, struct ext4_fc_tl *tl)
+static int ext4_fc_handle_del_range(e2fsck_t ctx, __u8 *val)
{
struct ext2fs_extent extent;
struct ext4_fc_del_range del_range;
int ret, ino;
- memcpy(&del_range, ext4_fc_tag_val(tl), sizeof(del_range));
+ memcpy(&del_range, val, sizeof(del_range));
ino = le32_to_cpu(del_range.fc_ino);
ext4_fc_flush_extents(ctx, ino);
@@ -854,8 +857,8 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,
e2fsck_t ctx = journal->j_fs_dev->k_ctx;
struct e2fsck_fc_replay_state *state = &ctx->fc_replay_state;
int ret = JBD2_FC_REPLAY_CONTINUE;
- struct ext4_fc_tl *tl;
- __u8 *start, *end;
+ struct ext4_fc_tl tl;
+ __u8 *start, *end, *cur, *val;
if (pass == PASS_SCAN) {
state->fc_current_pass = PASS_SCAN;
@@ -891,28 +894,31 @@ static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,
start = (__u8 *)bh->b_data;
end = (__u8 *)bh->b_data + journal->j_blocksize - 1;
- fc_for_each_tl(start, end, tl) {
+ for (cur = start; cur < end; cur = cur + le16_to_cpu(tl.fc_len) + sizeof(tl)) {
+ memcpy(&tl, cur, sizeof(tl));
+ val = cur + sizeof(tl);
+
if (state->fc_replay_num_tags == 0)
goto replay_done;
jbd_debug(3, "Replay phase processing %s tag\n",
- tag2str(le16_to_cpu(tl->fc_tag)));
+ tag2str(le16_to_cpu(tl.fc_tag)));
state->fc_replay_num_tags--;
- switch (le16_to_cpu(tl->fc_tag)) {
+ switch (le16_to_cpu(tl.fc_tag)) {
case EXT4_FC_TAG_CREAT:
case EXT4_FC_TAG_LINK:
- ret = ext4_fc_handle_link_and_create(ctx, tl);
+ ret = ext4_fc_handle_link_and_create(ctx, &tl, val);
break;
case EXT4_FC_TAG_UNLINK:
- ret = ext4_fc_handle_unlink(ctx, tl);
+ ret = ext4_fc_handle_unlink(ctx, &tl, val);
break;
case EXT4_FC_TAG_ADD_RANGE:
- ret = ext4_fc_handle_add_extent(ctx, tl);
+ ret = ext4_fc_handle_add_extent(ctx, val);
break;
case EXT4_FC_TAG_DEL_RANGE:
- ret = ext4_fc_handle_del_range(ctx, tl);
+ ret = ext4_fc_handle_del_range(ctx, val);
break;
case EXT4_FC_TAG_INODE:
- ret = ext4_fc_handle_inode(ctx, tl);
+ ret = ext4_fc_handle_inode(ctx, val);
break;
case EXT4_FC_TAG_TAIL:
ext4_fc_flush_extents(ctx, 0);
diff --git a/lib/ext2fs/fast_commit.h b/lib/ext2fs/fast_commit.h
index b83e1810..4ad38f12 100644
--- a/lib/ext2fs/fast_commit.h
+++ b/lib/ext2fs/fast_commit.h
@@ -155,13 +155,6 @@ struct ext4_fc_replay_state {
#define region_last(__region) (((__region)->lblk) + ((__region)->len) - 1)
#endif
-#define fc_for_each_tl(__start, __end, __tl) \
- for (tl = (struct ext4_fc_tl *)(__start); \
- (__u8 *)tl < (__u8 *)(__end); \
- tl = (struct ext4_fc_tl *)((__u8 *)tl + \
- sizeof(struct ext4_fc_tl) + \
- + le16_to_cpu(tl->fc_len)))
-
static inline const char *tag2str(__u16 tag)
{
switch (tag) {
@@ -194,10 +187,4 @@ static inline int ext4_fc_tag_len(struct ext4_fc_tl *tl)
return le16_to_cpu(tl->fc_len);
}
-/* Get a pointer to "value" of a tlv */
-static inline __u8 *ext4_fc_tag_val(struct ext4_fc_tl *tl)
-{
- return (__u8 *)tl + sizeof(*tl);
-}
-
#endif /* __FAST_COMMIT_H__ */
--
2.31.1.607.g51e8a6a459-goog
On Thu, May 06, 2021 at 05:21:10PM -0700, Harshad Shirwadkar wrote:
> From: Harshad Shirwadkar <[email protected]>
>
> Fast commit related struct ext4_fc_tl can be unaligned on disk. So,
> while accessing that we should ensure that the pointers are
> aligned. This patch fixes unaligned accesses to ext4_fc_tl and also
> gets rid of macros fc_for_each_tl and ext4_fc_tag_val that may result
> in unaligned accesses to struct ext4_fc_tl.
>
> Signed-off-by: Harshad Shirwadkar <[email protected]>
Looks good. I wrote my reply with a proposed version of
ext4_fc_tag_val() before I saw your patch.
This patch wasn't enough to fix the sparc64 crash, but after doing
some additional investigation, I was able to figure out how to fix
things so that j_recovery_fast_commit is working on sparc64. Patch
follows on this thread...
- Ted
These fast commit related structures can be unaligned on disk. So we
need to avoid accessing these structures directly, and first copy
them to memory which we know is appropriately aligned.
This fixes an e2fsck crash while running the j_recovery_fast_commit
regression test on a sparc64 system.
Signed-off-by: Theodore Ts'o <[email protected]>
---
e2fsck/journal.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index ae3df800..0128fbd3 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -284,7 +284,7 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
e2fsck_t ctx = j->j_fs_dev->k_ctx;
struct e2fsck_fc_replay_state *state;
int ret = JBD2_FC_REPLAY_CONTINUE;
- struct ext4_fc_add_range *ext;
+ struct ext4_fc_add_range ext;
struct ext4_fc_tl tl;
struct ext4_fc_tail tail;
__u8 *start, *cur, *end, *val;
@@ -321,9 +321,10 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
tag2str(le16_to_cpu(tl.fc_tag)), bh->b_blocknr);
switch (le16_to_cpu(tl.fc_tag)) {
case EXT4_FC_TAG_ADD_RANGE:
- ext = (struct ext4_fc_add_range *)val;
- ret = ext2fs_decode_extent(&ext2fs_ex, (void *)&ext->fc_ex,
- sizeof(ext->fc_ex));
+ memcpy(&ext, val, sizeof(ext));
+ ret = ext2fs_decode_extent(&ext2fs_ex,
+ (void *)&ext.fc_ex,
+ sizeof(ext.fc_ex));
if (ret)
ret = JBD2_FC_REPLAY_STOP;
else
@@ -764,12 +765,9 @@ static int ext4_fc_handle_inode(e2fsck_t ctx, __u8 *val)
inode_len);
if (err)
goto out;
-#ifdef WORDS_BIGENDIAN
- ext2fs_swap_inode_full(ctx->fs, fc_inode,
- (struct ext2_inode_large *)fc_raw_inode,
- 0, sizeof(*inode));
-#else
memcpy(fc_inode, fc_raw_inode, inode_len);
+#ifdef WORDS_BIGENDIAN
+ ext2fs_swap_inode_full(ctx->fs, fc_inode, fc_inode, 0, inode_len);
#endif
memcpy(inode, fc_inode, offsetof(struct ext2_inode_large, i_block));
memcpy(&inode->i_generation, &fc_inode->i_generation,
--
2.31.0