linux-kernel - Re: [PATCH v2] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240710103611.809895ff809df9ed411bfaa8@linux-foundation.org>
Date: Wed, 10 Jul 2024 10:36:11 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: zhangchun <zhang.chuna@....com>
Cc: <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
 <jiaoxupo@....com>, <zhang.zhengming@....com>, <zhang.zhansheng@....com>,
 <shaohaojize@....com>
Subject: Re: [PATCH v2] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

On Wed, 10 Jul 2024 20:20:28 +0800 zhangchun <zhang.chuna@....com> wrote:

> Use kmap_high and kmap_XXX or kumap_xxx among differt cores at the same
> time may cause deadlock. The issue is like this：

What is kmap_XXX?

>  CPU 0:                                                 CPU 1:
>  kmap_high(){                                           kmap_xxx() {
>                ...                                        irq_disable();
>         spin_lock(&kmap_lock)
>                ...
>         map_new_virtual                                     ...
>            flush_all_zero_pkmaps
>               flush_tlb_kernel_range         /* CPU0 holds the kmap_lock */
>                       smp_call_function_many         spin_lock(&kmap_lock)
>                       ...                                   ....
>         spin_unlock(&kmap_lock)
>                ...
> 
> CPU 0 holds the kmap_lock, waiting for CPU 1 respond to IPI. But CPU 1
> has disabled irqs, waiting for kmap_lock, cannot answer the IPI. Fix
> this by releasing  kmap_lock before call flush_tlb_kernel_range,
> avoid kmap_lock deadlock.
> 
> Fixes: 3297e760776a ("highmem: atomic highmem kmap page pinning")

Wow, that's 15 years old.  Has the deadlock been observed?

> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -220,8 +220,11 @@ static void flush_all_zero_pkmaps(void)
>  		set_page_address(page, NULL);
>  		need_flush = 1;
>  	}
> -	if (need_flush)
> +	if (need_flush) {
> +		unlock_kmap();
>  		flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP));
> +		lock_kmap();
> +	}
>  }

Why is dropping the lock like this safe?  What data is it protecting
and why is it OK to leave that data unprotected here?