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 [off-list ref] wrote:

On Tue, May 04, 2021 at 12:14:20PM -0700, Eric Biggers wrote:

quoted

On Tue, May 04, 2021 at 10:55:44AM -0700, harshad shirwadkar wrote:

quoted

quoted

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.