LinuxLists.cc - [PATCH] e2fsck: fix portability problems caused by unaligned accesses

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

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 <[email protected]> wrote:
>
> On May 3, 2021, at 9:10 PM, Theodore Ts'o <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
>
>
>
>

2021-05-04 13:49:41

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 02:40:08AM -0700, harshad shirwadkar wrote:
> 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?

Yeah, I had noticed that. I was thinking perhaps of doing something
like casting the pointer to void * or char *, and then adding offsetof
to work around the UBSAN warning. Or maybe asking the compiler folks
if they can make the UBSAN warning smarter, since what we're doing
should be perfectly safe.

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

I guess that would work too. The extra memory copy is unfortunate,
although I suspect the performance hit isn't measurable, and journal
replay isn't really a hot path in either the kernel or e2fsprogs.
(Note that want to keep recovery.c in sync between the kernel and
e2fsprogs, so whatever we do needs to be something we're happy with in
both places.)

- Ted

2021-05-04 15:19:24

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 12:29:21AM -0600, Andreas Dilger wrote:
> > @@ -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

Thanks, I fixed up the whitespace style issues (which were in the
original code, but while we're modifying these lines, might as well
fix them up).

- Ted

2021-05-04 17:18:05

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 09:49:15AM -0400, Theodore Ts'o wrote:
> On Tue, May 04, 2021 at 02:40:08AM -0700, harshad shirwadkar wrote:
> > 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?
>
> Yeah, I had noticed that. I was thinking perhaps of doing something
> like casting the pointer to void * or char *, and then adding offsetof
> to work around the UBSAN warning. Or maybe asking the compiler folks
> if they can make the UBSAN warning smarter, since what we're doing
> should be perfectly safe.

This does seem to be an UBSAN bug, although both gcc and clang report this same
error, which is odd... Dereferencing a misaligned field would be undefined
behavior, but just taking its address isn't (AFAIK).

>
> >
> > 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));
>
> I guess that would work too. The extra memory copy is unfortunate,
> although I suspect the performance hit isn't measurable, and journal
> replay isn't really a hot path in either the kernel or e2fsprogs.
> (Note that want to keep recovery.c in sync between the kernel and
> e2fsprogs, so whatever we do needs to be something we're happy with in
> both places.)
>

Modern compilers will optimize out the memcpy().

However, wouldn't it be easier to just add __attribute__((packed)) to the
definition of struct journal_block_tag_t?

- Eric

