Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 4 May 2021 09:46:19 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     harshad shirwadkar <harshadshirwadkar@gmail.com>,
        Andreas Dilger <adilger@dilger.ca>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Harshad Shirwadkar <harshads@google.com>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned
 accesses
Message-ID: <YJF6W7WHZBcVZexU@gmail.com>
References: <20210504031024.3888676-1-tytso@mit.edu>
 <8E9C71E8-FE5F-4CB8-BA62-8D8895DCA92A@dilger.ca>
 <CAD+ocbx9STMGrE0xkHtR8J_c_TgMEz1A6MmNOQyrQtakoZjq3Q@mail.gmail.com>
 <YJFQ20rLK16rise2@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YJFQ20rLK16rise2@mit.edu>
Precedence: bulk

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