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.
