Re: [PATCH] drm/gem-shmem: Install writable PTEs for write faults

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect page offset calculation for split VMAs in drm_gem_shmem fault handlers.
--

--- Patch [1]: [PATCH] drm/gem-shmem: Install writable PTEs for write faults ---
commit e62e2d7a0bc9f9a9b0ed15d3b6506aec1a894539
Author: Konstantin Fastov <kfastov@gmail.com>

    drm/gem-shmem: Install writable PTEs for write faults
    
    Since the introduction of dirty tracking, drm_gem_shmem_vm_ops has a
    .pfn_mkwrite handler, which causes read-only PTEs to be installed even
    for write faults. This commit fixes the issue by directly installing
    writable PTEs for write faults and recording the write for dirty tracking.
    This mirrors a similar fix previously applied to the PMD path.

    Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
    Cc: stable@vger.kernel.org
    Closes: https://github.com/AsahiLinux/linux/issues/560
    Assisted-by: Claude:claude-fable-5
    Signed-off-by: Konstantin Fastov <kfastov@gmail.com>

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 06d019d51d3e6..2afef0b9bb5b0 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -593,7 +593,25 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  				 unsigned long pfn)
>  {
>  	if (!order) {
> -		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> +		vm_fault_t ret;
> +
> +		/* .pfn_mkwrite enables write-notify, so vm_page_prot lacks
> +		 * the write bit and a plain vmf_insert_pfn() would install
> +		 * a read-only PTE even for a write fault.  Not every fault
> +		 * resolver refaults into .pfn_mkwrite() to upgrade it (KVM
> +		 * doesn't), so install a writable PTE and record the write
> +		 * directly, like the PMD path below.
> +		 */
> +		if (vmf->flags & FAULT_FLAG_WRITE) {
> +			ret = vmf_insert_pfn_mkwrite(vmf->vma, vmf->address,
> +						     pfn);
> +			if (ret == VM_FAULT_NOPAGE)
> +				drm_gem_shmem_record_mkwrite(vmf);

[Severity: High]
This is a pre-existing issue, but does drm_gem_shmem_record_mkwrite()
calculate the correct page offset for split VMAs?

In drm_gem_shmem_record_mkwrite(), the page offset is calculated as:
    pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff;

If userspace maps a GEM shmem object and then splits the VMA (e.g., via a
partial munmap or mprotect), this yields the offset relative to the VMA
start rather than the GEM object. This causes the wrong folio to be marked
dirty, leading to potential data loss if the actually modified pages aren't
written back.

A similar issue appears in drm_gem_shmem_any_fault(), which also calculates
page_offset as vmf->pgoff - vma->vm_pgoff, causing subsequent faults on the
remaining split VMA to map the wrong physical pages.

> +		} else {
> +			ret = vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> +		}
> +
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804140403.16337-1-kfastov@gmail.com?part=1