Re: [PATCH v1 2/2] mm/hugetlb: support write-faults in shared mappings

[David Hildenbrand]

On 10.08.22 11:37, David Hildenbrand wrote:
> On 09.08.22 00:08, Peter Xu wrote:
>> On Mon, Aug 08, 2022 at 04:21:39PM -0400, Peter Xu wrote:
>>> On Mon, Aug 08, 2022 at 06:25:21PM +0200, David Hildenbrand wrote:
>>>>>> Relying on VM_SHARED to detect MAP_PRIVATE vs. MAP_SHARED is
>>>>>> unfortunately wrong.
>>>>>>
>>>>>> If you're curious, take a look at f83a275dbc5c ("mm: account for
>>>>>> MAP_SHARED mappings using VM_MAYSHARE and not VM_SHARED in hugetlbfs")
>>>>>> and mmap() code.
>>>>>>
>>>>>> Long story short: if the file is read-only, we only have VM_MAYSHARE but
>>>>>> not VM_SHARED (and consequently also not VM_MAYWRITE).
>>>>>
>>>>> To ask in another way: if file is RO but mapped RW (mmap() will have
>>>>> VM_SHARED cleared but VM_MAYSHARE set), then if we check VM_MAYSHARE here
>>>>> won't we grant write bit errornously while we shouldn't? As the user
>>>>> doesn't really have write permission to the file.
>>>>
>>>> Thus the VM_WRITE check. :)
>>>>
>>>> I wonder if we should just do it cleanly and introduce the maybe_mkwrite
>>>> semantics here as well. Then there is no need for additional VM_WRITE
>>>> checks and hugetlb will work just like !hugetlb.
>>>
>>> Hmm yeah I think the VM_MAYSHARE check is correct, since we'll need to fail
>>> the cases where MAYSHARE && !SHARE - we used to silently let it pass.
>>
>> Sorry I think this is a wrong statement I made..  IIUC we'll fail correctly
>> with/without the patch on any write to hugetlb RO regions.
>>
>> Then I just don't see a difference on checking VM_SHARED or VM_MAYSHARE
>> here, it's just that VM_MAYSHARE check should work too like VM_SHARED so I
>> don't see a problem.
>>
>> It also means I can't think of any valid case of having VM_WRITE when
>> reaching here, then the WARN_ON_ONCE() is okay but maybe also redundant.
>> Using maybe_mkwrite() seems misleading to me if FOLL_FORCE not ready for
>> hugetlbfs after all.
>>
> 
> The main reason we'd have it would be to scream out lout and fail
> gracefully if someone would -- for example -- use it for something like
> FOLL_FORCE. I mean triggering a write fault without VM_WRITE on !hugetlb
> works, so it would be easy to assume that it similarly simply works for
> hugetlb as well. And the code most probably wouldn't even blow up
> immediately :)
> 

I propose the following change to hugetlb_wp():

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a18c071c294e..b92d30d3b33b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5233,6 +5233,21 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
        VM_BUG_ON(unshare && (flags & FOLL_WRITE));
        VM_BUG_ON(!unshare && !(flags & FOLL_WRITE));
 
+       /*
+        * hugetlb does not support FOLL_FORCE-style write faults that keep the
+        * PTE mapped R/O such as maybe_mkwrite() would do.
+        */
+       if (WARN_ON_ONCE(!unshare && !(vma->vm_flags & VM_WRITE)))
+               return VM_FAULT_SIGSEGV;
+
+       /* Let's take out shared mappings first, this should be a rare event. */
+       if (unlikely(vma->vm_flags & VM_MAYSHARE)) {
+               if (unlikely(unshare))
+                       return 0;
+               set_huge_ptep_writable(vma, haddr, ptep);
+               return 0;
+       }
+



-- 
Thanks,

David / dhildenb