Re: [Regression] [PATCH v5 2/4] vdso: Switch get/put unaligned from packed struct to memcpy

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Oct 19, 2026, at 19:04, Ian Rogers wrote:
> On Mon, Oct 19, 2026 at 9:28 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Oct 19, 2026, at 17:10, Ian Rogers wrote:
>> > On Mon, Oct 19, 2026 at 5:37 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> As I wrote, we need the packed struct version at least on mips32,
>> mips64, openrisc, riscv32, riscv64, sh4, sparc32, and sparc64 to avoid
>> the external library calls the same as on armv5, but I would prefer
>> to just always use that in the kernel. My guess is that this list
>> is actually the same as the architectures that lack unaligned word
>> access instructions in hardware, and armv5 is certainly not
>> a special case.
>
> Is it more than conjecture that these architectures have regressed? I
> have a hard time believing the codegen on say riscv64 is lagging
> aarch64. When the series originally landed the issue was looked at and
> workarounds were added, for example, to PA-RISC.

I tried compiling a trivial function that calls get_unaligned() and
looked at the assembler output for all architectures. The list is
everything that does a function call to memcpy() rather than open-coding
the data access.

By default (but depending on the target CPU), riscv64-gcc avoids unaligned
access because this is not a required hardware feature and may be
emulated by machine mode firmware even when it does work, so a
function call is cheaper than a trap.
On arm64, armv7 and most other modern architectures, unaligned non-atomic
word access is always faster than byte-at-a-time, so the compiler
defaults to -mno-strict-align or -munaligned-access.

>> For 7.4 (and stable kernels) reverting the in-kernel header seems
>> best to me, and then we can think about what to use for the tools.
>>
>> Looking at the uses of get_unaligned() in the tools directory,
>> I notice that at least some of them should just not exist, e.g.
>> struct debug_entry in tools/perf/util/jitdump.h is just
>> needs a __packed attribute to correctly describe what it does,
>> and tools/testing/cxl/test/mem.c does not need the unaligned
>> helpers at all because the structs are all packed already.
>
> There was supposed to be an 8-byte alignment requirement for perf
> events and unfortunately some events broke this and we added __packed
> as a workaround. The unaligned accesses wouldn't trip a sanitizer in
> our tests because we sadly lack testing on old perf.data files. There
> is a difference in using __packed as an attribute and using type
> punning to achieve an unaligned access.

Yes, that was my point: if the unaligned data is annotated correctly
as __packed, you don't need any type punning to access it -- you
can rely on the compiler to pick sensible instructions for the
target architecture, either an unaligned load/store a byte-at-a-time.

>> I'm also a bit suspicious of the usage in
>> tools/testing/selftests/bpf/test_kmods/bpf_testmod.c, which
>> looks like it may run into a corner case:
>>
>> __bpf_kfunc int bpf_kfunc_test_uninit_alias(int *out__uninit, const int *in)
>> {
>>         int value = get_unaligned(in);
>>         put_unaligned(42, out__uninit);
>>         return value;
>> }
>>
>> The current version of get_unaligned() turns into __builtin_memcpy(),
>> and the function prototype defines both arguments as pointers
>> to naturally aligned objects. Do you see anything that would
>> stop the compiler from replacing the __builtin_memcpy() with
>>  an invalid pointer dereference? This bit is probably fine
>> because of the way gcc handles __builtin_memcpy() optimization,
>> but I'm not sure if that's an implementation detail or required
>> by the C standard, especially if strict aliasing rules say
>> that the int* argument cannot overlap a misaligned int*.
>
> I agree the pointers should be aligned given their types, ubsan
> doesn't currently fail for this and waits for use of the pointer. So
> there is the specification and also what sanitizers enforce.
> For clang, when lowering from parsed C to machine code the compiler
> will use alloca to model local variables and memcpy as an intrinsic
> for aggregate copies. So:
>
>   struct point { int x, y; } a, b;
>   a = b; // Compiler will generate a builtin memcpy to copy b into a

The problem I am referring to is specifically where the object
itself is unaligned and passed into a function through a non-__packed
reference. In this case, the get_unaligned() must ensure both that
all address bits of the pointer are used and that each load/store
is done according to the target CPU definition and
-m{no-,}unaligned-access flag. We have had cases where x86_64-gcc 
optimized away the low-order pointer bits based on the psABI
guarantees, and it wouldn't surprise me if the inlined memcpy
tried to make similar assumptions in some corner cases on
-mno-unaligned-access targets.

On ARMv5 and similar architectures this would still appear to
work correctly but cause trapped loads that would be absolutely
detrimental to performance, much worse than a few added memcpy()
calls.

> This is why I have a hard time believing the lowering is broken on so
> many architectures. Calling memcpy to copy aggregates everywhere would
> be madness. I believe, especially given the period of time this code
> has been in the kernel, that this problem is specific to GCC and
> ARMv5. Clearly fixing a bug in GCC and ARMv5 should be done, but in
> the meantime, a config option workaround in the kernel is pragmatic.

I'm not sure what you expect the compiler to do on ARMv5 and other
targets with -mno-unaligned-access, or why you'd consider calling
memcpy() a bug when that is literally what the code asks for and
there is no easy way to convert it to word load/store.

Comparing the gcc and clang output in https://godbolt.org/z/r8aq4n87z
it's clear that the gcc version is at least shorter but depending
on the CPU and caller it can also be faster, e.g. when the data
is usually naturally aligned and the memcpy() library function
does a single store while the clang version fails to write-combine
the multiple bytes being written in the wrong order.

       Arnd