Re: [PATCH 6/6] drm/xe/pagefault: add CPU mirror VMA for SVA faults without PRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/xe: NULL deref and uninitialized PAT in xe_vm_alloc_cpu_addr_mirror_vma()
- [High] drm/xe: 4K VMA check before 64K mirror alloc in xe_pagefault_service()
--

commit c441331b0c386c437e9ad479c73465c57bd86054
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/pagefault: add CPU mirror VMA for SVA faults without PRI

This commit adds on-demand creation of a cpu-address-mirror vma in
xe_pagefault_service() when servicing sva faults on platforms with ats/pasid
enabled but without pri support. It also introduces the
XE_PAGEFAULT_ERROR_VMA_ALLOC error code to report vma allocation failures.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index 5247321c805ec..fbdf12d3d5580 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
[ ... ]
> @@ -298,6 +300,51 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>  	}
>  
>  	vma = xe_vm_find_vma_by_addr(vm, xe_pagefault_addr(pf));
> +
> +	/*
> +	 * ATS/PASID is negotiated but this hardware has no PRI, so there is
> +	 * no IOMMU self-healing path to fault the page in - the driver has
> +	 * to create the missing CPU-address-mirror binding itself and let
> +	 * the SVM path below service the fault.
> +	 */
> +	if (!vma && vm->sva_handle && xe_pci_svm_needs_sw_iopf(pdev)) {
> +		u64 page_size = vm->flags & XE_VM_FLAG_64K ? SZ_64K : SZ_4K;
> +		u64 start = ALIGN_DOWN(xe_pagefault_addr(pf), page_size);
> +		u64 end = start + page_size;
> +		int alloc_err = 0;
> +
> +		up_read(&vm->lock);
> +		down_write(&vm->lock);
> +		/* Lock was dropped, re-check: a racing fault may have won */
> +		vma = xe_vm_find_vma_by_addr(vm, xe_pagefault_addr(pf));

[Severity: High]
When vm->flags & XE_VM_FLAG_64K is set, xe_pagefault_service() aligns start
and end to SZ_64K, but xe_vm_find_vma_by_addr() in drivers/gpu/drm/xe/xe_vm.c
only checks a 4k range at the faulting address:

drivers/gpu/drm/xe/xe_vm.c:xe_vm_find_vma_by_addr() {
	return xe_vm_find_overlapping_vma(vm, page_addr, SZ_4K);
}

Because vm_bind_ioctl_check_args() in drivers/gpu/drm/xe/xe_vm.c and
madvise_args_are_sane() in drivers/gpu/drm/xe/xe_vm_madvise.c allow
4k-aligned mappings on 64k vms (xe_vm_bind_ioctl_validate_bo() only requires
64k alignment for bos with XE_BO_FLAG_INTERNAL_64K), another 4k-aligned vma
(such as a system-memory bo, userptr, sparse vma, or madvise-split vma) can
already exist elsewhere within [start, end).

If a fault hits an unmapped 4k page in that 64k window without first checking
xe_vm_find_overlapping_vma(vm, start, page_size), won't
xe_vm_alloc_cpu_addr_mirror_vma() -> xe_vm_alloc_vma() ->
drm_gpuvm_sm_map_ops_create() generate unmap or remap operations that destroy
the overlapping vma via xe_vma_destroy() without unbinding its page tables or
holding bo->ttm.base.resv, triggering xe_bo_assert_held(bo) in
xe_vma_destroy()?

> +		if (!vma) {
> +			if (xe_vm_is_closed_or_banned(vm)) {
> +				alloc_err = -ENOENT;
> +			} else {
> +				/* Merge with adjacent mirror VMAs instead of leaving singletons */
> +				xe_vm_find_cpu_addr_mirror_vma_range(vm, &start, &end);
> +				alloc_err = xe_vm_alloc_cpu_addr_mirror_vma(vm, start, end - start);

[Severity: Critical]
What happens in xe_pagefault_service() when a fault occurs at an address with
no immediately adjacent default-attribute mirror vma at start - SZ_4K or
end + 1 (such as the first sva page fault in the vm)?

In that case, xe_vm_find_cpu_addr_mirror_vma_range() leaves [start, end)
unchanged so the range does not overlap any existing gpuva in vm->gpuvm.

When xe_vm_alloc_cpu_addr_mirror_vma() calls xe_vm_alloc_vma(vm, &map_req,
false) in drivers/gpu/drm/xe/xe_vm.c, vma_flags is initialized to 0 and
default_pat is left uninitialized on the stack, because both are only
populated from an existing vma during DRM_GPUVA_OP_UNMAP or
DRM_GPUVA_OP_REMAP:

drivers/gpu/drm/xe/xe_vm.c:xe_vm_alloc_vma() {
	unsigned int vma_flags = 0;
	bool remap_op = false;
	struct xe_vma_mem_attr tmp_attr = {};
	u16 default_pat;
	...
		if (!is_madvise) {
			if (__op->op == DRM_GPUVA_OP_UNMAP) {
				vma = gpuva_to_vma(op->base.unmap.va);
				XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
				default_pat = vma->attr.default_pat_index;
				vma_flags = vma->gpuva.flags;
			}

			if (__op->op == DRM_GPUVA_OP_REMAP) {
				vma = gpuva_to_vma(op->base.remap.unmap->va);
				default_pat = vma->attr.default_pat_index;
				vma_flags = vma->gpuva.flags;
			}

			if (__op->op == DRM_GPUVA_OP_MAP) {
				op->map.vma_flags |= vma_flags & XE_VMA_CREATE_MASK;
				op->map.pat_index = default_pat;
			}
		}
}

Because drm_gpuvm_sm_map_ops_create() only emits DRM_GPUVA_OP_MAP when no
existing gpuva overlaps [start, end), op->map.vma_flags remains 0 (missing
XE_VMA_SYSTEM_ALLOCATOR) and op->map.pat_index uses the uninitialized
default_pat variable.

From there, vm_bind_ioctl_ops_parse() -> new_vma() -> xe_vma_create() in
drivers/gpu/drm/xe/xe_vm.c sees !bo && !is_null && !is_cpu_addr_mirror,
allocates a struct xe_userptr_vma instead of a mirror vma, and calls
xe_userptr_setup() in drivers/gpu/drm/xe/xe_userptr.c:

drivers/gpu/drm/xe/xe_userptr.c:xe_userptr_setup() {
	...
	err = mmu_interval_notifier_insert(&userptr->notifier, current->mm,
					   start, range,
					   &vma_userptr_notifier_ops);
}

Since xe_pagefault_service() runs on a workqueue kthread where current->mm is
NULL, won't this cause a null pointer dereference inside
mmu_interval_notifier_insert()?

> +			}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021041117.1774877-1-naresh.kumar.g@intel.com?part=6