2021-05-04 17:56:28

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 4, 2021 at 9:46 AM Eric Biggers <[email protected]> wrote:
>
> On Tue, May 04, 2021 at 09:49:15AM -0400, Theodore Ts'o wrote:
> > On Tue, May 04, 2021 at 02:40:08AM -0700, harshad shirwadkar wrote:
> > > 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?
> >
> > Yeah, I had noticed that. I was thinking perhaps of doing something
> > like casting the pointer to void * or char *, and then adding offsetof
> > to work around the UBSAN warning. Or maybe asking the compiler folks
> > if they can make the UBSAN warning smarter, since what we're doing
> > should be perfectly safe.
>
> This does seem to be an UBSAN bug, although both gcc and clang report this same
> error, which is odd... Dereferencing a misaligned field would be undefined
> behavior, but just taking its address isn't (AFAIK).
>
> >
> > >
> > > 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));
> >
> > I guess that would work too. The extra memory copy is unfortunate,
> > although I suspect the performance hit isn't measurable, and journal
> > replay isn't really a hot path in either the kernel or e2fsprogs.
> > (Note that want to keep recovery.c in sync between the kernel and
> > e2fsprogs, so whatever we do needs to be something we're happy with in
> > both places.)
> >
>
> Modern compilers will optimize out the memcpy().
>
> However, wouldn't it be easier to just add __attribute__((packed)) to the
> definition of struct journal_block_tag_t?
While we know that journal_block_tag_t can be unaligned, our code
should still ensure that we are reading this struct in an
alignment-safe way (like Ted's patch does). IIUC, using
__attribute__((packed)) might result in us keeping the door open for
unaligned accesses in future. If someone tries to read 4 bytes
starting at &journal_block_tag_t->t_flags, with attribute packed,
UBSAN won't complain but this may still cause issues on some
architectures.

Another option would be to define wrappers that access known unaligned
structures in an alignment safe way and declare those wrappers with
__attribute__((no_sanitize("undefined")). This would both make sure
that we always access unaligned structs in an alignment safe way and
also would get rid of UBSAN warnings.

- Harshad

>
> - Eric

2021-05-04 19:15:33

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 10:55:44AM -0700, harshad shirwadkar wrote:
> > However, wouldn't it be easier to just add __attribute__((packed)) to the
> > definition of struct journal_block_tag_t?
> While we know that journal_block_tag_t can be unaligned, our code
> should still ensure that we are reading this struct in an
> alignment-safe way (like Ted's patch does). IIUC, using
> __attribute__((packed)) might result in us keeping the door open for
> unaligned accesses in future. If someone tries to read 4 bytes
> starting at &journal_block_tag_t->t_flags, with attribute packed,
> UBSAN won't complain but this may still cause issues on some
> architectures.

I don't understand your concern here. Accesses to a packed struct are assumed
to be unaligned -- that's why I suggested it. The packed attribute is pretty
widely used to implement unaligned accesses in C (as an alternative to memcpy()
or explicit byte-by-byte accesses, both of which also work, though the latter
seems to run into an UBSAN bug in this case).

- Eric

2021-05-04 19:55:50

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 12:14:20PM -0700, Eric Biggers wrote:
> On Tue, May 04, 2021 at 10:55:44AM -0700, harshad shirwadkar wrote:
> > > However, wouldn't it be easier to just add __attribute__((packed)) to the
> > > definition of struct journal_block_tag_t?
> > While we know that journal_block_tag_t can be unaligned, our code
> > should still ensure that we are reading this struct in an
> > alignment-safe way (like Ted's patch does). IIUC, using
> > __attribute__((packed)) might result in us keeping the door open for
> > unaligned accesses in future. If someone tries to read 4 bytes
> > starting at &journal_block_tag_t->t_flags, with attribute packed,
> > UBSAN won't complain but this may still cause issues on some
> > architectures.
>
> I don't understand your concern here. Accesses to a packed struct are assumed
> to be unaligned -- that's why I suggested it. The packed attribute is pretty
> widely used to implement unaligned accesses in C (as an alternative to memcpy()
> or explicit byte-by-byte accesses, both of which also work, though the latter
> seems to run into an UBSAN bug in this case).

So part of the problem is that documentation for
__attribute__((packed)) is terrible. From the GCC documentation:

'packed'
The 'packed' attribute specifies that a structure member should
have the smallest possible alignment--one bit for a bit-field and
one byte otherwise, unless a larger value is specified with the
'aligned' attribute. The attribute does not apply to non-member
objects.

For example in the structure below, the member array 'x' is packed
so that it immediately follows 'a' with no intervening padding:

struct foo
{
char a;
int x[2] __attribute__ ((packed));
};

_Note:_ The 4.1, 4.2 and 4.3 series of GCC ignore the 'packed'
attribute on bit-fields of type 'char'. This has been fixed in GCC
4.4 but the change can lead to differences in the structure layout.
See the documentation of '-Wpacked-bitfield-compat' for more
information.

I was under the impression that the only thing the packed attribute
did was to change the structure layout. So I was about to reply with
a message saying, "How does this do anything? The structure is
already packed, so isn't this a no-op?"

But I did the experiment, and your suggestion worked.... so I then
started digging for documentation for this being guaranteed behavior
for gcc and clang.... and I found nothing but blog posts. One of them
is by Roland Dreir (infiniband Linux hacker):

http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

which does state:

But adding __attribute__((packed)) goes further than just telling
gcc that it shouldn’t add padding — it also tells gcc that it can’t
make any assumptions about the alignment of accesses to structure
members

But I wouldn't exactly call this documented behavior on the part of
GCC. I guess we could hope that behavior that apparently has been
around for 15+ years is probably not going to change on us, but I
would really prefer something in writing. :-)

- Ted

P.S. Roland's blog goes on to say:

... And this leads to disastrously bad code on some architectures.

and has some examples of some really terrible codegen on ia64 and
sparc64.

2021-05-04 20:32:58

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

I see thanks for the explanation. I quickly tried it too and saw that
UBSAN warnings went away but I got compiler warnings
"recovery.c:413:27: warning: taking address of packed member
't_blocknr_high' of class or structure 'journal_block_tag_s' may
result in an unaligned pointer value [-Waddress-of-packed-member]".
These compiler warnings seem to be added in [1].

These warnings make me think that de-referencing a member of a packed
struct is still not safe. My concern is this - If we define
journal_block_tag_t as a packed struct AND if we have following unsafe
code, then we won't see UBSAN warnings and the following unaligned
accesses would go unnoticed. That may not go well on certain
architectures.

j_block_tag_t *unaligned_ptr;

flags = unaligned_ptr->t_flags;

It looks like if the compiler doesn't support
-Waddress-of-packed-member [1], we may not even see these warnings, we
won't see UBSAN warnings and the unaligned accesses may cause problems
on the architectures that you mentioned.

In other words, what I'm trying to say is that while
__atribute__((packed)) would silence UBSAN warnings (and we should do
it), it's still not sufficient to ensure that our code doesn't do
unaligned accesses to the struct in question. Does that make sense?

- Harshad

[1] https://reviews.llvm.org/rG722a4db1985ca9a8b982074d658dfee9c4624d53

On Tue, May 4, 2021 at 12:53 PM Theodore Ts'o <[email protected]> wrote:
>
> On Tue, May 04, 2021 at 12:14:20PM -0700, Eric Biggers wrote:
> > On Tue, May 04, 2021 at 10:55:44AM -0700, harshad shirwadkar wrote:
> > > > However, wouldn't it be easier to just add __attribute__((packed)) to the
> > > > definition of struct journal_block_tag_t?
> > > While we know that journal_block_tag_t can be unaligned, our code
> > > should still ensure that we are reading this struct in an
> > > alignment-safe way (like Ted's patch does). IIUC, using
> > > __attribute__((packed)) might result in us keeping the door open for
> > > unaligned accesses in future. If someone tries to read 4 bytes
> > > starting at &journal_block_tag_t->t_flags, with attribute packed,
> > > UBSAN won't complain but this may still cause issues on some
> > > architectures.
> >
> > I don't understand your concern here. Accesses to a packed struct are assumed
> > to be unaligned -- that's why I suggested it. The packed attribute is pretty
> > widely used to implement unaligned accesses in C (as an alternative to memcpy()
> > or explicit byte-by-byte accesses, both of which also work, though the latter
> > seems to run into an UBSAN bug in this case).
>
> So part of the problem is that documentation for
> __attribute__((packed)) is terrible. From the GCC documentation:
>
> 'packed'
> The 'packed' attribute specifies that a structure member should
> have the smallest possible alignment--one bit for a bit-field and
> one byte otherwise, unless a larger value is specified with the
> 'aligned' attribute. The attribute does not apply to non-member
> objects.
>
> For example in the structure below, the member array 'x' is packed
> so that it immediately follows 'a' with no intervening padding:
>
> struct foo
> {
> char a;
> int x[2] __attribute__ ((packed));
> };
>
> _Note:_ The 4.1, 4.2 and 4.3 series of GCC ignore the 'packed'
> attribute on bit-fields of type 'char'. This has been fixed in GCC
> 4.4 but the change can lead to differences in the structure layout.
> See the documentation of '-Wpacked-bitfield-compat' for more
> information.
>
> I was under the impression that the only thing the packed attribute
> did was to change the structure layout. So I was about to reply with
> a message saying, "How does this do anything? The structure is
> already packed, so isn't this a no-op?"
>
> But I did the experiment, and your suggestion worked.... so I then
> started digging for documentation for this being guaranteed behavior
> for gcc and clang.... and I found nothing but blog posts. One of them
> is by Roland Dreir (infiniband Linux hacker):
>
> http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
>
> which does state:
>
> But adding __attribute__((packed)) goes further than just telling
> gcc that it shouldn’t add padding — it also tells gcc that it can’t
> make any assumptions about the alignment of accesses to structure
> members
>
> But I wouldn't exactly call this documented behavior on the part of
> GCC. I guess we could hope that behavior that apparently has been
> around for 15+ years is probably not going to change on us, but I
> would really prefer something in writing. :-)
>
>
>
> - Ted
>
> P.S. Roland's blog goes on to say:
>
> ... And this leads to disastrously bad code on some architectures.
>
> and has some examples of some really terrible codegen on ia64 and
> sparc64.

2021-05-04 20:42:18

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 03:53:48PM -0400, Theodore Ts'o wrote:
> On Tue, May 04, 2021 at 12:14:20PM -0700, Eric Biggers wrote:
> > On Tue, May 04, 2021 at 10:55:44AM -0700, harshad shirwadkar wrote:
> > > > However, wouldn't it be easier to just add __attribute__((packed)) to the
> > > > definition of struct journal_block_tag_t?
> > > While we know that journal_block_tag_t can be unaligned, our code
> > > should still ensure that we are reading this struct in an
> > > alignment-safe way (like Ted's patch does). IIUC, using
> > > __attribute__((packed)) might result in us keeping the door open for
> > > unaligned accesses in future. If someone tries to read 4 bytes
> > > starting at &journal_block_tag_t->t_flags, with attribute packed,
> > > UBSAN won't complain but this may still cause issues on some
> > > architectures.
> >
> > I don't understand your concern here. Accesses to a packed struct are assumed
> > to be unaligned -- that's why I suggested it. The packed attribute is pretty
> > widely used to implement unaligned accesses in C (as an alternative to memcpy()
> > or explicit byte-by-byte accesses, both of which also work, though the latter
> > seems to run into an UBSAN bug in this case).
>
> So part of the problem is that documentation for
> __attribute__((packed)) is terrible. From the GCC documentation:
>
> 'packed'
> The 'packed' attribute specifies that a structure member should
> have the smallest possible alignment--one bit for a bit-field and
> one byte otherwise, unless a larger value is specified with the
> 'aligned' attribute. The attribute does not apply to non-member
> objects.
>
> For example in the structure below, the member array 'x' is packed
> so that it immediately follows 'a' with no intervening padding:
>
> struct foo
> {
> char a;
> int x[2] __attribute__ ((packed));
> };
>
> _Note:_ The 4.1, 4.2 and 4.3 series of GCC ignore the 'packed'
> attribute on bit-fields of type 'char'. This has been fixed in GCC
> 4.4 but the change can lead to differences in the structure layout.
> See the documentation of '-Wpacked-bitfield-compat' for more
> information.
>
> I was under the impression that the only thing the packed attribute
> did was to change the structure layout. So I was about to reply with
> a message saying, "How does this do anything? The structure is
> already packed, so isn't this a no-op?"
>
> But I did the experiment, and your suggestion worked.... so I then
> started digging for documentation for this being guaranteed behavior
> for gcc and clang.... and I found nothing but blog posts. One of them
> is by Roland Dreir (infiniband Linux hacker):
>
> http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
>
> which does state:
>
> But adding __attribute__((packed)) goes further than just telling
> gcc that it shouldn’t add padding — it also tells gcc that it can’t
> make any assumptions about the alignment of accesses to structure
> members
>
> But I wouldn't exactly call this documented behavior on the part of
> GCC. I guess we could hope that behavior that apparently has been
> around for 15+ years is probably not going to change on us, but I
> would really prefer something in writing. :-)
>

There is a lot of code that relies on __attribute__((packed)) setting all the
field alignments to 1, including one of the implementations of get_unaligned_*
in the Linux kernel. But yes, it's a gcc/clang extension that's not well
documented. For individual accesses like get_unaligned_le32(), I'd recommend
the memcpy() approach instead, as it's standard and works well with all modern
compilers and architectures. But for a whole struct that can be misaligned,
__attribute__((packed)) seems to be the only solution that doesn't require
annotating every individual field access. Adding the aligned(1) attribute (in
addition to packed) can also make the intent clearer, although it's not actually
necessary.

>
> P.S. Roland's blog goes on to say:
>
> ... And this leads to disastrously bad code on some architectures.
>
> and has some examples of some really terrible codegen on ia64 and
> sparc64.

I don't speak ia64 or sparc64, but it looks like gcc just generated the code
that is needed for unaligned accesses. So it's only "bad" when that's not
actually wanted/needed.

- Eric

2021-05-04 20:45:33

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 01:14:22PM -0700, harshad shirwadkar wrote:
> I see thanks for the explanation. I quickly tried it too and saw that
> UBSAN warnings went away but I got compiler warnings
> "recovery.c:413:27: warning: taking address of packed member
> 't_blocknr_high' of class or structure 'journal_block_tag_s' may
> result in an unaligned pointer value [-Waddress-of-packed-member]".
> These compiler warnings seem to be added in [1].
>
> These warnings make me think that de-referencing a member of a packed
> struct is still not safe. My concern is this - If we define
> journal_block_tag_t as a packed struct AND if we have following unsafe
> code, then we won't see UBSAN warnings and the following unaligned
> accesses would go unnoticed. That may not go well on certain
> architectures.
>
> j_block_tag_t *unaligned_ptr;
>
> flags = unaligned_ptr->t_flags;
>
> It looks like if the compiler doesn't support
> -Waddress-of-packed-member [1], we may not even see these warnings, we
> won't see UBSAN warnings and the unaligned accesses may cause problems
> on the architectures that you mentioned.
>
> In other words, what I'm trying to say is that while
> __atribute__((packed)) would silence UBSAN warnings (and we should do
> it), it's still not sufficient to ensure that our code doesn't do
> unaligned accesses to the struct in question. Does that make sense?
>
> - Harshad

No, 'flags = unaligned_ptr->t_flags' is fine, provided that unaligned_ptr is a
pointer to a struct with the packed attribute. What -Waddress-of-packed-member
will warn about is if you do something like &unaligned_ptr->t_flags to get a
pointer directly to the t_flags field, as such pointers can then be incorrectly
used for misaligned accesses.

If we really don't want to use __attribute__((packed)) that is fine, but then
we'll need to remember to use an unaligned accessor *every* field access (except
for bytes), which seems harder to me -- and the compiler won't warn when one of
these is missing. (They can only be detected at runtime using UBSAN.)

- Eric

2021-05-04 21:11:19

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 01:45:02PM -0700, Eric Biggers wrote:
>
> No, 'flags = unaligned_ptr->t_flags' is fine, provided that unaligned_ptr is a
> pointer to a struct with the packed attribute. What -Waddress-of-packed-member
> will warn about is if you do something like &unaligned_ptr->t_flags to get a
> pointer directly to the t_flags field, as such pointers can then be incorrectly
> used for misaligned accesses.

Yeah, the warnings appear if you apply my patch, and then *also* add
the __attribute__((__packed__)) annotation. If you revert my patch,
and then only add the __packed__ annotation, there are no warnings,
because we're no longer taking the address of the field with a packed
attribute.

Basically, what gcc (and presumably clang) is doing is it is special
casing packed_ptr->field so that the compiled code will work
regardless of the alignment of packed_ptr. This isn't documented
anywhere, but it apparently is the case. (I had assumed that it would
only generate unaligned access for those fields that are not aligned
if the structure started on an aligned boundary.)

However, if you take an address of a packed memory,

short *p = &packed_ptr->field;

.. and then later derference that pointer, the C compiler can't know
that it needs to generate the magic unaligned derferencing code when
it dereferences that pointer. So that's why that warning is there.

But if you just add __packed__ attribute, without my proposed patch,
we aren't taking the &packed_ptr->field anywhere in e2fsprogs, so
we're fine.

> If we really don't want to use __attribute__((packed)) that is fine, but then
> we'll need to remember to use an unaligned accessor *every* field access (except
> for bytes), which seems harder to me -- and the compiler won't warn when one of
> these is missing. (They can only be detected at runtime using UBSAN.)

One reason not to use the __packed__ attribute is that there are cases
where people attempt to build e2fsprogs on non-gcc/non-clang binaries.
At one point FreeBSD was trying to use pcc to build e2fsprogs IIRC.
And certainly there are people who try to build e2fsprogs on MSVC on
Windows.

So maybe the memcpy to a local copy is the better way to go, and
hopefully the C compiler will optimize away the local copy on
architectures where it is safe to do so. And in the unlikely case
that it is a performance bottleneck, we could add a -DUBSAN when
configure --enable-ubsan is in force, which switches in the memcpy
when only when ubsan is enabled.

- Ted

2021-05-04 21:31:55

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 05:10:34PM -0400, Theodore Ts'o wrote:
>
> Basically, what gcc (and presumably clang) is doing is it is special
> casing packed_ptr->field so that the compiled code will work
> regardless of the alignment of packed_ptr. This isn't documented
> anywhere, but it apparently is the case. (I had assumed that it would
> only generate unaligned access for those fields that are not aligned
> if the structure started on an aligned boundary.)

I don't think it's related to the pointer dereference per se, but rather the
compiler assigns an alignment of 1 to all fields in a packed struct (even the
field at the beginning of the struct). If you had a packed struct as a global
variable and did 'packed_struct.field', the behavior would be the same.

> > If we really don't want to use __attribute__((packed)) that is fine, but then
> > we'll need to remember to use an unaligned accessor *every* field access (except
> > for bytes), which seems harder to me -- and the compiler won't warn when one of
> > these is missing. (They can only be detected at runtime using UBSAN.)
>
> One reason not to use the __packed__ attribute is that there are cases
> where people attempt to build e2fsprogs on non-gcc/non-clang binaries.
> At one point FreeBSD was trying to use pcc to build e2fsprogs IIRC.
> And certainly there are people who try to build e2fsprogs on MSVC on
> Windows.

