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 <[email protected]> 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/[email protected]?part=3
