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
