linux-kernel - Re: [PATCH 06/14] mm: handle_pte_fault() use pte_offset_map_maywrite

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: <4b867535-8481-4fa1-bed1-ad25a76682f0@redhat.com>
Date: Wed, 21 Aug 2024 11:41:57 +0200
From: David Hildenbrand <david@...hat.com>
To: Qi Zheng <zhengqi.arch@...edance.com>,
 LEROY Christophe <christophe.leroy2@...soprasteria.com>
Cc: "hughd@...gle.com" <hughd@...gle.com>,
 "willy@...radead.org" <willy@...radead.org>,
 "muchun.song@...ux.dev" <muchun.song@...ux.dev>,
 "vbabka@...nel.org" <vbabka@...nel.org>,
 "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
 "rppt@...nel.org" <rppt@...nel.org>,
 "vishal.moola@...il.com" <vishal.moola@...il.com>,
 "peterx@...hat.com" <peterx@...hat.com>,
 "ryan.roberts@....com" <ryan.roberts@....com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mm@...ck.org" <linux-mm@...ck.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH 06/14] mm: handle_pte_fault() use
 pte_offset_map_maywrite_nolock()

On 21.08.24 11:24, Qi Zheng wrote:
> 
> 
> On 2024/8/21 17:17, LEROY Christophe wrote:
>>
>>
>> Le 21/08/2024 à 10:18, Qi Zheng a écrit :
>>> In handle_pte_fault(), we may modify the vmf->pte after acquiring the
>>> vmf->ptl, so convert it to using pte_offset_map_maywrite_nolock(). But
>>> since we already do the pte_same() check, so there is no need to get
>>> pmdval to do pmd_same() check, just pass NULL to pmdvalp parameter.
>>>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@...edance.com>
>>> ---
>>>     mm/memory.c | 9 +++++++--
>>>     1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 93c0c25433d02..d3378e98faf13 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -5504,9 +5504,14 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
>>>     		 * pmd by anon khugepaged, since that takes mmap_lock in write
>>>     		 * mode; but shmem or file collapse to THP could still morph
>>>     		 * it into a huge pmd: just retry later if so.
>>> +		 *
>>> +		 * Use the maywrite version to indicate that vmf->pte will be
>>> +		 * modified, but since we will use pte_same() to detect the
>>> +		 * change of the pte entry, there is no need to get pmdval.
>>>     		 */
>>> -		vmf->pte = pte_offset_map_nolock(vmf->vma->vm_mm, vmf->pmd,
>>> -						 vmf->address, &vmf->ptl);
>>> +		vmf->pte = pte_offset_map_maywrite_nolock(vmf->vma->vm_mm,
>>> +							  vmf->pmd, vmf->address,
>>> +							  NULL, &vmf->ptl);

I think we discussed that passing NULL should be forbidden for that 
function.

>>
>> This might be the demonstration that the function name is becoming too long.
>>
>> Can you find shorter names ?
> 
> Maybe use abbreviations?
> 
> pte_offset_map_ro_nolock()
> pte_offset_map_rw_nolock()

At least the "ro" is better, but "rw" does not express the "maywrite" -- 
because without taking the lock we are not allowed to write. But maybe 
"rw" is good enough for that if we document it properly.

And you can use up to 100 characters, if it helps readability.

-- 
Cheers,

David / dhildenb