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
