linux-kernel - Re: [PATCH] atomic: Specify natural alignment for atomic

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: <30a55f56-93c2-4408-b1a5-5574984fb45f@linux.dev>
Date: Mon, 25 Aug 2025 19:36:51 +0800
From: Lance Yang <lance.yang@...ux.dev>
To: Finn Thain <fthain@...ux-m68k.org>
Cc: akpm@...ux-foundation.org, geert@...ux-m68k.org,
 linux-kernel@...r.kernel.org, mhiramat@...nel.org, oak@...sinkinet.fi,
 peterz@...radead.org, stable@...r.kernel.org, will@...nel.org,
 Lance Yang <ioworker0@...il.com>, linux-m68k@...ts.linux-m68k.org
Subject: Re: [PATCH] atomic: Specify natural alignment for atomic_t



On 2025/8/25 19:19, Lance Yang wrote:
> Thanks for digging deeper!
> 
> On 2025/8/25 18:49, Finn Thain wrote:
>>
>> [Belated Cc linux-m68k...]
>>
>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>
>>>
>>> On 2025/8/25 14:17, Finn Thain wrote:
>>>>
>>>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>>>
>>>>>
>>>>> What if we squash the runtime check fix into your patch?
>>>>
>>>> Did my patch not solve the problem?
>>>
>>> Hmm... it should solve the problem for natural alignment, which is a
>>> critical fix.
>>>
>>> But it cannot solve the problem of forced misalignment from drivers
>>> using #pragma pack(1). The runtime warning will still trigger in those
>>> cases.
>>>
>>> I built a simple test module on a kernel with your patch applied:
>>>
>>> ```
>>> #include <linux/module.h>
>>> #include <linux/init.h>
>>>
>>> struct __attribute__((packed)) test_container {
>>>      char padding[49];
>>>      struct mutex io_lock;
>>> };
>>>
>>> static int __init alignment_init(void)
>>> {
>>>      struct test_container cont;
>>>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
>>> long)&cont.io_lock % 4);
>>>      return 0;
>>> }
>>>
>>> static void __exit alignment_exit(void)
>>> {
>>>      pr_info("Module unloaded\n");
>>> }
>>>
>>> module_init(alignment_init);
>>> module_exit(alignment_exit);
>>> MODULE_LICENSE("GPL");
>>> MODULE_AUTHOR("x");
>>> MODULE_DESCRIPTION("x");
>>> ```
>>>
>>> Result from dmesg:
>>> [Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1
>>>
>>
>> Thanks for sending code to illustrate your point. Unfortunately, I was 
>> not
>> able to reproduce your results. Tested on x86, your test module shows no
>> misalignment:
>>
>> [131840.349042] io_lock address offset mod 4: 0
>>
>> Tested on m68k I also get 0, given the patch at the top of this thread:
>>
>> [    0.400000] io_lock address offset mod 4: 0
>>
>>>
>>> As we can see, a packed struct can still force the entire mutex object
>>> to an unaligned address. With an address like this, the WARN_ON_ONCE can
>>> still be triggered.
> 
>>
>> I don't think so. But there is something unexpected going on here -- the
>> output from pahole appears to say io_lock is at offset 49, which seems to
>> contradict the printk() output above.
> 
> Interesting! That contradiction is the key. It seems we're seeing different
> compiler behaviors.
> 
> I'm on GCC 14.2.0 (Debian 14.2.0-19), and it appears to be strictly 
> respecting
> the packed attribute.
> 
> So let's print something more:
> 
> ```
> #include <linux/module.h>
> #include <linux/init.h>
> 
> struct __attribute__((packed)) test_container {
>      char padding[49];
>      struct mutex io_lock;
> };
> 
> static int __init alignment_init(void)
> {
>      struct test_container cont;
>      pr_info("Container base address      : %px\n", &cont);
>      pr_info("io_lock member address      : %px\n", &cont.io_lock);
>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
> long)&cont.io_lock % 4);
>      return 0;
> }
> 
> static void __exit alignment_exit(void)
> {
>      pr_info("Module unloaded\n");
> }
> 
> module_init(alignment_init);
> module_exit(alignment_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("x");
> MODULE_DESCRIPTION("x");
> ```
> 
> Result from dmesg:
> 
> ```
> [Mon Aug 25 19:15:33 2025] Container base address      : ff1100063570f840
> [Mon Aug 25 19:15:33 2025] io_lock member address      : ff1100063570f871
> [Mon Aug 25 19:15:33 2025] io_lock address offset mod 4: 1
> ```
> 
> io_lock is exactly base + 49, resulting in misalignment.
> 
> Seems like your compiler is cleverly re-aligning the whole struct on the
> stack, but we can't rely on that behavior, as it's not guaranteed across
> all compilers or versions. wdyt?


Same here, using a global static variable instead of a local one. The result
is consistently misaligned.

```
#include <linux/module.h>
#include <linux/init.h>

static struct __attribute__((packed)) test_container {
     char padding[49];
     struct mutex io_lock;
} cont;

static int __init alignment_init(void)
{
     pr_info("Container base address      : %px\n", &cont);
     pr_info("io_lock member address      : %px\n", &cont.io_lock);
     pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
long)&cont.io_lock % 4);
     return 0;
}

static void __exit alignment_exit(void)
{
     pr_info("Module unloaded\n");
}

module_init(alignment_init);
module_exit(alignment_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("x");
MODULE_DESCRIPTION("x");
```

Result from dmesg:

```
[Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
[Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
[Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
```