Re: [PATCH v2 1/2] vdso: Add VDSO_UNALIGNED_PACKED config option

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

On Sat, Oct 24, 2026, at 10:32, David Laight wrote:
> On Fri, 23 Oct 2026 23:49:50 +0200 "Arnd Bergmann" <arnd@arndb.de> wrote:
>> On Tue, Oct 20, 2026, at 20:07, David Laight wrote:
>> > On Tue, 20 Oct 2026 09:32:09 -0700 Ian Rogers <irogers@google.com> wrote:  
>>
>> This is the traditional way we have been doing the unaligned access
>> in the kernel before the change. By annotating the structure as
>> __packed, we tell the compiler that it cannot rely on alignment here,
>> so it uses whatever method it knows to access unaligned data depending
>> on the architecture and flags.
>
> That relies on the compiler not tracking the alignment through the casts.
> The only way the C language lets you get a 'foo *' pointer is taking the
> address of a 'foo' data item so it must be aligned.

I have definitely seen the reverse, where a u32 pointer dereference in
__raw_readl() gets turned into individual byte accesses because
the pointer originally came from a packed structure. This is
also bad because it breaks MMIO register access.

We also know that the packed struct version works, either because
the standard requires a punned pointer to respect the minimum
alignment, or because the implementation is smart enough to only
ever lower the alignment for punned pointers.

> So even if you cast it to another type that alignment can be assumed.
> I've seen crashes from memcpy() using word accesses when trying to
> copy from a misaligned pointer into an aligned copy of the structure.
> Now the current code must be avoiding that - but I think the compiler
> can do it.

Right, especially the memcpy() without type punning seems dangerous.
We have had several instances of code breaking on armv7 where
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set but the stp/ldp
instructions for 64-bit objects still require alignment.

I don't see any instances of the exact issue you describe, but this
one is similar and definitely broken on armv7 (adding the dw_mmc
maintainers to Cc for information):

#ifndef readq
#define __raw_writeq(__value, __reg) \
        (*(volatile u64 __force *)(__reg) = (__value))
#define __raw_readq(__reg) (*(volatile u64 __force *)(__reg))
#endif
#define mci_fifo_writeq(__value, __reg) __raw_writeq(__reg, __value)
static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
{
...
#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
        if (unlikely((unsigned long)buf & 0x7)) {
                while (cnt >= 8) {
                        u64 aligned_buf[16];
                        int len = min(cnt & -8, (int)sizeof(aligned_buf));
                        int items = len >> 3;
                        int i;
                        /* memcpy from input buffer into aligned buffer */
                        memcpy(aligned_buf, buf, len);
                        buf += len;
                        cnt -= len;
                        /* push data from aligned buffer into fifo */
                        for (i = 0; i < items; ++i)
                                mci_fifo_writeq(host->fifo_reg, aligned_buf[i]);
                }
        } else
#endif
        {
                u64 *pdata = buf;

                for (; cnt >= 8; cnt -= 8)
                        mci_fifo_writeq(host->fifo_reg, *pdata++);
                buf = pdata;
        }

The 'u64 *pdata = buf' cast is of course broken here since that
is not allowed for unaligned pointers on supported target regardless
of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. The else{...} portion
has to be written using get_unaligned_u64() here, as the #ifndef
branch should only optimize for performance and not be required
for correctness.
If '*buf' was a u64 pointer instead of void, I could see the
compiler messing this up in the way you describe, by optimizing
out the temporary array entirely.

An unrelated problem is that the mci_fifo_writeq() definition
is not portable because the volatile u64 pointer dereference
does not guarantee that the two 32-bit halves are stored
in the order that the hardware register requires. Presumably
that was what led to DW_MMC_QUIRK_FIFO64_32.

>> The reason we need the struct here is that you cannot use __packed
>> as a qualifier on a variable, only on a type or struct member.
>
> I know all about that.
> Somewhere it also says that you can only increase the alignment (I think
> of a type), but you can definitely have a type for 'u64 with 32bit
> alignment' and use it in a structure.

Right, we do rely on that in e.g. x86

typedef s64 __attribute__((aligned(4))) compat_s64;

We cannot use this in a macro though because there is no
way to define a local typedef inside of a function or
to change the alignment of pointer without the struct.

       Arnd