linux-kernel - Re: [PATCH 2/3] userfaultfd: convert __mcopy_atomic

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] [day] [month] [year] [list]

Date:   Tue, 14 Mar 2023 19:47:20 +0800
From:   "zhangpeng (AS)" <zhangpeng362@...wei.com>
To:     Matthew Wilcox <willy@...radead.org>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <akpm@...ux-foundation.org>, <mike.kravetz@...cle.com>,
        <vishal.moola@...il.com>, <sidhartha.kumar@...cle.com>,
        <wangkefeng.wang@...wei.com>, <sunnanyong@...wei.com>
Subject: Re: [PATCH 2/3] userfaultfd: convert __mcopy_atomic_hugetlb() to use
 a folio

On 2023/3/14 16:31, Matthew Wilcox wrote:

> On Tue, Mar 14, 2023 at 03:37:33AM +0000, Peng Zhang wrote:
>> +++ b/include/linux/mm.h
>> @@ -3546,9 +3546,8 @@ extern void copy_user_huge_page(struct page *dst, struct page *src,
>>   				unsigned long addr_hint,
>>   				struct vm_area_struct *vma,
>>   				unsigned int pages_per_huge_page);
>> -extern long copy_huge_page_from_user(struct page *dst_page,
>> +extern long copy_large_folio_from_user(struct folio *dst_folio,
> You can drop the 'extern'.

Got it.

>> +++ b/mm/memory.c
>> @@ -5769,26 +5769,28 @@ void copy_user_huge_page(struct page *dst, struct page *src,
>>   	process_huge_page(addr_hint, pages_per_huge_page, copy_subpage, &arg);
>>   }
>>   
>> -long copy_huge_page_from_user(struct page *dst_page,
>> +long copy_large_folio_from_user(struct folio *dst_folio,
>>   				const void __user *usr_src,
>> -				unsigned int pages_per_huge_page,
>>   				bool allow_pagefault)
>>   {
>>   	void *page_kaddr;
>>   	unsigned long i, rc = 0;
>> -	unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
>> +	unsigned int nr_pages = folio_nr_pages(dst_folio);
>> +	unsigned long ret_val = nr_pages * PAGE_SIZE;
>>   	struct page *subpage;
>> +	struct folio *inner_folio;
> What is an 'inner folio'?
>
>> -	for (i = 0; i < pages_per_huge_page; i++) {
>> -		subpage = nth_page(dst_page, i);
>> +	for (i = 0; i < nr_pages; i++) {
>> +		subpage = folio_page(dst_folio, i);
>> +		inner_folio = page_folio(subpage);
>>   		if (allow_pagefault)
>> -			page_kaddr = kmap(subpage);
>> +			page_kaddr = kmap_local_folio(inner_folio, 0);
> This doesn't do what you think it does.  Did you test this?
>
>>   		else
>>   			page_kaddr = kmap_atomic(subpage);
> Pretty sure all this should be converted to kmap_local and the atomic
> bits should go away.
>
>>   		rc = copy_from_user(page_kaddr,
>>   				usr_src + i * PAGE_SIZE, PAGE_SIZE);
>>   		if (allow_pagefault)
>> -			kunmap(subpage);
>> +			kunmap_local(page_kaddr);
>>   		else
>>   			kunmap_atomic(page_kaddr);
>>   
>> @@ -5796,7 +5798,7 @@ long copy_huge_page_from_user(struct page *dst_page,
>>   		if (rc)
>>   			break;
>>   
>> -		flush_dcache_page(subpage);
>> +		flush_dcache_folio(inner_folio);
> The flush should probably be pulled outside the loop.
>
>> +			err = copy_large_folio_from_user(folio,
>> +						(const void __user *) src_addr, true);
> I wonder if this shouldn't be 'copy_folio_from_user()'.  after all,
> it'll work for any size folio, right?

Thanks for your review.

I'll rename copy_large_folio_from_user() to copy_folio_from_user().
I'll delete the inner_folio. kmap() and kmap_atomic() will be
converted to the following code.

	page_kaddr = kmap_local_page(subpage);
	if (!allow_pagefault)
		pagefault_disable();
	rc = copy_from_user(page_kaddr,
			usr_src + i * PAGE_SIZE, PAGE_SIZE);
	if (!allow_pagefault)
		pagefault_enable();
	kunmap_local(page_kaddr);

flush_dcache_folio() will be placed outside the loop.

I'll fix all this in a v2 of this patch series.


Thanks,
Peng.