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.

There are actually some preexisting functions that suggest how
this is supposed to be handled, namely vmf_insert_page_mkwrite() and
vmf_insert_pfn_pmd().  So, this series adjusts mm/memory.c to export
two new functions vmf_insert_pfn_mkwrite() and __vmf_insert_pfn_prot(),
and then teaches drm's two users of .pfn_mkwrite() to call them.  Let
me know if I should use another name like vmf_insert_pfn_prot_mkwrite(),
instead of the "__"-prefixed one.

The drm_gem_shmem_helper case was reported as a KVM regression, while
the vmwgfx one was found by inspection of .pfn_mkwrite() implementors.

Thanks,

Paolo

Paolo Bonzini (3):
  mm: export variants of vmf_insert_pfn* for use with pfn_mkwrite()
  drm/shmem_helper: use vmf_insert_pfn_mkwrite()
  drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is
    in use

 drivers/gpu/drm/drm_gem_shmem_helper.c     | 38 +++++------
 drivers/gpu/drm/ttm/ttm_bo_vm.c            |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 44 +++++++------
 include/linux/mm.h                         |  4 ++
 mm/huge_memory.c                           |  2 +-
 mm/memory.c                                | 75 ++++++++++++++++------
 6 files changed, 107 insertions(+), 59 deletions(-)

-- 
2.55.0

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?

-- 
Cheers,

David

On 8/3/26 10:55, David Hildenbrand (Arm) wrote:
> On 7/31/26 18:05, Paolo Bonzini wrote:
>> Reported-by: Sergio Lopez <slp@redhat.com>
> 
> Reported-by: without Fixes: is odd.

Fixes: 6da8e9634bb7 ("mm: new follow_pfnmap API") would also be odd :) 
but I can certainly add it.
>> +	 * @write_fault: if true, fail with -EFAULT unless the mapping is
> 
> Just wondering whether EPERM would be better.

It would be EACCES if anything, not EPERM; but almost all callers 
already pass EFAULT to userspace, and write() to a PROT_READ area 
returns EFAULT, so I don't think EACCES is the right choice.

>> +	 * writable
>>  	 */
>>  	struct vm_area_struct *vma;
>>  	unsigned long address;
>> +	bool write_fault;
> 
> "write_fault" is a rather odd name for this, given that this function will not
> trigger a write fault.
> 
> You want something that matches FOLL_WRITE.
> 
> "write_access" / "check_writable" maybe?
There are no for_write, write_access or check_write in mm/,  but there 
are a handful of each of these

	int write = (gup_flags & FOLL_WRITE);
	bool write = vmf->flags & FAULT_FLAG_WRITE;

so I'll go for just "write".

Thanks,

Paolo

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,

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

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.

Paolo

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

On Mon, Aug 3, 2026 at 5:18 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
> >>> 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?
> >
> 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.

Why not? mprotect_fixup() calls vma_set_page_prot(), the PTE as you
say lacks the writable bit (unless pte_dirty(pte)), and then the next
fault calls .pfn_mkwrite().

Paolo

On 8/4/26 09:50, Paolo Bonzini wrote:
> On Mon, Aug 3, 2026 at 5:18 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>>>
>> 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.
> 
> Why not? mprotect_fixup() calls vma_set_page_prot(), the PTE as you
> say lacks the writable bit (unless pte_dirty(pte)), and then the next
> fault calls .pfn_mkwrite().

Ah, if this works, great. I guess I was confused about your explanation about
fixup_user_fault().

So this really only about avoiding the second fault, makes sense thanks!

-- 
Cheers,

David

Paolo Bonzini <pbonzini@redhat.com> writes:

> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
> the reporter to help with testing these patches.

Tested alongside with the mm fix [1], and it fixes the virtio-gpu
mapping issue [2] for both the absent PTE case and the pre-faulted
read-only PTE case.

Tested-by: Sergio Lopez <slp@redhat.com>

Thanks, Paolo.

[1] https://lore.kernel.org/kvm/CABgObfbkqYNsPQnKxK1_4adXF_tSdtScPU5-Xrg-sXyeWfMVfQ@mail.gmail.com/T/#t
[2] https://lore.kernel.org/kvm/299bddc0-fafc-48b3-a9c6-ec171136a46a@redhat.com/T/#t