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 <adilger@xxxxxxxxx> wrote:
>
> On May 3, 2021, at 9:10 PM, Theodore Ts'o <tytso@xxxxxxx> 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
>
> Minor style comments inline.
>
> > Addresses-Debian-Bug: #987641
> > Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> > ---
> > 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
>
> > 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
>
> > 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
>
>
>
>
>
