Re: [PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

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

Pre-existing issues:
- [High] Incorrect page offset calculation causes wrong pages to be mapped after VMA split (munmap/mremap)
--

--- Patch [2]: [PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite() ---
commit e238b92cc06e6221554d7e59a6c25c7b8c19b7ac
Author: Paolo Bonzini <[email protected]>

drm/shmem_helper: use vmf_insert_pfn_mkwrite()

This commit changes the shmem helper to use vmf_insert_pfn_mkwrite() to
ensure KVM or VFIO correctly see a writable PTE when requested. It also
updates the PMD mapping case to pass the write flag instead of recording
the write fault early.

Link: https://lore.kernel.org/kvm/[email protected]/

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index c989459eb2159..c81be3e973173 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]));
>  }

[Severity: High]
This is a pre-existing issue, but does this result in the wrong page being
marked dirty or mapped after a VMA split?

Looking at drm_gem_shmem_record_mkwrite() (line 583) and
drm_gem_shmem_any_fault() (line 638) in drivers/gpu/drm/drm_gem_shmem_helper.c,
page_offset is calculated as:

    pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff;

If userspace maps a GEM object and then splits the VMA (e.g., via a partial
munmap() at the beginning of the VMA), the kernel advances vma->vm_pgoff to
account for the split. When a page fault subsequently occurs in the new VMA,
vmf->pgoff evaluates to the fake offset plus the split offset plus the fault
offset.

This makes the calculation mathematically equivalent to
(address - vma->vm_start) >> PAGE_SHIFT. This completely discards the split
offset, meaning the remaining VMA will erroneously map the GEM object starting
from page 0 again, rather than the correct sub-page.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2