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

[David Laight]

On Sat, 24 Oct 2026 13:05:19 +0200
"Arnd Bergmann" <arnd@xxxxxxxx> wrote:

> On Sat, Oct 24, 2026, at 10:32, David Laight wrote:
> > On Fri, 23 Oct 2026 23:49:50 +0200 "Arnd Bergmann" <arnd@xxxxxxxx> wrote:  
> >> On Tue, Oct 20, 2026, at 20:07, David Laight wrote:  
> >> > On Tue, 20 Oct 2026 09:32:09 -0700 Ian Rogers <irogers@xxxxxxxxxx> 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.

The code rather relies on the compiler not splitting a volatile access.
But it is allowed to do that (eg 64bit accesses on 32bit) so it
may be allowed all the time and the kernel just hopes it never does.

The whole thing is a bear pit :-)

> 
> 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.

Or the implementation currently chooses to honour the lower alignment
for punned pointers.

The standard doesn't allow many casts at all. IIRC you can cast a pointer
to 'void *' and then back to its original type, and you might be able to
cast a 'pointer to struct' to a pointer to its first element.
Remember it has to allow for word addressable systems where the offset
of a char in the word is held in (say) the top 2 bits.
(I doubt any such systems still exist...)

> 
> > 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 had the compiler unroll loops on x86 and had a 'sum += *buf++;'
checksum crash when the buffer (mmap() of a file) was misaligned.

> 
> 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.

Jeepers that is horrid.
The simple:
	for (off = 0; off < cnt; off += 8)
		mci_fifo_writeq(host->fifo_reg, get_unaligned_u64(buf + off));
will generate better code.
The compiler will usually optimise out 'off', decrement 'cnt' and
increment 'buf'. It does that even when you try to use negative offsets
from the end of the buffer - which generates the best code on x86.
(And you might want to copy host->fifo_reg into a local.)

It might be worth optimising for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
being unset and the buffer being aligned (to sizeof long, not 8).
But that isn't the conditional there.	

> 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.

I think it could be inside the cast - the same as the struct definition.
But that is not really any different to the packed struct.

David

> 
>        Arnd