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

[Eric Biggers <ebiggers@kernel.org>]

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