[PATCH v2] x86/mtrr: allocate the cache map before taking mtrr_mutex

[PATCH v2] x86/mtrr: allocate the cache map before taking mtrr_mutex

[Yogesh Gaur]

mtrr_copy_map() does a GFP_KERNEL allocation with mtrr_mutex held.
It runs once at boot from mtrr_init_finalize(), but lockdep keeps
the mtrr_mutex -> fs_reclaim dependency it records there for the
life of the system. Once another path gives lockdep the rest of a
cycle back to a lock held around mtrr_mutex, the next MTRR ioctl
reports a circular dependency. syzbot found this through nbd, which
takes cpu_hotplug_lock (via sk_set_memalloc() -> static_key_slow_inc())
under its tx_lock, while mtrr_del_page() takes mtrr_mutex under
cpu_hotplug_lock:

  WARNING: possible circular locking dependency detected
  syz.9.5848/21744 is trying to acquire lock:
   (mtrr_mutex), at: mtrr_del_page arch/x86/kernel/cpu/mtrr/mtrr.c:408
  but task is already holding lock:
   (cpu_hotplug_lock), at: mtrr_del_page arch/x86/kernel/cpu/mtrr/mtrr.c:407
  -> #1 (fs_reclaim):
         fs_reclaim_acquire
         might_alloc
         slab_pre_alloc_hook
         __kmalloc_noprof
         mtrr_copy_map arch/x86/kernel/cpu/mtrr/generic.c:413
         mtrr_init_finalize arch/x86/kernel/cpu/mtrr/mtrr.c:618
  Chain exists of:
    mtrr_mutex --> &nsock->tx_lock --> cpu_hotplug_lock

The allocation does not need the mutex; only publishing the new map
does. Allocate first and assign cache_map once mtrr_mutex is held.
The rest of the function is unchanged.

Fixes: 061b984aab58 ("x86/mtrr: Construct a memory map with cache modes")
Reported-by: syzbot+342762971f666337474e@syzkaller.appspotmail.com
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
v2: Incorporated review comments, assign cache_map = new_map after mutex_lock()
v1: https://lore.kernel.org/all/20261005102152.368-1-yogeshgaur.83@gmail.com/

Built with W=1 only. syzbot has no reproducer for this report, so the
fix has not been runtime-tested.

 arch/x86/kernel/cpu/mtrr/generic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 67cf69f24b00..92c3fcb7fb6d 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -402,15 +402,19 @@ void __init mtrr_build_map(void)
 void __init mtrr_copy_map(void)
 {
 	unsigned int new_size = get_cache_map_size();
+	struct cache_map *new_map;
 
 	if (!mtrr_state.enabled || !new_size) {
 		cache_map = NULL;
 		return;
 	}
 
+	/* Allocate before taking mtrr_mutex, the allocation may reclaim. */
+	new_map = kzalloc_objs(*new_map, new_size);
+
 	mutex_lock(&mtrr_mutex);
 
-	cache_map = kzalloc_objs(*cache_map, new_size);
+	cache_map = new_map;
 	if (cache_map) {
 		memmove(cache_map, init_cache_map,
 			cache_map_n * sizeof(*cache_map));
-- 
2.55.0.windows.5

Re: [PATCH v2] x86/mtrr: allocate the cache map before taking mtrr_mutex

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1766 bytes --]

On 05.10.26 15:02, Yogesh Gaur wrote:
> mtrr_copy_map() does a GFP_KERNEL allocation with mtrr_mutex held.
> It runs once at boot from mtrr_init_finalize(), but lockdep keeps
> the mtrr_mutex -> fs_reclaim dependency it records there for the
> life of the system. Once another path gives lockdep the rest of a
> cycle back to a lock held around mtrr_mutex, the next MTRR ioctl
> reports a circular dependency. syzbot found this through nbd, which
> takes cpu_hotplug_lock (via sk_set_memalloc() -> static_key_slow_inc())
> under its tx_lock, while mtrr_del_page() takes mtrr_mutex under
> cpu_hotplug_lock:
> 
>    WARNING: possible circular locking dependency detected
>    syz.9.5848/21744 is trying to acquire lock:
>     (mtrr_mutex), at: mtrr_del_page arch/x86/kernel/cpu/mtrr/mtrr.c:408
>    but task is already holding lock:
>     (cpu_hotplug_lock), at: mtrr_del_page arch/x86/kernel/cpu/mtrr/mtrr.c:407
>    -> #1 (fs_reclaim):
>           fs_reclaim_acquire
>           might_alloc
>           slab_pre_alloc_hook
>           __kmalloc_noprof
>           mtrr_copy_map arch/x86/kernel/cpu/mtrr/generic.c:413
>           mtrr_init_finalize arch/x86/kernel/cpu/mtrr/mtrr.c:618
>    Chain exists of:
>      mtrr_mutex --> &nsock->tx_lock --> cpu_hotplug_lock
> 
> The allocation does not need the mutex; only publishing the new map
> does. Allocate first and assign cache_map once mtrr_mutex is held.
> The rest of the function is unchanged.
> 
> Fixes: 061b984aab58 ("x86/mtrr: Construct a memory map with cache modes")
> Reported-by: syzbot+342762971f666337474e@syzkaller.appspotmail.com
> Assisted-by: LLM
> Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]