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: <1721319512-23585-1-git-send-email-zhang.chuna@h3c.com>
Date: Fri, 19 Jul 2024 00:18:32 +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.

Very sorry to disturb! Just a friendly ping to check in on the status of the 
patch "Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.".  
Please let me know if there is any additional information from my side.

Sincerely look forward to your suggestions and guidance!

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

>>No, the risk here is that when the lock is dropped, other threads will modify the data.  And when this thread (the one running
>>flush_all_zero_pkmaps()) retakes the lock, that data may now be unexpectedly altered.

>map_new_virtual aims to find an usable entry pkmap_count[last_pkmap_nr]. When read and modify the pkmap_count[last_pkmap_nr], the kmap_lock is 
>not dropped. 
>"if (!pkmap_count[last_pkmap_nr])" determine pkmap_count[last_pkmap_nr] is usable or not. If unusable, try agin.

>Furthermore, the value of static variable last_pkmap_nr is stored in a local variable last_pkmap_nr, when kmap_lock is acquired, 
>this is thread-safe.

>In an extreme case, if Thread A and Thread B access the same last_pkmap_nr, Thread A calls function flush_tlb_kernel_range and release the 
>kmap_lock, and Thread B then acquires the kmap_lock and modifies the variable pkmap_count[last_pkmap_nr]. After Thread A completes 
>the execution of function flush_tlb_kernel_range, it will check the variable pkmap_count[last_pkmap_nr]. 
>If pkmap_count[last_pkmap_nr] != 0, Thread A continue to call get_next_pkmap_nr and get next last_pkmap_nr. 

>static inline unsigned long map_new_virtual(struct page *page)
>{
>        unsigned long vaddr;
>        int count;
>        unsigned int last_pkmap_nr; // local variable to store static variable last_pkmap_nr
>        unsigned int color = get_pkmap_color(page);

>start:
>        ...
>                        flush_all_zero_pkmaps();// release kmap_lock, then acquire it
>                        count = get_pkmap_entries_count(color);
>                } 
>                ...
>                if (!pkmap_count[last_pkmap_nr]) // pkmap_count[last_pkmap_nr] is used or not
>                        break;  /* Found a usable entry */
>                if (--count) 
>                        continue;
>
>               ...
>        vaddr = PKMAP_ADDR(last_pkmap_nr);
>        set_pte_at(&init_mm, vaddr,
>                   &(pkmap_page_table[last_pkmap_nr]), mk_pte(page, kmap_prot));
>
>        pkmap_count[last_pkmap_nr] = 1;
>        ...
>        return vaddr;
>}
   
-- 
1.8.3.1