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

[Ian Rogers]

On Mon, Oct 19, 2026 at 2:00 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Mon, Oct 19, 2026, at 19:04, Ian Rogers wrote:
> > On Mon, Oct 19, 2026 at 9:28 AM Arnd Bergmann <arnd@xxxxxxxx> 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@xxxxxxxxxxxxxx> 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.

Is there a reason that the compiler's instruction selection can't make
the same decision for the memcpy case as the __packed case? We can
work around the compiler bug with a config option.
The intent with using __builtin_memcpy, as opposed to memcpy, is to
avoid a library call.
It looks like all the unaligned code is using void* so I'm not sure
where the low bit concerns are stemming from. If anything, type
punning seems to be a worse problem, but the hope is that the __packed
attribute avoids it. I know this assumption makes compiler people
uncomfortable and memcpy is their preferred go to.

Thanks,
Ian

>        Arnd