On 9/16/26 12:02, Yifan Zhang wrote: > GPU recovery invalidates CPU mappings before resetting the device. A > concurrent GEM fault can install a new mapping after invalidation and > allow userspace to access the framebuffer BAR while the device is being > reset. Serialize PFN insertion with the reset-domain write semaphore to > close this race. > > Holding the read semaphore across the entire fault is unsafe: migration > fence waits may depend on recovery, and page-table allocation inside > vmf_insert_pfn_prot() can enter reclaim and wait for GPU fences through > MMU notifier callbacks. Recovery cannot complete those fences while it > is blocked acquiring the write semaphore. > > Add an optional mapping semaphore to the TTM reserved fault helper. > Prepare page tables with apply_to_page_range() before taking the read > semaphore, then insert the PFN through vmf_insert_pfn_prot(). Restrict > this path to non-COW PFNMAP VMAs with mmap_lock held, relying on mapping > invalidation clearing PTEs without freeing the prepared page tables. > Keep the existing MM PFN permission and cache-mode checks, and leave > fence waits, BO population and page-table allocation outside reset > exclusion. > > On semaphore contention, retry the fault when allowed. Drop the BO > reservation and mmap_lock before waiting, and retain an extra GEM > reference to protect the object and semaphore lifetime. RETRY_NOWAIT > keeps both locks held; faults that cannot retry return SIGBUS. Keep the > existing TTM helper as an unlocked wrapper so other callers retain their > original behavior, and opt AMDGPU GEM faults into the new helper.
Clear NAK to the whole thing. That violates core kernel locking and to be honest the implementation is just a mess. There was already a dicussion ongoing upstream which rejected that approach. Please don't try to fix up something without knowing the full technical background. Regards, Christian. > > Signed-off-by: Yifan Zhang <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 104 +++++++++++++++++++++++- > include/drm/ttm/ttm_bo.h | 3 + > 3 files changed, 109 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > index 0d579517c03c..2937f6cda62a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > @@ -119,6 +119,7 @@ amdgpu_gem_update_timeline_node(struct drm_file *filp, > static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf) > { > struct ttm_buffer_object *bo = vmf->vma->vm_private_data; > + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev); > struct drm_device *ddev = bo->base.dev; > vm_fault_t ret; > int idx; > @@ -134,8 +135,9 @@ static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf) > goto unlock; > } > > - ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, > - TTM_BO_VM_NUM_PREFAULT); > + ret = ttm_bo_vm_fault_reserved_lock(vmf, vmf->vma->vm_page_prot, > + TTM_BO_VM_NUM_PREFAULT, > + &adev->reset_domain->sem); > > drm_dev_exit(idx); > } else { > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c > index a80510489c45..e9ac14c471ab 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c > @@ -162,6 +162,70 @@ vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object > *bo, > } > EXPORT_SYMBOL(ttm_bo_vm_reserve); > > +static vm_fault_t ttm_bo_vm_mapping_retry(struct ttm_buffer_object *bo, > + struct vm_fault *vmf, > + struct rw_semaphore *mapping_lock) > +{ > + if (!(vmf->flags & FAULT_FLAG_ALLOW_RETRY)) > + return VM_FAULT_SIGBUS; > + > + if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT) > + return VM_FAULT_RETRY; > + > + drm_gem_object_get(&bo->base); > + dma_resv_unlock(bo->base.resv); > + mmap_read_unlock(vmf->vma->vm_mm); > + if (!down_read_killable(mapping_lock)) > + up_read(mapping_lock); > + drm_gem_object_put(&bo->base); > + > + return VM_FAULT_RETRY; > +} > + > +/** > + * ttm_bo_vm_insert_pfn - prepare page tables before taking mapping exclusion > + * @vma: non-COW PFNMAP VMA, protected by mmap_lock > + * @address: page-aligned address within @vma > + * @pfn: backing page frame > + * @prot: protection for this page > + * @mapping_lock: semaphore held for writing while invalidating mappings > + * > + * The empty page tables must survive until insertion: mmap_lock excludes > + * munmap/mremap, PFNMAP excludes MADV_DONTNEED and THP collapse, and mapping > + * invalidation clears PTEs without freeing page tables. Thus the subsequent > + * vmf_insert_pfn_prot() does not allocate page tables under @mapping_lock, > + * while retaining the MM's PFN permission and cache-mode checks. > + * > + * Return: VM_FAULT_RETRY on contention, with all caller-owned locks > retained; > + * otherwise the insertion result or an allocation/validation error. > + */ > +static vm_fault_t ttm_bo_vm_insert_pfn(struct vm_area_struct *vma, > + unsigned long address, unsigned long pfn, > + pgprot_t prot, > + struct rw_semaphore *mapping_lock) > +{ > + vm_fault_t ret; > + int err; > + > + mmap_assert_locked(vma->vm_mm); > + if ((vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) != VM_PFNMAP || > + is_cow_mapping(vma->vm_flags)) > + return VM_FAULT_SIGBUS; > + if (address < vma->vm_start || address >= vma->vm_end) > + return VM_FAULT_SIGBUS; > + > + err = apply_to_page_range(vma->vm_mm, address, PAGE_SIZE, NULL, NULL); > + if (err) > + return err == -ENOMEM ? VM_FAULT_OOM : VM_FAULT_SIGBUS; > + > + if (!down_read_trylock(mapping_lock)) > + return VM_FAULT_RETRY; > + ret = vmf_insert_pfn_prot(vma, address, pfn, prot); > + up_read(mapping_lock); > + > + return ret; > +} > + > /** > * ttm_bo_vm_fault_reserved - TTM fault helper > * @vmf: The struct vm_fault given as argument to the fault callback > @@ -183,6 +247,33 @@ EXPORT_SYMBOL(ttm_bo_vm_reserve); > vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf, > pgprot_t prot, > pgoff_t num_prefault) > +{ > + return ttm_bo_vm_fault_reserved_lock(vmf, prot, num_prefault, NULL); > +} > +EXPORT_SYMBOL(ttm_bo_vm_fault_reserved); > + > +/** > + * ttm_bo_vm_fault_reserved_lock - fault a BO with optional mapping exclusion > + * @vmf: fault information, with mmap_lock held > + * @prot: page protection > + * @num_prefault: maximum number of pages to prefault > + * @mapping_lock: optional semaphore held for writing during invalidation > + * > + * The BO must be reserved. The caller must not hold @mapping_lock. Its > lifetime > + * must be protected by the BO's reference to the DRM device. When supplied, > + * only non-COW PFNMAP mappings are supported. Fence waits, BO population and > + * page-table allocation run without the semaphore. > + * > + * On contention, ALLOW_RETRY drops the BO reservation and mmap_lock before > + * waiting, except RETRY_NOWAIT which retains both locks. Without > ALLOW_RETRY, > + * contention returns SIGBUS with both locks held rather than blocking reset. > + * > + * Return: Fault result. VM_FAULT_RETRY without RETRY_NOWAIT means both locks > + * have been dropped; otherwise both locks remain held. > + */ > +vm_fault_t ttm_bo_vm_fault_reserved_lock(struct vm_fault *vmf, pgprot_t prot, > + pgoff_t num_prefault, > + struct rw_semaphore *mapping_lock) > { > struct vm_area_struct *vma = vmf->vma; > struct ttm_buffer_object *bo = vma->vm_private_data; > @@ -263,7 +354,16 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf, > * at arbitrary times while the data is mmap'ed. > * See vmf_insert_pfn_prot() for a discussion. > */ > - ret = vmf_insert_pfn_prot(vma, address, pfn, prot); > + if (mapping_lock) { > + ret = ttm_bo_vm_insert_pfn(vma, address, pfn, prot, > + mapping_lock); > + if (ret == VM_FAULT_RETRY) > + return ttm_bo_vm_mapping_retry(bo, vmf, > mapping_lock); > + if (ret & VM_FAULT_ERROR) > + return i ? VM_FAULT_NOPAGE : ret; > + } else { > + ret = vmf_insert_pfn_prot(vma, address, pfn, prot); > + } > > /* Never error on prefaulted PTEs */ > if (unlikely((ret & VM_FAULT_ERROR))) { > @@ -279,7 +379,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf, > } > return ret; > } > -EXPORT_SYMBOL(ttm_bo_vm_fault_reserved); > +EXPORT_SYMBOL_GPL(ttm_bo_vm_fault_reserved_lock); > > static void ttm_bo_release_dummy_page(struct drm_device *dev, void *res) > { > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h > index 8310bc3d55f9..06138c1688f6 100644 > --- a/include/drm/ttm/ttm_bo.h > +++ b/include/drm/ttm/ttm_bo.h > @@ -438,6 +438,9 @@ vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo, > vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf, > pgprot_t prot, > pgoff_t num_prefault); > +vm_fault_t ttm_bo_vm_fault_reserved_lock(struct vm_fault *vmf, pgprot_t prot, > + pgoff_t num_prefault, > + struct rw_semaphore *mapping_lock); > vm_fault_t ttm_bo_vm_fault(struct vm_fault *vmf); > void ttm_bo_vm_open(struct vm_area_struct *vma); > void ttm_bo_vm_close(struct vm_area_struct *vma);
