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] [day] [month] [year] [list]

Message-Id: <da6cf99f-6da9-4cf7-9028-79580a8d7b2a@app.fastmail.com>
Date: Tue, 23 Sep 2025 21:11:42 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Finn Thain" <fthain@...ux-m68k.org>
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, Sep 23, 2025, at 10:05, Finn Thain wrote:
> On Tue, 23 Sep 2025, Arnd Bergmann wrote:
>> On Tue, Sep 23, 2025, at 08:28, Finn Thain wrote:
>>
>> Maybe we should just change __alignof__(unsigned long long) to a plain 
>> '8' here and make that the minimum alignment everywhere, same as the 
>> atomic64_t alignment change.
>> 
>
> It would be better (less wasteful of memory) to specify the alignment 
> parameter to kmem_cache_create() only at those call sites where it 
> matters.

I think that's the idea: __alignof__(unsigned long long) is the
alignment that the ABI requires for allocating arbitrary data
structures that may contain 64-bit integers, so this is already
the minimum. Architectures that may have noncoherent DMA master
devices often need to raise the ARCH_KMALLOC_MINALIGN to the cache
line size but leave the ARCH_SLAB_MINALIGN at the ABI-required
minimum value. Any caller that needs a higher alignment than the
ABI minimum needs to specify that.

>> Alternatively, we can keep the __alignof__ here in order to reduce 
>> padding on architectures with a default 4-byte alignment for __u64, but 
>> then override ARCH_SLAB_MINALIGN and ARCH_KMALLOC_MINALIGN on m68k to be 
>> '4' instead of '2'.
>> 
>
> Raising that to 4 would probably have little or no effect (for m68k or any 
> other arch). Back when I prepared the RFC patch series, I instrumented 
> create_cache() in mm/slab_common.c, and those caches that were allocated 
> at boot (for my usual minimal m68k .config) were already aligned to 4 
> bytes or 16.

Ok

> Also, increasing ARCH_SLAB_MINALIGN to 8 didn't solve the VFS/TTY layer 
> problem I have with CONFIG_DEBUG_ATOMIC on m68k. So the culprit is not the 
> obvious suspect (a kmem cache of objects with atomic64_t members). There's 
> some other allocator at work and it's aligning objects to 2 bytes not 4.

In that case, my guess would be something that concatenates structures
in some variant of

struct s1 {
     int a;
     short b;
     int rest[];
};

struct s2 {
     atomic_t a;
};

     struct s1 *obj1 = kmalloc(sizeof(struct s1) + sizeof(struct s2), GFP_KERNEL);
     struct s2 *obj2 = (void*)&obj1[1];

which causes problems because s2 has a larger alignment than s1.
We've had problems with DMA to structures like this in the past.

The more common construct that does

struct s1 {
     short a;
     struct s2;
};

should not produce misaligned members in the inner struct,
unless the outer one has a __packed attribute. Unfortunately
we disabled -Wpacked-not-aligned, which would otherwise catch
that problem.

     Arnd