Re: [PATCH v2 3/4] drm/xe: Fix up BO TTM priority for late VM binds

[sashiko-bot@kernel.org]

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

New issues:
- [High] Data race and TTM LRU corruption due to missing invariant check for cross-VM binds of private BOs.

Pre-existing issues:
- [Medium] Meaningful code and commit message mismatch (Incomplete Implementation of extobj priority lowering).
--

--- Patch [3]: [PATCH v2 3/4] drm/xe: Fix up BO TTM priority for late VM binds ---
commit b5439d40abd0e7b0ac5ecc4fc4045178d3dcfcb7
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Fix up BO TTM priority for late VM binds

This patch updates the BO TTM priority when a BO is (re)mapped into a VM.
It recalculates the priority band based on the VM's current exec queues or
sets it to the highest priority for external objects.

>     A BO's TTM priority is only ever set once, at creation time, based on
>     the priority band of exec queues attached to its VM at that moment
>     (xe_vm_bo_priority()), or XE_BO_PRIORITY_HIGHEST for an extobj not
>     tied to any single VM. This misses the common flow of VM create -> BO
>     create -> exec queue create -> VM bind: the BO is created before any
>     exec queue exists on the VM (so it gets XE_BO_PRIORITY_HIGH, the "no
>     exec queues yet" default), the exec queue is added afterwards
>     (xe_vm_add_exec_queue() only re-prioritizes BOs already bound to the
>     VM at that time), and only then is the BO bound into the VM via
>     VM_BIND, leaving it stuck at a stale priority that no longer reflects
>     the VM's exec queues.
>     
>     It also misses a second flow specific to extobjs: create BO with no
>     vm_id (bo->vm == NULL) -> bind -> unbind -> rebind. The unbind can
>     lower the BO's priority to XE_BO_PRIORITY_LOW once it has no VMA
>     mappings left anywhere (see xe_vma_destroy()), but on rebind nothing

[Severity: Medium]
This is a pre-existing issue, but I don't see any logic in xe_vma_destroy()
or elsewhere in the codebase that actually lowers an extobj's priority to
XE_BO_PRIORITY_LOW.

Because extobjs appear to permanently retain XE_BO_PRIORITY_HIGHEST, won't
the fix in op_update_bo_priority() always hit the early return for extobjs,
missing the intended working set eviction optimization entirely?

>     brought it back up, leaving a still-useful, freshly rebound extobj
>     stuck at the lowest priority, an easy target for eviction/shrinking.
>     
>     Add op_update_bo_priority(), called from vm_bind_ioctl_ops_fini() for

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 0f2b8ffefb8ec..dd64a46b554ed 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c

[ ... ]

> @@ -3538,6 +3538,38 @@ static void op_add_ufence(struct xe_vm *vm, struct xe_vma_op *op,
>  	}
>  }
>  
> +/*
> + * A BO's TTM priority is set when it is created, based on the priority band
> + * of @vm's exec queues at that time (for a private BO), or to the highest
> + * level (for an extobj, bo->vm == NULL). If a BO is (re)mapped into @vm
> + * afterwards, its priority may be stale: a private BO's VM may have gained
> + * or lost exec queues since, and an extobj may have been lowered to
> + * XE_BO_PRIORITY_LOW by xe_vma_destroy() after losing all of its VMA
> + * mappings. Bring it in line with the current priority band here.
> + */
> +static void op_update_bo_priority(struct xe_vm *vm, struct xe_vma_op *op)
> +{
> +	struct xe_bo *bo;
> +	int priority;
> +
> +	if (op->base.op != DRM_GPUVA_OP_MAP)
> +		return;
> +
> +	bo = xe_vma_bo(op->map.vma);
> +	if (!bo)
> +		return;
> +
> +	/* Private BOs are only ever bound to the VM they were created for. */
> +	xe_assert(vm->xe, !bo->vm || bo->vm == vm);

[Severity: High]
Could this regression allow a data race and TTM LRU list corruption if
userspace binds a private BO associated with another VM?

If userspace binds a private BO associated with VM_A to VM_B, the xe_assert()
doesn't prevent execution from continuing (and may compile out in non-debug
builds).

During the bind operation, vma_lock_and_validate() skips acquiring the BO's
resv lock because it assumes any BO with a non-NULL bo->vm shares the target
VM's lock:

drivers/gpu/drm/xe/xe_vm.c:vma_lock_and_validate() {
    ...
	if (bo) {
		if (!bo->vm)
			err = drm_exec_lock_obj(exec, &bo->ttm.base);
    ...
}

This means op_update_bo_priority() will call xe_bo_update_ttm_priority() to
mutate bo->ttm.priority and TTM LRU structures without holding the required
bo->ttm.base.resv lock. Can an unprivileged userspace application trigger this
by passing a mismatched BO handle to the bind IOCTL?

> +
> +	priority = bo->vm ? xe_vm_bo_priority(vm) : XE_BO_PRIORITY_HIGHEST;
> +	if (bo->ttm.priority == priority)
> +		return;
> +
> +	xe_vm_assert_held(vm);
> +	xe_bo_update_ttm_priority(bo, priority);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715042832.3625096-1-matthew.brost@intel.com?part=3