Re: [PATCH v2] drm/xe/vm: Handle purged BOs in VM snapshots

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Delayed BO reference drop due to `IS_ERR` skip in deferred worker.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `xe_vm_snapshot_capture()` leading to a Use-After-Free of `ttm_resource`.
--

commit a4a76e70071a5c5421a95bba41a63f025685beaf
Author: Arvind Yadav <arvind.yadav@intel.com>
drm/xe/vm: Handle purged BOs in VM snapshots

A purged BO can remain in a VM after its TTM resource has been freed.
xe_vm_snapshot_capture() can then dereference a NULL resource while
capturing a devcoredump. Record -ENODATA for the affected VMA instead
of dereferencing the resource.

Closes: https://sashiko.dev/#/patchset/20260831064942.315720-17-tejas.upadhyay%40intel.com?part=4

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 19b3d0be79282..fcdd34c4f0fa2 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -4663,20 +4663,29 @@ struct xe_vm_snapshot *xe_vm_snapshot_capture(struct xe_vm *vm)
>  			XE_VM_SNAP_FLAG_READ_ONLY : 0;
>  		snap->snap[i].pat_index = vma->attr.pat_index;
>  		if (bo) {
> +			struct ttm_resource *res;
> +
>  			snap->snap[i].cpu_caching = bo->cpu_caching;
>  			snap->snap[i].bo = xe_bo_get(bo);
>  			snap->snap[i].bo_ofs = xe_vma_bo_offset(vma);
> -			switch (bo->ttm.resource->mem_type) {
> -			case XE_PL_SYSTEM:
> -			case XE_PL_TT:
> -				snap->snap[i].uapi_mem_region = 0;
> -				break;
> -			case XE_PL_VRAM0:
> -				snap->snap[i].uapi_mem_region = 1;
> -				break;
> -			case XE_PL_VRAM1:
> -				snap->snap[i].uapi_mem_region = 2;
> -				break;
> +			snap->snap[i].uapi_mem_region = -1;
> +
> +			res = READ_ONCE(bo->ttm.resource);
> +			if (!res) {
> +				snap->snap[i].data = ERR_PTR(-ENODATA);

[Severity: Medium]
Does setting this to ERR_PTR(-ENODATA) cause a delayed BO reference drop?

In xe_vm_snapshot_capture_delayed(), there is an early check that skips
the cleanup label:

	if (IS_ERR(snap->snap[i].data) ||
	    snap->snap[i].flags & XE_VM_SNAP_FLAG_IS_NULL)
		continue;

If this continue statement is hit, will it bypass cleanup_bo: and
xe_bo_put(bo), leaving the xe_bo metadata pinned for the lifetime of the
devcoredump?

> +			} else {
> +				switch (res->mem_type) {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
Time-of-Check to Time-of-Use (TOCTOU) race condition to occur here in
xe_vm_snapshot_capture()?

Since the res pointer is cached locklessly via READ_ONCE(), could a concurrent
execution of the shrinker (xe_bo_shrink()) lock the BO and purge it, calling
ttm_resource_free() to free the resource memory before it is dereferenced?

Would this result in a Use-After-Free when evaluating switch (res->mem_type)?

> +				case XE_PL_SYSTEM:
> +				case XE_PL_TT:
> +					snap->snap[i].uapi_mem_region = 0;
> +					break;
> +				case XE_PL_VRAM0:
> +					snap->snap[i].uapi_mem_region = 1;
> +					break;
> +				case XE_PL_VRAM1:
> +					snap->snap[i].uapi_mem_region = 2;
> +					break;
> +				}
>  			}
>  		} else if (xe_vma_is_userptr(vma)) {
>  			struct mm_struct *mm =

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904072252.4009491-1-arvind.yadav@intel.com?part=1