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

[PATCH] 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 take mtrr_mutex just to copy the boot-time
entries and switch cache_map over. On allocation failure cache_map is
now set to NULL explicitly rather than by the failed assignment, so
the behaviour is unchanged and cache_map still never points at the
__initdata array after init.

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>
---
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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 67cf69f24b00..430bc9dd6f20 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -402,20 +402,25 @@ 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);
-	if (cache_map) {
-		memmove(cache_map, init_cache_map,
-			cache_map_n * sizeof(*cache_map));
+	if (new_map) {
+		memmove(new_map, init_cache_map,
+			cache_map_n * sizeof(*new_map));
+		cache_map = new_map;
 		cache_map_size = new_size;
 	} else {
+		cache_map = NULL;
 		mtrr_state.enabled = 0;
 		pr_err("MTRRs disabled due to allocation failure for lookup map.\n");
 	}
-- 
2.55.0.windows.5

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

[Jürgen Groß]

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

On 05.10.26 12:21, 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 take mtrr_mutex just to copy the boot-time
> entries and switch cache_map over. On allocation failure cache_map is
> now set to NULL explicitly rather than by the failed assignment, so
> the behaviour is unchanged and cache_map still never points at the
> __initdata array after init.
> 
> 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>
> ---
> 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 | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 67cf69f24b00..430bc9dd6f20 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -402,20 +402,25 @@ 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);

Having here:

+	cache_map = new_map;

would avoid all the code churn below.

> -	if (cache_map) {
> -		memmove(cache_map, init_cache_map,
> -			cache_map_n * sizeof(*cache_map));
> +	if (new_map) {
> +		memmove(new_map, init_cache_map,
> +			cache_map_n * sizeof(*new_map));
> +		cache_map = new_map;
>   		cache_map_size = new_size;
>   	} else {
> +		cache_map = NULL;
>   		mtrr_state.enabled = 0;
>   		pr_err("MTRRs disabled due to allocation failure for lookup map.\n");
>   	}


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 --]

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

[Yogesh Gaur]

On Mon, Oct 5, 2026 at 5:55 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 05.10.26 12:21, 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 take mtrr_mutex just to copy the boot-time
> > entries and switch cache_map over. On allocation failure cache_map is
> > now set to NULL explicitly rather than by the failed assignment, so
> > the behaviour is unchanged and cache_map still never points at the
> > __initdata array after init.
> >
> > 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>
> > ---
> > 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 | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index 67cf69f24b00..430bc9dd6f20 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -402,20 +402,25 @@ 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);
>
> Having here:
>
> +       cache_map = new_map;
>
> would avoid all the code churn below.
>
Thanks for review. Would remove in v2.

Regards
Yogesh

> > -     if (cache_map) {
> > -             memmove(cache_map, init_cache_map,
> > -                     cache_map_n * sizeof(*cache_map));
> > +     if (new_map) {
> > +             memmove(new_map, init_cache_map,
> > +                     cache_map_n * sizeof(*new_map));
> > +             cache_map = new_map;
> >               cache_map_size = new_size;
> >       } else {
> > +             cache_map = NULL;
> >               mtrr_state.enabled = 0;
> >               pr_err("MTRRs disabled due to allocation failure for lookup map.\n");
> >       }
>
>
> Juergen