[PATCH] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

[PATCH] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

[zhangchun]

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

 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")
Signed-off-by: zhangchun <zhang.chuna@h3c.com>
Co-developed-by: zhangzhansheng <zhang.zhansheng@h3c.com>
Signed-off-by: zhangzhansheng <zhang.zhansheng@h3c.com>
Reviewed-by: zhangzhengming <zhang.zhengming@h3c.com>
---
 mm/highmem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/highmem.c b/mm/highmem.c
index bd48ba4..841b370 100644
--- 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) {
+		spin_unlock(&kmap_lock);
 		flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP));
+		spin_lock(&kmap_lock);
+	}
 }
 
 void __kmap_flush_unused(void)
-- 
1.8.3.1

Re: [PATCH] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

[Matthew Wilcox]

On Tue, Jul 09, 2024 at 05:17:38PM +0800, zhangchun wrote:
> +++ 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) {
> +		spin_unlock(&kmap_lock);

should this be a raw spin_unlock(), or should it be unlock_kmap()?
ie when ARCH_NEEDS_KMAP_HIGH_GET is set, do we also need to re-enable
interrupts here?

>  		flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP));
> +		spin_lock(&kmap_lock);
> +	}
>  }
>  
>  void __kmap_flush_unused(void)
> -- 
> 1.8.3.1
> 
> 

[PATCH] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock V2.

[zhangchun]

>> +++ 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) {
>> +		spin_unlock(&kmap_lock);

>should this be a raw spin_unlock(), or should it be unlock_kmap()?
>ie when ARCH_NEEDS_KMAP_HIGH_GET is set, do we also need to re-enable interrupts here?

Thanks! Using lock_map/unlock_kmap is better.
Patch V2 will be sent.
 
1.8.3.1