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

["Jürgen Groß" <jgross@suse.com>]

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