From: Lance Yang lance.yang@linux.dev
The blocker tracking mechanism assumes that lock pointers are at least 4-byte aligned to use their lower bits for type encoding.
However, as reported by Geert Uytterhoeven, some architectures like m68k only guarantee 2-byte alignment of 32-bit values. This breaks the assumption and causes two related WARN_ON_ONCE checks to trigger.
To fix this, enforce a minimum of 4-byte alignment on the core lock structures supported by the blocker tracking mechanism. This ensures the algorithm's alignment assumption now holds true on all architectures.
This patch adds __aligned(4) to the definitions of "struct mutex", "struct semaphore", and "struct rw_semaphore", resolving the warnings.
Thanks to Geert for bisecting!
Reported-by: Geert Uytterhoeven geert@linux-m68k.org Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc... Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker") Cc: stable@vger.kernel.org Signed-off-by: Lance Yang lance.yang@linux.dev --- include/linux/mutex_types.h | 2 +- include/linux/rwsem.h | 2 +- include/linux/semaphore.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/mutex_types.h b/include/linux/mutex_types.h index fdf7f515fde8..de798bfbc4c7 100644 --- a/include/linux/mutex_types.h +++ b/include/linux/mutex_types.h @@ -51,7 +51,7 @@ struct mutex { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */
#else /* !CONFIG_PREEMPT_RT */ /* diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h index f1aaf676a874..f6ecf4a4710d 100644 --- a/include/linux/rwsem.h +++ b/include/linux/rwsem.h @@ -64,7 +64,7 @@ struct rw_semaphore { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */
#define RWSEM_UNLOCKED_VALUE 0UL #define RWSEM_WRITER_LOCKED (1UL << 0) diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h index 89706157e622..ac9b9c87bfb7 100644 --- a/include/linux/semaphore.h +++ b/include/linux/semaphore.h @@ -20,7 +20,7 @@ struct semaphore { #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER unsigned long last_holder; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */
#ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER #define __LAST_HOLDER_SEMAPHORE_INITIALIZER \
Hi Lance,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/locking/core] [also build test WARNING on akpm-mm/mm-everything sysctl/sysctl-next linus/master v6.17-rc2 next-20250822] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lance-Yang/hung_task-fix-warn... base: tip/locking/core patch link: https://lore.kernel.org/r/20250823074048.92498-1-lance.yang%40linux.dev patch subject: [PATCH 1/1] hung_task: fix warnings by enforcing alignment on lock structures config: x86_64-buildonly-randconfig-001-20250823 (https://download.01.org/0day-ci/archive/20250824/202508240539.ARmC1Umu-lkp@i...) compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250824/202508240539.ARmC1Umu-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202508240539.ARmC1Umu-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from sound/soc/codecs/mt6660.c:15:
sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct mt6660_chip' is less than 8 [-Wpacked-not-aligned]
28 | }; | ^
sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
25 | struct mutex io_lock; | ^~~~~~~
vim +28 sound/soc/codecs/mt6660.h
f289e55c6eeb43 Jeff Chang 2020-01-16 19 f289e55c6eeb43 Jeff Chang 2020-01-16 20 struct mt6660_chip { f289e55c6eeb43 Jeff Chang 2020-01-16 21 struct i2c_client *i2c; f289e55c6eeb43 Jeff Chang 2020-01-16 22 struct device *dev; f289e55c6eeb43 Jeff Chang 2020-01-16 23 struct platform_device *param_dev; f289e55c6eeb43 Jeff Chang 2020-01-16 24 struct mt6660_platform_data plat_data; f289e55c6eeb43 Jeff Chang 2020-01-16 @25 struct mutex io_lock; f289e55c6eeb43 Jeff Chang 2020-01-16 26 struct regmap *regmap; f289e55c6eeb43 Jeff Chang 2020-01-16 27 u16 chip_rev; f289e55c6eeb43 Jeff Chang 2020-01-16 @28 }; f289e55c6eeb43 Jeff Chang 2020-01-16 29 #pragma pack(pop) f289e55c6eeb43 Jeff Chang 2020-01-16 30
On Sun, 24 Aug 2025, kernel test robot wrote:
All warnings (new ones prefixed by >>):
In file included from sound/soc/codecs/mt6660.c:15:
sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct mt6660_chip' is less than 8 [-Wpacked-not-aligned]
28 | }; | ^sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
25 | struct mutex io_lock; | ^~~~~~~
Misalignment warnings like this one won't work if you just pick an alignment arbitrarily i.e. to suit whatever bitfield you happen to need.
Instead, I think I would naturally align the actual locks, that is, arch_spinlock_t and arch_rwlock_t in include/linux/spinlock_types*.h.
Hi Finn, Hi all,
Thanks to the kernel test robot for finding this issue, and thank you, Finn, for the explanation!
On 2025/8/24 08:47, Finn Thain wrote:
On Sun, 24 Aug 2025, kernel test robot wrote:
All warnings (new ones prefixed by >>):
In file included from sound/soc/codecs/mt6660.c:15:sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct mt6660_chip' is less than 8 [-Wpacked-not-aligned]
28 | }; | ^sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
25 | struct mutex io_lock; | ^~~~~~~Misalignment warnings like this one won't work if you just pick an alignment arbitrarily i.e. to suit whatever bitfield you happen to need.
Yes.
The build warnings reported by the test robot are exactly the kind of unintended side effect I was concerned about. It confirms that forcing alignment on a core structure like struct mutex breaks other parts of the kernel that rely on packed structures ;)
Instead, I think I would naturally align the actual locks, that is, arch_spinlock_t and arch_rwlock_t in include/linux/spinlock_types*.h.
That's an interesting point. The blocker tracking mechanism currently operates on higher-level structures like struct mutex. Moving the type encoding down to the lowest-level locks would be a more complex and invasive change, likely beyond the scope of fixing this particular issue.
Looking further ahead, a better long-term solution might be to stop repurposing pointer bits altogether. We could add an explicit blocker_type field to task_struct to be used alongside the blocker field. That would be a much cleaner design. TODO +1 for that idea :)
So, let's drop the patch[1] that enforces alignment and go back to my initial proposal[2], which adjusts the runtime checks to gracefully handle unaligned pointers. That one is self-contained, has minimal impact, and is clearly the safer solution for now.
[1] https://lore.kernel.org/lkml/20250823074048.92498-1-lance.yang@linux.dev [2] https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
Thanks, Lance
On Sun, 24 Aug 2025, Lance Yang wrote:
On 2025/8/24 08:47, Finn Thain wrote:
On Sun, 24 Aug 2025, kernel test robot wrote:
All warnings (new ones prefixed by >>):
In file included from sound/soc/codecs/mt6660.c:15:sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct mt6660_chip' is less than 8 [-Wpacked-not-aligned]
28 | }; | ^sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
25 | struct mutex io_lock; | ^~~~~~~Misalignment warnings like this one won't work if you just pick an alignment arbitrarily i.e. to suit whatever bitfield you happen to need.
Yes.
The build warnings reported by the test robot are exactly the kind of unintended side effect I was concerned about. It confirms that forcing alignment on a core structure like struct mutex breaks other parts of the kernel that rely on packed structures ;)
Sure, your patch broke the build. So why not write a better patch? You don't need to align the struct, you need to align the lock, like I said already.
Instead, I think I would naturally align the actual locks, that is, arch_spinlock_t and arch_rwlock_t in include/linux/spinlock_types*.h.
That's an interesting point. The blocker tracking mechanism currently operates on higher-level structures like struct mutex. Moving the type encoding down to the lowest-level locks would be a more complex and invasive change, likely beyond the scope of fixing this particular issue.
I don't see why changing kernel struct layouts on m68k is particularly invasive. Perhaps I'm missing something (?)
Looking further ahead, a better long-term solution might be to stop repurposing pointer bits altogether. We could add an explicit blocker_type field to task_struct to be used alongside the blocker field. That would be a much cleaner design. TODO +1 for that idea :)
So, let's drop the patch[1] that enforces alignment and go back to my initial proposal[2], which adjusts the runtime checks to gracefully handle unaligned pointers. That one is self-contained, has minimal impact, and is clearly the safer solution for now.
[1] https://lore.kernel.org/lkml/20250823074048.92498-1-lance.yang@linux.dev [2] https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
I am willing to send a patch if it serves correctness and portability. So you may wish to refrain from crippling your blocker tracking algorithm for now.
On 2025/8/24 12:18, Finn Thain wrote:
On Sun, 24 Aug 2025, Lance Yang wrote:
On 2025/8/24 08:47, Finn Thain wrote:
On Sun, 24 Aug 2025, kernel test robot wrote:
All warnings (new ones prefixed by >>):
In file included from sound/soc/codecs/mt6660.c:15:sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct mt6660_chip' is less than 8 [-Wpacked-not-aligned]
28 | }; | ^sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
25 | struct mutex io_lock; | ^~~~~~~Misalignment warnings like this one won't work if you just pick an alignment arbitrarily i.e. to suit whatever bitfield you happen to need.
Yes.
The build warnings reported by the test robot are exactly the kind of unintended side effect I was concerned about. It confirms that forcing alignment on a core structure like struct mutex breaks other parts of the kernel that rely on packed structures ;)
Sure, your patch broke the build. So why not write a better patch? You don't need to align the struct, you need to align the lock, like I said already.
I think there might be a misunderstanding about the level of abstraction at which the blocker tracking operates.
The blocker tracking mechanism operates on pointers to higher-level locks (like struct mutex), as that is what is stored in the task_struct->blocker field. It does not operate on the lower-level arch_spinlock_t inside it.
While we could track the internal arch_spinlock_t, that would break encapsulation. The hung task detector should remain generic and not depend on lock-specific implementation details ;)
Instead, I think I would naturally align the actual locks, that is, arch_spinlock_t and arch_rwlock_t in include/linux/spinlock_types*.h.
That's an interesting point. The blocker tracking mechanism currently operates on higher-level structures like struct mutex. Moving the type encoding down to the lowest-level locks would be a more complex and invasive change, likely beyond the scope of fixing this particular issue.
I don't see why changing kernel struct layouts on m68k is particularly invasive. Perhaps I'm missing something (?)
Looking further ahead, a better long-term solution might be to stop repurposing pointer bits altogether. We could add an explicit blocker_type field to task_struct to be used alongside the blocker field. That would be a much cleaner design. TODO +1 for that idea :)
So, let's drop the patch[1] that enforces alignment and go back to my initial proposal[2], which adjusts the runtime checks to gracefully handle unaligned pointers. That one is self-contained, has minimal impact, and is clearly the safer solution for now.
[1] https://lore.kernel.org/lkml/20250823074048.92498-1-lance.yang@linux.dev [2] https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
I am willing to send a patch if it serves correctness and portability. So you may wish to refrain from crippling your blocker tracking algorithm for now.
Completely agreed that correctness and portability are the goals.
Please, feel free to send a patch.
On Sun, 24 Aug 2025, Lance Yang wrote:
The blocker tracking mechanism operates on pointers to higher-level locks (like struct mutex), as that is what is stored in the task_struct->blocker field. It does not operate on the lower-level arch_spinlock_t inside it.
Perhaps you are aware that the minimum alignment of the struct is at least the minimum alignment of the first member. I believe that the reason why the lock is always the first member is that misaligned accesses would harm performance.
I really don't know why you want to argue about fixing this.
While we could track the internal arch_spinlock_t, that would break encapsulation.
Would it.
The hung task detector should remain generic and not depend on lock-specific implementation details ;)
OK, like a new class derived from bitfield and pointer? Is that what you mean by "generic" and "encapsulated"?
On 2025/8/24 13:57, Finn Thain wrote:
On Sun, 24 Aug 2025, Lance Yang wrote:
The blocker tracking mechanism operates on pointers to higher-level locks (like struct mutex), as that is what is stored in the task_struct->blocker field. It does not operate on the lower-level arch_spinlock_t inside it.
Perhaps you are aware that the minimum alignment of the struct is at least the minimum alignment of the first member. I believe that the reason why
Yes, that's how it should work in theory.
the lock is always the first member is that misaligned accesses would harm performance.
I really don't know why you want to argue about fixing this.
Okay, arguing further isn't productive. Looking forward to seeing your patch ;)
Thanks, Lance
Hi Lence,
On Sat, 23 Aug 2025 15:40:48 +0800 Lance Yang lance.yang@linux.dev wrote:
From: Lance Yang lance.yang@linux.dev
The blocker tracking mechanism assumes that lock pointers are at least 4-byte aligned to use their lower bits for type encoding.
However, as reported by Geert Uytterhoeven, some architectures like m68k only guarantee 2-byte alignment of 32-bit values. This breaks the assumption and causes two related WARN_ON_ONCE checks to trigger.
To fix this, enforce a minimum of 4-byte alignment on the core lock structures supported by the blocker tracking mechanism. This ensures the algorithm's alignment assumption now holds true on all architectures.
This patch adds __aligned(4) to the definitions of "struct mutex", "struct semaphore", and "struct rw_semaphore", resolving the warnings.
Instead of putting the type flags in the blocker address (pointer), can't we record the type information outside? It is hard to enforce the alignment to the locks, because it is embedded in the data structure. Instead, it is better to record the type as blocker_type in current task_struct.
Thank you,
Thanks to Geert for bisecting!
Reported-by: Geert Uytterhoeven geert@linux-m68k.org Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc... Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker") Cc: stable@vger.kernel.org Signed-off-by: Lance Yang lance.yang@linux.dev
include/linux/mutex_types.h | 2 +- include/linux/rwsem.h | 2 +- include/linux/semaphore.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/mutex_types.h b/include/linux/mutex_types.h index fdf7f515fde8..de798bfbc4c7 100644 --- a/include/linux/mutex_types.h +++ b/include/linux/mutex_types.h @@ -51,7 +51,7 @@ struct mutex { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #else /* !CONFIG_PREEMPT_RT */ /* diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h index f1aaf676a874..f6ecf4a4710d 100644 --- a/include/linux/rwsem.h +++ b/include/linux/rwsem.h @@ -64,7 +64,7 @@ struct rw_semaphore { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #define RWSEM_UNLOCKED_VALUE 0UL #define RWSEM_WRITER_LOCKED (1UL << 0) diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h index 89706157e622..ac9b9c87bfb7 100644 --- a/include/linux/semaphore.h +++ b/include/linux/semaphore.h @@ -20,7 +20,7 @@ struct semaphore { #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER unsigned long last_holder; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER
#define __LAST_HOLDER_SEMAPHORE_INITIALIZER \
2.49.0
On 2025/8/26 13:02, Masami Hiramatsu (Google) wrote:
Hi Lence,
On Sat, 23 Aug 2025 15:40:48 +0800 Lance Yang lance.yang@linux.dev wrote:
From: Lance Yang lance.yang@linux.dev
The blocker tracking mechanism assumes that lock pointers are at least 4-byte aligned to use their lower bits for type encoding.
However, as reported by Geert Uytterhoeven, some architectures like m68k only guarantee 2-byte alignment of 32-bit values. This breaks the assumption and causes two related WARN_ON_ONCE checks to trigger.
To fix this, enforce a minimum of 4-byte alignment on the core lock structures supported by the blocker tracking mechanism. This ensures the algorithm's alignment assumption now holds true on all architectures.
This patch adds __aligned(4) to the definitions of "struct mutex", "struct semaphore", and "struct rw_semaphore", resolving the warnings.
Instead of putting the type flags in the blocker address (pointer), can't we record the type information outside? It is hard to enforce
Yes. Of course. The current pointer-encoding is a tricky trade-off ...
the alignment to the locks, because it is embedded in the data structure. Instead, it is better to record the type as blocker_type in current task_struct.
TODO +1. Separating the type into its own field in task_struct is the right long-term solution ;)
Cheers, Lance
Thank you,
Thanks to Geert for bisecting!
Reported-by: Geert Uytterhoeven geert@linux-m68k.org Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc... Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker") Cc: stable@vger.kernel.org Signed-off-by: Lance Yang lance.yang@linux.dev
include/linux/mutex_types.h | 2 +- include/linux/rwsem.h | 2 +- include/linux/semaphore.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/mutex_types.h b/include/linux/mutex_types.h index fdf7f515fde8..de798bfbc4c7 100644 --- a/include/linux/mutex_types.h +++ b/include/linux/mutex_types.h @@ -51,7 +51,7 @@ struct mutex { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #else /* !CONFIG_PREEMPT_RT */ /* diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h index f1aaf676a874..f6ecf4a4710d 100644 --- a/include/linux/rwsem.h +++ b/include/linux/rwsem.h @@ -64,7 +64,7 @@ struct rw_semaphore { #ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #define RWSEM_UNLOCKED_VALUE 0UL #define RWSEM_WRITER_LOCKED (1UL << 0) diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h index 89706157e622..ac9b9c87bfb7 100644 --- a/include/linux/semaphore.h +++ b/include/linux/semaphore.h @@ -20,7 +20,7 @@ struct semaphore { #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER unsigned long last_holder; #endif -}; +} __aligned(4); /* For hung_task blocker tracking, which encodes type in LSBs */ #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER
#define __LAST_HOLDER_SEMAPHORE_INITIALIZER \
2.49.0
linux-stable-mirror@lists.linaro.org
-
Finn Thain -
kernel test robot -
Lance Yang -
Masami Hiramatsu