Is that really true, given that e2fsprogs already uses a lot of gcc/clang
extensions, including __attribute((packed))__ already?

> So maybe the memcpy to a local copy is the better way to go, and
> hopefully the C compiler will optimize away the local copy on
> architectures where it is safe to do so. And in the unlikely case
> that it is a performance bottleneck, we could add a -DUBSAN when
> configure --enable-ubsan is in force, which switches in the memcpy
> when only when ubsan is enabled.

These days the memcpy() approach does get optimized properly. armv6 and armv7
with gcc used to be a notable exception, but it got fixed in gcc 6
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67366).

- Eric

2021-05-06 19:47:19

[permalink] [raw]

Subject: [PATCH -v2] 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 <[email protected]>
---

Changes from v1:
- Copy the structures to assure alignment instead of using a
byte-by-byte access because taking a pointer of a structure
member since that could trigger UBSAN complaints.

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.0

2021-05-07 01:07:12

[permalink] [raw]

Subject: Re: [PATCH -v2] e2fsck: fix portability problems caused by unaligned accesses

Hi Ted,

Thanks for working on this. The patch looks good, but there are some
more places where we need to apply the fix for alignment. I am working
on creating another patch on top of this patch to handle that. So,
I'll send out your patch + extra patch that fixes remaining alignment
issues soon. I have inlined places that also need memcpy fixes:

