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:   Fri, 7 May 2021 11:56:22 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Eric Biggers <ebiggers@kernel.org>
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: <YJVjJoI8sX531AL2@mit.edu>
References: <YJFQ20rLK16rise2@mit.edu>
 <YJF6W7WHZBcVZexU@gmail.com>
 <CAD+ocby+01k9kx3-gEY_z+Ub9GFxi=AwxRS4Ax5-HUFDrVkT0w@mail.gmail.com>
 <YJGdDHLcYuRajhsb@gmail.com>
 <YJGmTNIHixCLiKok@mit.edu>
 <CAD+ocbwS9h4knUbhiXFUicvi-PwKSnPdF7hrZUhbg1MkzbDmrw@mail.gmail.com>
 <YJGyTjYKcEkx+fQq@gmail.com>
 <YJG4SrJ/ZEjv3Ha0@mit.edu>
 <YJG9CjVXKkha57RU@gmail.com>
 <YJTh9T3sgdFFE7fM@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YJTh9T3sgdFFE7fM@sol.localdomain>
Precedence: bulk

On Thu, May 06, 2021 at 11:45:09PM -0700, Eric Biggers wrote:
> Just to be clear (looking at the latest patches on the list which are copying
> whole structs), by "the memcpy() approach does get optimized properly", I meant
> that it gets optimized properly in implementations of get_unaligned_le16(),
> get_unaligned_le32(), put_unaligned_le32(), etc., where a single word (or less
> than a word) is loaded or stored.  I don't know how reliably the compilers will
> optimize out the copy if you memcpy() a whole struct instead of a single word.
> 
> Even if they don't optimize it out, I don't expect that it would be a
> performance problem in this context, so it's probably still fine to solve the
> problem.  But I just wanted to clarify what I meant here.

For the most recent patch that sent out, we really needed to copy out
the whole structure since we're then passing it to ext2fs library
functions.  I agree that it's not likely going to be a performance
problem, and at this point, I'm more concerned about code clarity and
correctness.

Especially since apparently the problems which Harshad's change and my
most recent commit addressed were not picked up by UBSAN (either using
gcc or clang), --- and IMHO they really should have.  So we can't
count on UBSAN to find all possible alignment problems.

Lesson learned, before I do future releases, I should do a build and
"make check" on a armhf chroot running on a arm-64 machine, as well as
on a sparc64 machine, since these seem to be the most sensitive to
alignment issues.  And if I miss anything, fortunately Debian's
autobuilders on a large cross-section of architectures will catch them
since we run the regression test suite as part of the package build.

					- Ted

P.S.  Harshad, could you prepare patches to kernel files in ext4 and
jbd2 to make similar alignment portability fixes?   Thanks!!