On 05.08.22 20:12, Peter Xu wrote:
> On Fri, Aug 05, 2022 at 01:03:29PM +0200, David Hildenbrand wrote:
>> Let's add a safety net if we ever get (again) a write-fault on a R/O-mapped
>> page in a shared mapping, in which case we simply have to map the
>> page writable.
>>
>> VM_MAYSHARE handling in hugetlb_fault() for FAULT_FLAG_WRITE
>> indicates that this was at least envisioned, but could never have worked
>> as expected. This theoretically paves the way for softdirty tracking
>> support in hugetlb.
>>
>> Tested without the fix for softdirty tracking.
>>
>> Note that there is no need to do any kind of reservation in hugetlb_fault()
>> in this case ... because we already have a hugetlb page mapped R/O
>> that we will simply map writable and we are not dealing with COW/unsharing.
>>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>> mm/hugetlb.c | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index a18c071c294e..bbab7aa9d8f8 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -5233,6 +5233,16 @@ 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));
>>
>> + /* Let's take out shared mappings first, this should be a rare event. */
>> + if (unlikely(vma->vm_flags & VM_MAYSHARE)) {
>
> Should we check VM_SHARED instead?
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).
>
>> + if (unshare)
>> + return 0;
>
> Curious when will this happen especially if we switch to VM_SHARED above.
> Shouldn't "unshare" not happen at all on a shared region?
FAULT_FLAG_UNSHARE is documented to behave like:
"FAULT_FLAG_UNSHARE is ignored and treated like an ordinary read fault
when no existing R/O-mapped anonymous page is encountered."
It should currently not happen. Focus on should ;)
>
>> + if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE)))
>> + return VM_FAULT_SIGSEGV;
>
> I had a feeling that you just want to double check we have write
> permission, but IIUC this should be checked far earlier or we'll have
> problem. No strong opinion if so, but I'd suggest dropping this one,
> otherwise we could add tons of WARN_ON_ONCE() in anywhere in the page fault
> stack and they mostly won't trigger at all.
Not quite. We usually (!hugetlb) have maybe_mkwrite() all over the
place. This is just an indication that we don't have maybe semantics
here. But as we also don't have it for hugetlb anon code below, maybe I
can just drop it. (or check it for both call paths)
>
>> + set_huge_ptep_writable(vma, haddr, ptep);
>
> Do we wanna set dirty bits too?
set_huge_ptep_writable() handles that.
Thanks!
--
Thanks,
David / dhildenb
