Date:   Tue, 4 May 2021 13:14:22 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     "Theodore Ts'o" <tytso@....edu>
Cc:     Eric Biggers <ebiggers@...nel.org>,
        Andreas Dilger <adilger@...ger.ca>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Harshad Shirwadkar <harshads@...gle.com>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

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 <tytso@....edu> 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.  :-)
>
>
>
>                                                 - 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.

Powered by blists - more mailing lists