Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/3/26 16:19, Paolo Bonzini wrote:
> On 8/3/26 13:54, David Hildenbrand (Arm) wrote:
>> On 7/31/26 18:43, Paolo Bonzini wrote:
>>> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
>>> the reporter to help with testing these patches.
>>>
>>> Right now, users of .pfn_mkwrite() have no way to create a PTE
>>> that has gone through maybe_mkwrite().  Because vma_set_page_prot()
>>> will have cleared the writable PTE bit, users of fixup_user_fault()
>>> will see a read-only PTE and have no clue that the page needs
>>> a *second* fault to reach its final status.
>>>
>>> Handling this in fixup_user_fault() is problematic: the information
>>> about the presence of *_mkwrite is only recorded in vma->vm_page_prot,
>>> which is an opaque pgprot_t, therefore only follow_pfnmap_start()
>>> knows how to retrieve it.
>>
>> How is mprotect() supposed to work in that case?
> 
> Hi David,

Hi!

> 
> not sure what you are worried about specifically, but fixup_user_fault() catches !VM_WRITE VMAs and returns early (see vma_permits_fault()).

That part is clear, I was wondering about the following:

mprotect(PROT_READ)

followed by

mprotect(PROT_READ | PROT_WRITE)

You'd similarly end up without the writable bit in the PTE, and apparently there is not really a way
to recover from this.

Maybe that's just ok (just sounded odd :) ).

> 
> Also, do_wp_page() has the comment:
> 
>         /*
>          * Shared mapping: we are guaranteed to have VM_WRITE and
>          * FAULT_FLAG_WRITE set at this point.
>          */
> 
> before the call to wp_pfn_shared() which is where .pfn_mkwrite() is called.
> 
> Let me know if this was not what you were asking.
> 
> Thanks for the review of patch 1---I mentioned here in the cover letter that the name was temporary and I'll take your suggestion.  I can either use EXPORT_SYMBOL_GPL or switch to inlines, but not
> both because the existing functions like vmf_insert_pfn_prot() need to stay non-GPL-only.
> 
> Anyhow, now that the series has a Tested-by I'll clean up everything, and repost later this week.

Thanks!

-- 
Cheers,

David