On Thu, May 6, 2021 at 11:30 AM Theodore Ts'o <[email protected]> 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
>
> Addresses-Debian-Bug: #987641
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
>
> Changes from v1:
> - Copy the structures to assure alignment instead of using a
> byte-by-byte access because taking a pointer of a structure
> member since that could trigger UBSAN complaints.
>
> 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);
The above line where we dereference tl, this can also result in
unaligned accesses. So, we need to do memcpy stuff for "tl" too.
Changing all access of tl to a memcpy-ed local variable is itself a
big change which I'll send along with your patch.

>
> - fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl);
> + memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));

If we do the memcpy fix here, ext4_fc_tag_val macro becomes unusable -
since at this point that macro just does (tl + 1), which will fail on
a memcpy-ed version of "tl".

Interesting bit is that even the kernel does these kinds of accesses
in the recovery code. I have a suspicion that these unaligned accesses
are the reason why you see failures on sparc?

Thanks,
Harshad
>
> - 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.0
>

2021-05-07 02:18:19

[permalink] [raw]

Subject: Re: [PATCH -v2] e2fsck: fix portability problems caused by unaligned accesses

On Thu, May 06, 2021 at 04:30:39PM -0700, harshad shirwadkar wrote:
> > -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);
> The above line where we dereference tl, this can also result in
> unaligned accesses. So, we need to do memcpy stuff for "tl" too.
> Changing all access of tl to a memcpy-ed local variable is itself a
> big change which I'll send along with your patch.

