On Mon, Apr 26, 2021 at 11:01:45PM +0200, Aurelien Jarno wrote: > Source: e2fsprogs > Version: 1.46.2-1 > Severity: serious > Tags: upstream ftbfs > Justification: fails to build from source (but built successfully in the past) > Forwarded: https://github.com/tytso/e2fsprogs/issues/65 > > e2fsprogs builds fine on armel/armhf when built on a machine with a > 32-bit kernel. However it fails to build on a machine with a 64-bit > kernel due to alignments issues which are not trapped by the kernel: > > A build log is available there: > https://tests.reproducible-builds.org/debian/logs/unstable/armhf/e2fsprogs_1.46.2-1.build2.log.gz
Hi, thanks for the bug report. I have a patch which should address this problem. (See below). I have a question for the Debian Release Team (cc'ed). Do you agree this is considered "serious"? It will build from source on a system with a arm-32 kernel. It is only when cross-compiling on armel or armhf on a aarch64 platform that some regression tests (j_recover_csum2_32bit, j_recover_csum2_64bit, and j_recover_fast_commit) will fail, and it is this which causes dpkg-buildpackage when run on a arm-32 chroot on a 64-bit arm system to fail. So it is not completely clear to me that this qualifies as a FTBFS, such that the release team would grant an migration into bullseye given that we are currently in "frozen hard to get hot"[1] [1] https://lists.debian.org/debian-devel-announce/2021/03/msg00006.html I actually wouldn't mind it if I could sneak in an update into Bullseye, especially if I could fix one or two other bugs at the same time that don't qualify for RC. (For example, Debian Bug: #984472[2], and a bug fix for mke2fs -D which was reported by the Yocto project[3]. [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=984472 [3] https://github.com/tytso/e2fsprogs/pull/68 However, given that e2fsprogs is used by the installer, and we're pretty late in the Bulleye Freeze --- I'd like to get a general approval of the concept of an e2fsprogs update for Bullseye at this point in time before I prepare an update and file a formal unblock request. Or we can do this after the initial Bullseye release, if that would be more convenient for the release process. What say ye? Many thanks, - Ted commit bc8e4e56fcdd2a9e65cc87f6303dd127f79ad22d Author: Theodore Ts'o <ty...@mit.edu> Date: Mon May 3 15:37:33 2021 -0400 e2fsck: fix portability problems caused by unaligned accesses 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 <ty...@mit.edu> diff --git a/e2fsck/journal.c b/e2fsck/journal.c index a425bbd1..2231b811 100644 --- a/e2fsck/journal.c +++ b/e2fsck/journal.c @@ -278,6 +278,23 @@ static int process_journal_block(ext2_filsys fs, return 0; } +/* + * This function works on unaligned 32-bit pointers, which is needed + * since fast_commit's on-disk format does not guarantee that pointers + * are 32-bit aligned. + */ +static __u32 get_le32(__le32 *p) +{ + unsigned char *cp = (unsigned char *) p; + __u32 ret; + + ret = (__u32) *cp++; + ret = ret | ((__u32) *cp++ << 8); + ret = ret | ((__u32) *cp++ << 16); + ret = ret | ((__u32) *cp++ << 24); + return ret; +} + static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh, int off, tid_t expected_tid) { @@ -344,10 +361,10 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh, offsetof(struct ext4_fc_tail, fc_crc)); jbd_debug(1, "tail tid %d, expected %d\n", - le32_to_cpu(tail->fc_tid), + get_le32(&tail->fc_tid), expected_tid); - if (le32_to_cpu(tail->fc_tid) == expected_tid && - le32_to_cpu(tail->fc_crc) == state->fc_crc) { + if (get_le32(&tail->fc_tid) == expected_tid && + get_le32(&tail->fc_crc) == state->fc_crc) { state->fc_replay_num_tags = state->fc_cur_tag; } else { ret = state->fc_replay_num_tags ? @@ -357,12 +374,12 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh, break; case EXT4_FC_TAG_HEAD: head = (struct ext4_fc_head *)ext4_fc_tag_val(tl); - if (le32_to_cpu(head->fc_features) & + if (get_le32(&head->fc_features) & ~EXT4_FC_SUPPORTED_FEATURES) { ret = -EOPNOTSUPP; break; } - if (le32_to_cpu(head->fc_tid) != expected_tid) { + if (get_le32(&head->fc_tid) != expected_tid) { ret = -EINVAL; break; } @@ -620,8 +637,8 @@ static inline void tl_to_darg(struct dentry_info_args *darg, fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl); - darg->parent_ino = le32_to_cpu(fcd->fc_parent_ino); - darg->ino = le32_to_cpu(fcd->fc_ino); + darg->parent_ino = get_le32(&fcd->fc_parent_ino); + darg->ino = get_le32(&fcd->fc_ino); darg->dname = (char *) fcd->fc_dname; darg->dname_len = ext4_fc_tag_len(tl) - sizeof(struct ext4_fc_dentry_info); @@ -735,7 +752,7 @@ static int ext4_fc_handle_inode(e2fsck_t ctx, struct ext4_fc_tl *tl) blk64_t blks; fc_inode_val = (struct ext4_fc_inode *)ext4_fc_tag_val(tl); - ino = le32_to_cpu(fc_inode_val->fc_ino); + ino = get_le32(&fc_inode_val->fc_ino); if (EXT2_INODE_SIZE(ctx->fs->super) > EXT2_GOOD_OLD_INODE_SIZE) inode_len += ext2fs_le16_to_cpu( @@ -797,7 +814,7 @@ static int ext4_fc_handle_add_extent(e2fsck_t ctx, struct ext4_fc_tl *tl) int ret = 0, ino; add_range = (struct ext4_fc_add_range *)ext4_fc_tag_val(tl); - ino = le32_to_cpu(add_range->fc_ino); + ino = get_le32(&add_range->fc_ino); ext4_fc_flush_extents(ctx, ino); ret = ext4_fc_read_extents(ctx, ino); @@ -823,12 +840,12 @@ static int ext4_fc_handle_del_range(e2fsck_t ctx, struct ext4_fc_tl *tl) int ret, ino; del_range = (struct ext4_fc_del_range *)ext4_fc_tag_val(tl); - ino = le32_to_cpu(del_range->fc_ino); + ino = get_le32(&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 = get_le32(&del_range->fc_lblk); + extent.e_len = get_le32(&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..920c3dab 100644 --- a/e2fsck/recovery.c +++ b/e2fsck/recovery.c @@ -121,6 +121,36 @@ failed: #endif /* __KERNEL__ */ +/* + * This function works on unaligned 32-bit pointers, which is needed for + * an obsolete jbd2 checksum variant. + */ +static inline __u32 get_be32(__be32 *p) +{ + unsigned char *cp = (unsigned char *) p; + __u32 ret; + + ret = *cp++; + ret = (ret << 8) + *cp++; + ret = (ret << 8) + *cp++; + ret = (ret << 8) + *cp++; + return ret; +} + +/* + * This function works on unaligned 16-bit pointers, which is needed for + * an obsolete jbd2 checksum variant. + */ +static inline __u16 get_be16(__be16 *p) +{ + unsigned char *cp = (unsigned char *) p; + __u16 ret; + + ret = *cp++; + ret = (ret << 8) + *cp++; + return ret; +} + /* * Read a block from the journal */ @@ -210,10 +240,10 @@ static int count_tags(journal_t *journal, struct buffer_head *bh) nr++; tagp += tag_bytes; - if (!(tag->t_flags & cpu_to_be16(JBD2_FLAG_SAME_UUID))) + if (!(get_be16(&tag->t_flags) & JBD2_FLAG_SAME_UUID)) tagp += 16; - if (tag->t_flags & cpu_to_be16(JBD2_FLAG_LAST_TAG)) + if (get_be16(&tag->t_flags) & JBD2_FLAG_LAST_TAG) break; } @@ -377,9 +407,9 @@ int jbd2_journal_skip_recovery(journal_t *journal) static inline unsigned long long read_tag_block(journal_t *journal, journal_block_tag_t *tag) { - unsigned long long block = be32_to_cpu(tag->t_blocknr); + unsigned long long block = get_be32(&tag->t_blocknr); if (jbd2_has_feature_64bit(journal)) - block |= (u64)be32_to_cpu(tag->t_blocknr_high) << 32; + block |= (u64)get_be32(&tag->t_blocknr_high) << 32; return block; } @@ -450,7 +480,7 @@ static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag, if (jbd2_has_feature_csum3(j)) return tag3->t_checksum == cpu_to_be32(csum32); else - return tag->t_checksum == cpu_to_be16(csum32); + return get_be16(&tag->t_checksum) == csum32; } static int do_one_pass(journal_t *journal, @@ -615,7 +645,7 @@ static int do_one_pass(journal_t *journal, unsigned long io_block; tag = (journal_block_tag_t *) tagp; - flags = be16_to_cpu(tag->t_flags); + flags = get_be16(&tag->t_flags); io_block = next_log_block++; wrap(journal, next_log_block); 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
