Hi Ted,

Thanks for the patch. While I now see that these accesses are safe,
ubsan still complains about it the dereferences not being aligned.
With your changes, the way we read journal_block_tag_t is now safe.
But IIUC, ubsan still complains mainly because we still pass the
pointer as "&tag->t_flags" and at which point ubsan thinks that we are
accessing member t_flags in an aligned way. Is there a way to silence
these errors?

I was wondering if it makes sense to do something like this for known
unaligned structures:

journal_block_tag_t local, *unaligned;
...
memcpy(&local, unaligned, sizeof(&local));

// access local.t_flags instead of unaligned

Here are the failures ubsan that I still see:

recovery.c:243:24: runtime error: member access within misaligned
address 0x000001c18fae for type 'journal_block_tag_t' (aka 'struct
journal_block_tag_s'), which requires 4 byte alignment
0x000001c18fae: note: pointer points here
 00 00 00 00 00 01  e0 01 ac 26 00 02 00 00  00 00 01 03 ac 26 00 02
00 00 00 01 e0 02 ac 26  00 02
             ^
Thanks,
Harshad

On Mon, May 3, 2021 at 11:33 PM Andreas Dilger [off-list ref] wrote:

On May 3, 2021, at 9:10 PM, Theodore Ts'o [off-list ref] wrote:

quoted

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

Minor style comments inline.

quoted

Addresses-Debian-Bug: #987641
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
e2fsck/journal.c                   | 41 ++++++++++++++++++++---------
e2fsck/recovery.c                  | 42 +++++++++++++++++++++++++-----
tests/j_recover_fast_commit/script |  1 -
3 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a425bbd1..2231b811 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c

@@ -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) {

(style) better to align continued line after '(' on previous line?  That way
it can be distinguished from the next (body) line more easily

quoted

                              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) {

(style) same

quoted

                              ret = -EOPNOTSUPP;
                              break;
                      }
-                     if (le32_to_cpu(head->fc_tid) != expected_tid) {
+                     if (get_le32(&head->fc_tid) != expected_tid) {
                              ret = -EINVAL;
                              break;
                      }

Cheers, Andreas