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