linux-kernel - Re: [RFC v2 2/3] atomic: Specify alignment for atomic_t and atomic64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60325e45-e4a7-d0cf-ba28-a1a811f9a890@linux-m68k.org>
Date: Wed, 1 Oct 2025 11:03:16 +1000 (AEST)
From: Finn Thain <fthain@...ux-m68k.org>
To: Arnd Bergmann <arnd@...db.de>
cc: Geert Uytterhoeven <geert@...ux-m68k.org>, 
    Peter Zijlstra <peterz@...radead.org>, Will Deacon <will@...nel.org>, 
    Andrew Morton <akpm@...ux-foundation.org>, 
    Boqun Feng <boqun.feng@...il.com>, Jonathan Corbet <corbet@....net>, 
    Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org, 
    Linux-Arch <linux-arch@...r.kernel.org>, linux-m68k@...r.kernel.org, 
    Lance Yang <lance.yang@...ux.dev>
Subject: Re: [RFC v2 2/3] atomic: Specify alignment for atomic_t and
 atomic64_t


On Tue, 30 Sep 2025, Arnd Bergmann wrote:

> On Tue, Sep 30, 2025, at 04:18, Finn Thain wrote:
> >
> > It turned out that the problem wasn't dynamic allocations, it was a 
> > local variable in the core locking code (kernel/locking/rwsem.c): a 
> > misaligned long used with an atomic operation (cmpxchg). To get 
> > natural alignment for 64-bit quantities, I had to align other local 
> > variables as well, such as the one in ktime_get_real_ts64_mg() that's 
> > used with atomic64_try_cmpxchg(). The atomic_t branch in my github 
> > repo has the patches I wrote for that.
> 
> It looks like the variable you get the warning for is not even the 
> atomic64_t but the 'old' argument to atomic64_try_cmpxchg(), at least in 
> some of the cases you found if not all of them.
> 
> I don't see where why there is a requirement to have that aligned at 
> all, even if we do require the atomic64_t to be naturally aligned, and I 
> would expect the same warning to hit on x86-32 and the other 
> architectures with 4-byte alignment of u64 variable on stack and .data.
> 

Right -- there's only one memory operand in a CAS instruction on m68k, and 
there's only one pointer in the C implementation in asm-generic.

> > To silence the misalignment WARN from CONFIG_DEBUG_ATOMIC, for 64-bit 
> > atomic operations, for my small m68k .config, it was also necesary to 
> > increase ARCH_SLAB_MINALIGN to 8. However, I'm not advocating a 
> > ARCH_SLAB_MINALIGN increase, as that wastes memory.
> 
> Have you tried to quantify the memory waste here?

I think it's entirely workload dependent. The memory efficiency question 
comes down to the misalignment distance as a proportion of the size of the 
allocation.

> I assume that most slab allocations are already 8-byte aligned, at least 
> kmalloc() with size>4, while custom caches are usually done for larger 
> structures where an extra average of 2 bytes per allocation may not be 
> that bad.
> 
> > diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> > index 402a999a0d6b..cd569a87c0a8 100644
> > --- a/include/linux/instrumented.h
> > +++ b/include/linux/instrumented.h
> > @@ -68,7 +68,7 @@ static __always_inline void 
> > instrument_atomic_read(const volatile void *v, size_
> >  {
> >  	kasan_check_read(v, size);
> >  	kcsan_check_atomic_read(v, size);
> > -	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & 
> > (size - 1)));
> > +	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & 
> > (size - 1) & 3));
> >  }
> 
> What is the alignment of stack variables on m68k? E.g. if you have a 
> function with two local variables, would that still be able to trigger 
> the check?
> 
> int f(atomic64_t *a)
> {
>      u16 pad;
>      u64 old;
>      
>      g(&pad);
>      atomic64_try_cmpxchg(a, &old, 0);
> }
> 

I assume so:

int foo(void) {
    short s;
    long long ll;
    return alignof(ll);
}

# Compilation provided by Compiler Explorer at https://godbolt.org/
foo():
        link.w %fp,#0
        moveq #2,%d0
        unlk %fp
        rts

> Since there is nothing telling the compiler that the 'old' argument to 
> atomic*_try_cmpcxchg() needs to be naturally aligned, maybe that check 
> should be changed to only test for the ABI-guaranteed alignment? I think 
> that would still be needed on x86-32.
>  

I don't know why we would check the alignment of the 'old' quantity. It's 
going to be loaded into a register before being used, right?