Ah, I didn't realize that 16-bit shorts could be misaligned. With the
jbd2 checksum v2, that wasn't an issue, since the entries were always
an even number of bytes, so it was only the 32-bit accesses that were
problematic. But yeah, if the dentry is an odd number of bytes, we're
not padding that out.

> >
> > - fcd = (struct ext4_fc_dentry_info *)ext4_fc_tag_val(tl);
> > + memcpy(&fcd, ext4_fc_tag_val(tl), sizeof(fcd));
>
> If we do the memcpy fix here, ext4_fc_tag_val macro becomes unusable -
> since at this point that macro just does (tl + 1), which will fail on
> a memcpy-ed version of "tl".

Well, we can make define them as:

/* Get length of a particular tlv */
static inline int ext4_fc_tag_len(struct ext4_fc_tl *tl)
{
__u8 *p = (__u8 *) tl;

return *cp + (*(cp+1) << 8);
}

/* Get a pointer to "value" of a tlv */
static inline __u8 *ext4_fc_tag_val(struct ext4_fc_tl *tl)
{
__u8 *p = ((__u8 *) tl) + 2;

return *cp + (*(cp+1) << 8);
}

> Interesting bit is that even the kernel does these kinds of accesses
> in the recovery code. I have a suspicion that these unaligned accesses
> are the reason why you see failures on sparc?

