Thread (20 messages) 20 messages, 4 authors, 2021-05-07

Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

From: Eric Biggers <ebiggers@kernel.org>
Date: 2021-05-04 21:30:53

On Tue, May 04, 2021 at 05:10:34PM -0400, Theodore Ts'o wrote:
Basically, what gcc (and presumably clang) is doing is it is special
casing packed_ptr->field so that the compiled code will work
regardless of the alignment of packed_ptr.  This isn't documented
anywhere, but it apparently is the case.  (I had assumed that it would
only generate unaligned access for those fields that are not aligned
if the structure started on an aligned boundary.)
I don't think it's related to the pointer dereference per se, but rather the
compiler assigns an alignment of 1 to all fields in a packed struct (even the
field at the beginning of the struct).  If you had a packed struct as a global
variable and did 'packed_struct.field', the behavior would be the same.
quoted
If we really don't want to use __attribute__((packed)) that is fine, but then
we'll need to remember to use an unaligned accessor *every* field access (except
for bytes), which seems harder to me -- and the compiler won't warn when one of
these is missing.  (They can only be detected at runtime using UBSAN.)
One reason not to use the __packed__ attribute is that there are cases
where people attempt to build e2fsprogs on non-gcc/non-clang binaries.
At one point FreeBSD was trying to use pcc to build e2fsprogs IIRC.
And certainly there are people who try to build e2fsprogs on MSVC on
Windows.
Is that really true, given that e2fsprogs already uses a lot of gcc/clang
extensions, including __attribute((packed))__ already?
So maybe the memcpy to a local copy is the better way to go, and
hopefully the C compiler will optimize away the local copy on
architectures where it is safe to do so.  And in the unlikely case
that it is a performance bottleneck, we could add a -DUBSAN when
configure --enable-ubsan is in force, which switches in the memcpy
when only when ubsan is enabled.
These days the memcpy() approach does get optimized properly.  armv6 and armv7
with gcc used to be a notable exception, but it got fixed in gcc 6
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67366).

- Eric
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help