[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#988830: marked as done (unblock: e2fsprogs/1.46.2-1)

Your message dated Mon, 21 Jun 2021 19:51:08 +0200
with message-id <500fbb60-7d0e-791b-5c1c-5f1823bef986@debian.org>
and subject line Re: Bug#988830: [pre-approval] unblock e2fsprogs [Was: Bug#987641: e2fsprogs: FTBFS on armel/armhf with a 64-bit kernel]
has caused the Debian Bug report #988830,
regarding unblock: e2fsprogs/1.46.2-1
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
988830: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=988830
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
Package: release.debian.org
User: release.debian.org@packages.debian.org
Usertags: unblock
Severity: normal
Tags: d-i moreinfo

Hi Theodore,

On 20-05-2021 00:11, Theodore Y. Ts'o wrote:
> Ping to the debian-release bug.

Unfortunately, there was no release.debian.org bug to track this. Due to
the current high volume to our list, this fell from the radar. To avoid
this I now generate a pre-approval unblock request to discuss this,
because than it shows up in our tools. Please follow up there.

> Do you want me to upload a fix to
> this bug where e2fsprogs fails its regression test (and thus its
> package build) when armhf and armel are running on a 64-bit ARM
> platform, but they were built successfully when run on a 32-bit ARM
> builder?

e2fsprogs is on the (build-)essential list [1], so I'm glad you ask
before uploading as we ask in the freeze-policy [2].

> No question this is a real bug, and it is fixed upstream already.  But
> do you want me to upload a fix *now*, during the hard freeze, given
> the impact on the installer, et. al.?

Can you elaborate where you see the *risks* of the patch? Is this patch
backwards compatible? I.e. does it work correctly on data generated with
the old e2fsprogs? If not, what must the user do to avoid issues? Should
it be mentioned in the release notes?

Apart from the failing test cases, I see in the patch description that
there's also real use cases impacted (corner cases if I interpret them
right). IIUC these are no regressions but I'd like to be sure. And
what's the impact for users of those corner cases (especially the new
Linux feature, I would expect that some users would be going to use those).

Paul

[1] https://release.debian.org/bullseye/essential-and-build-essential.txt
[2] https://release.debian.org/bullseye/freeze_policy.html#transition
--- Begin Message --- 
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 <tytso@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 <tytso@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

--- End Message ---

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


--- End Message ---
--- Begin Message --- 
Hi Ted,

On 08-06-2021 05:42, Theodore Ts'o wrote:
> I've just uploaded the package to unstable.

unblocked.

Paul

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


--- End Message ---
Reply to: