linux-kernel - [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: <1720681676-53147-1-git-send-email-zhang.chuna@h3c.com>
Date: Thu, 11 Jul 2024 15:07:56 +0800
From: zhangchun <zhang.chuna@....com>
To: <akpm@...ux-foundation.org>
CC: <jiaoxupo@....com>, <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
        <shaohaojize@....com>, <zhang.chuna@....com>,
        <zhang.zhansheng@....com>, <zhang.zhengming@....com>
Subject: [PATCH v2] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

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

kmap/kunmap.

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

Yeah, the device crashed due to this reason. 
 
>> --- 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?

kmap_lock is used to protect pkmap_count, pkmap_page_table and last_pkmap_nr(static variable). 
When call flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP)), flush_tlb_kernel_range
will neither modify nor read these variables. Leave that data unprotected here is safe.
-- 
1.8.3.1