Yeah, it could be that arm allows unaligned 16-bit dereferences, which
is why this isn't blowing up on armhf and armel.

But at least with this patch, armhf and armel builds aren't blowing
up, and UBSAN is happy. (Although I wonder why UBSAN isn't
complaining about the unaligned 16-bit dereferences.)

- Ted

2021-05-07 07:52:59

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Tue, May 04, 2021 at 02:30:50PM -0700, Eric Biggers wrote:
> > So maybe the memcpy to a local copy is the better way to go, and
> > hopefully the C compiler will optimize away the local copy on
> > architectures where it is safe to do so. And in the unlikely case
> > that it is a performance bottleneck, we could add a -DUBSAN when
> > configure --enable-ubsan is in force, which switches in the memcpy
> > when only when ubsan is enabled.
>
> These days the memcpy() approach does get optimized properly. armv6 and armv7
> with gcc used to be a notable exception, but it got fixed in gcc 6
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67366).
>

Just to be clear (looking at the latest patches on the list which are copying
whole structs), by "the memcpy() approach does get optimized properly", I meant
that it gets optimized properly in implementations of get_unaligned_le16(),
get_unaligned_le32(), put_unaligned_le32(), etc., where a single word (or less
than a word) is loaded or stored. I don't know how reliably the compilers will
optimize out the copy if you memcpy() a whole struct instead of a single word.

