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@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Signed-off-by: Yogesh Gaur <yogeshgaur.83@xxxxxxxxx>
> > ---
> > 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