Re: [PATCH v2 2/6] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

On Tue, 4 Aug 2026 16:15:49 +0200
Boris Brezillon <[email protected]> wrote:

> On Tue,  4 Aug 2026 14:05:24 +0200
> Paolo Bonzini <[email protected]> wrote:
> 
> > This ensures that KVM or VFIO correctly see a writable PTE when
> > they request one.  Otherwise, a guest write to an unpopulated
> > PTE from a mapping backed by a DRM GEM BO triggers a VM exit
> > with EFAULT.
> > 
> > The code actually is simpler, because the same logic already
> > applied to the hugepage mapping case using vmf_insert_pfn_pmd().
> > 
> > Reported-by: Sergio Lopez <[email protected]>
> > Link: https://lore.kernel.org/kvm/[email protected]/
> > Tested-by: Sergio Lopez <[email protected]>
> > Reviewed-by: Boris Brezillon <[email protected]>
> > Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
> > Cc: [email protected]
> > Signed-off-by: Paolo Bonzini <[email protected]>
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++++++------------
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index c989459eb215..c81be3e97317 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -589,11 +589,25 @@ static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
> >  	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
> >  }
> >  
> > +/*
> > + * Because the vm_ops have a .pfn_mkwrite() callback, vma_set_page_prot()
> > + * has cleared the write bit from vma->vm_page_prot.  vmf_insert_pfn()
> > + * would install a read-only entry even for a write fault, relying on a
> > + * second fault to reach .pfn_mkwrite() and upgrade it, but that second
> > + * fault never happens for fixup_user_fault() callers that directly
> > + * walk the page tables with follow_pfnmap_start().  To ensure that
> > + * they don't see the read-only entry, pass FAULT_FLAG_WRITE info down
> > + * to install a writable entry right away.  Because .pfn_mkwrite() is
> > + * not invoked, record the write afterwards.
> > + */
> >  static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
> >  				 unsigned long pfn)
> >  {
> > +	bool write = vmf->flags & FAULT_FLAG_WRITE;
> > +	vm_fault_t ret = VM_FAULT_FALLBACK;
> > +
> >  	if (!order) {
> > -		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> > +		ret = vmf_insert_pfn_mkwrite(vmf->vma, vmf->address, pfn, write);
> >  #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
> >  	} else if (order == PMD_ORDER) {
> >  		unsigned long paddr = pfn << PAGE_SHIFT;
> > @@ -601,27 +615,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
> >  
> >  		if (aligned &&
> >  		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
> > -			vm_fault_t ret;
> > -
> >  			pfn &= PMD_MASK >> PAGE_SHIFT;
> > -
> > -			/* Unlike PTEs which are automatically upgraded to
> > -			 * writeable entries, the PMD upgrades go through
> > -			 * .huge_fault(). Make sure we pass the "write" info
> > -			 * along in that case.
> > -			 * This also means we have to record the write fault
> > -			 * here, instead of in .pfn_mkwrite().
> > -			 */
> > -			ret = vmf_insert_pfn_pmd(vmf, pfn,
> > -						 vmf->flags & FAULT_FLAG_WRITE);
> > -			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> > -				drm_gem_shmem_record_mkwrite(vmf);
> > -
> > -			return ret;
> > +			ret = vmf_insert_pfn_pmd(vmf, pfn, write);
> >  		}
> >  #endif
> >  	}
> > -	return VM_FAULT_FALLBACK;
> > +
> > +	if (ret == VM_FAULT_NOPAGE && write)
> > +		drm_gem_shmem_record_mkwrite(vmf);  
> 
> Actually, if we're making the drm_gem_shmem_record_mkwrite() call
> unconditional (for PTE and PMD updates) in that path, can't we drop the
> drm_gem_shmem_pfn_mkwrite() call living in drm_gem_shmem_pfn_mkwrite()?
> 
> Also, I'm not even sure we can end up with write=true for PTE updates,
> because our pfn_mkwrite implementation returns zero, not VM_FAULT_ERROR
> or VM_FAULT_NOPAGE. This means the default RO -> RW PTE upgrade
> implemented in finish_mkwrite_fault() [1] will take place. If we really
> want out try_insert_pfn() to be called for those RO -> RW updgrades, we
> need to call try_insert_pfn() from drm_gem_shmem_pfn_mkwrite().

Nevermind, it's all explained in the comment you've added. Sorry for
the noise. I keep wondering if we shouldn't call try_insert_pfn() from
pfn_mkwrite() though, like is done in other places.