Even if they don't optimize it out, I don't expect that it would be a
performance problem in this context, so it's probably still fine to solve the
problem. But I just wanted to clarify what I meant here.

- Eric

2021-05-07 17:33:01

[permalink] [raw]

Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

On Thu, May 06, 2021 at 11:45:09PM -0700, Eric Biggers wrote:
> Just to be clear (looking at the latest patches on the list which are copying
> whole structs), by "the memcpy() approach does get optimized properly", I meant
> that it gets optimized properly in implementations of get_unaligned_le16(),
> get_unaligned_le32(), put_unaligned_le32(), etc., where a single word (or less
> than a word) is loaded or stored. I don't know how reliably the compilers will
> optimize out the copy if you memcpy() a whole struct instead of a single word.
>
> Even if they don't optimize it out, I don't expect that it would be a
> performance problem in this context, so it's probably still fine to solve the
> problem. But I just wanted to clarify what I meant here.

For the most recent patch that sent out, we really needed to copy out
the whole structure since we're then passing it to ext2fs library
functions. I agree that it's not likely going to be a performance
problem, and at this point, I'm more concerned about code clarity and
correctness.

Especially since apparently the problems which Harshad's change and my
most recent commit addressed were not picked up by UBSAN (either using
gcc or clang), --- and IMHO they really should have. So we can't
count on UBSAN to find all possible alignment problems.

Lesson learned, before I do future releases, I should do a build and
"make check" on a armhf chroot running on a arm-64 machine, as well as
on a sparc64 machine, since these seem to be the most sensitive to
alignment issues. And if I miss anything, fortunately Debian's
autobuilders on a large cross-section of architectures will catch them
since we run the regression test suite as part of the package build.

- Ted

P.S. Harshad, could you prepare patches to kernel files in ext4 and
jbd2 to make similar alignment portability fixes? Thanks!!

2021-05-07 17:34:55