Re: [PATCH v2 3/3] drm/xe/oa: Quiesce OA streams on driver unbind and PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/xe/oa: NULL deref and dummy page race in xe_oa_buffer_fault()
- [High] drm/xe/oa: mapped OA BO left DMA-mapped and un-evicted in xe_oa_quiesce()
- [High] drm/xe/oa: ungated xe_bo_vm_access() in xe_oa_vm_ops after quiesce
- [High] drm/xe/oa: race between in-flight observation ioctls and xe_oa_fini()
- [High] drm/xe/oa: lock inversion holding mmap_mutex across xe_oa_quiesce_oa_unit()
- [High] drm/xe/oa: unwaited LRI job in xe_oa_quiesce_oa_unit() races devres teardown
--

commit 6eef6267f9216029752eabf4c50aa7de5a8ca9d1
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/oa: Quiesce OA streams on driver unbind and PCI reset

This commit adds xe_oa_quiesce() to disable active OA streams, cancel their
poll timers, unmap OA buffers, and release device-bound resources during
driver unbind and PCI slot reset when xe_oa_fini() runs.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 4916101fce541..6faa40d7ebbe7 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c

[ ... ]

> @@ -1731,8 +1775,11 @@ static vm_fault_t xe_oa_buffer_fault(struct vm_fault *vmf)
>  	 * Once the stream is dead its OA buffer may already be gone (or about
>  	 * to be). Refuse to (re-)install a PTE rather than letting the
>  	 * generic BO fault handler touch a buffer that's being torn down.
> +	 * drm_dev_unplug() zaps the mapping before the stream is marked dead,
> +	 * so also refuse once unplugged, the generic handler would install a
> +	 * dummy page.
>  	 */
> -	if (READ_ONCE(stream->dead))
> +	if (READ_ONCE(stream->dead) || drm_dev_is_unplugged(&stream->oa->xe->drm))
>  		return VM_FAULT_SIGBUS;
>  
>  	return xe_bo_cpu_fault(vmf);

[Severity: High]
Can xe_oa_buffer_fault() dereference a NULL pointer when racing with
xe_oa_fini(), or still install a dummy page PTE?

If a fault enters xe_oa_buffer_fault() and observes READ_ONCE(stream->dead)
as false just before xe_oa_quiesce_oa_unit() sets stream->dead = true,
xe_oa_fini() can finish xe_oa_quiesce() and clear oa->xe:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_fini() {
	...
	xe_oa_quiesce(xe);

	idr_for_each(&oa->metrics_idr, destroy_config, oa);
	idr_destroy(&oa->metrics_idr);

	oa->xe = NULL;
}

When xe_oa_buffer_fault() then evaluates the second operand of the logical
or, stream->oa->xe is NULL, so &stream->oa->xe->drm is NULL and
drm_dev_is_unplugged() dereferences NULL in drm_dev_enter() (whereas
xe_oa_mmap(), xe_oa_destroy_locked(), and xe_oa_release() use
gt_to_xe(stream->gt)).

In addition, drm_dev_is_unplugged() immediately drops its SRCU read lock via
drm_dev_exit(). If drm_dev_unplug() and drm_vma_node_unmap() complete in the
window between drm_dev_is_unplugged() in xe_oa_buffer_fault() and
drm_dev_enter() in xe_bo_cpu_fault(), or if the device is wedged before
unplug (such as during PCI error recovery), xe_bo_cpu_fault() still hits:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_cpu_fault() {
	...
	if (xe_wedged(xe) || !drm_dev_enter(&xe->drm, &idx))
		return ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot);

and installs a dummy page PTE instead of returning VM_FAULT_SIGBUS.

> @@ -1745,18 +1792,12 @@ static const struct vm_operations_struct xe_oa_vm_ops = {
>  	.access	= xe_bo_vm_access,
>  };

[Severity: High]
What prevents userspace from accessing the OA buffer via xe_bo_vm_access()
after xe_oa_quiesce()?

While xe_oa_vm_ops gates .fault with xe_oa_buffer_fault(), .access is wired
directly to xe_bo_vm_access():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_vm_access() {
	struct ttm_buffer_object *ttm_bo = vma->vm_private_data;
	struct xe_bo *bo = ttm_to_xe_bo(ttm_bo);
	struct xe_device *xe = xe_bo_device(bo);

	guard(xe_pm_runtime)(xe);
	return ttm_bo_vm_access(vma, addr, buf, len, write);
}

Because an existing VMA holds a GEM reference to stream->oa_buffer.bo in
vma->vm_private_data until munmap() or process exit, reading the VMA via
/proc/self/mem, ptrace, or process_vm_readv() after xe_oa_quiesce() will get
VM_FAULT_SIGBUS from xe_oa_buffer_fault() and fall back to
vma->vm_ops->access() -> xe_bo_vm_access(). That acquires runtime PM on the
removed device and accesses the BO backing store (including unmapped
vram->mapping or torn-down migration state when the OA buffer is in VRAM
under mert_wa_14026633728()). Should .access also be wrapped to check
stream->dead and drm_dev_enter()?

[ ... ]

> @@ -1925,6 +1988,12 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
>  	int stream_fd;
>  	int ret;
>  
> +	if (param->hwe->gt->oa.quiesced) {
> +		drm_dbg(&oa->xe->drm, "OA is quiesced\n");
> +		ret = -ENODEV;
> +		goto exit;
> +	}
> +

[Severity: High]
Could an in-flight observation ioctl that entered drm_ioctl() before
drm_dev_unplug() race with xe_oa_fini() before reaching gt_lock?

In xe_oa_stream_open_ioctl(), oa->xe is only checked unlocked at entry before
sleeping copy_from_user() calls in xe_oa_user_extensions() and before taking
gt_lock:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_stream_open_ioctl() {
	...
	if (!oa->xe) {
		drm_dbg(&xe->drm, "xe oa interface not available for this system\n");
		return -ENODEV;
	}

	param.xef = xef;
	param.period_exponent = -1;
	ret = xe_oa_user_extensions(oa, XE_OA_USER_EXTN_FROM_OPEN, data, 0, &param);
	if (ret)
		return ret;

	/* If not provided, OA unit defaults to OA unit 0 as per uapi */
	if (!param.oa_unit)
		param.oa_unit = &xe_root_mmio_gt(oa->xe)->oa.oa_unit[0];

If xe_oa_fini() runs concurrently and sets oa->xe = NULL right after
xe_oa_quiesce(), an in-flight xe_oa_stream_open_ioctl() can dereference NULL
oa->xe in xe_oa_lookup_oa_unit(), xe_root_mmio_gt(oa->xe),
oa_unit_supports_oa_format(), or even in drm_dbg(&oa->xe->drm, ...) inside
xe_oa_stream_open_ioctl_locked() above. When param.period_exponent >= 0,
xe_oa_stream_open_ioctl() also calls oa_exponent_to_ns() ->
xe_oa_timestamp_frequency() -> xe_mmio_read32() before acquiring gt_lock and
checking gt->oa.quiesced.

Similarly, in-flight xe_oa_add_config_ioctl() and xe_oa_remove_config_ioctl()
calls that already passed their unlocked !oa->xe check can dereference NULL
oa->xe in xe_oa_is_valid_config_reg() or access oa->metrics_idr while
xe_oa_fini() destroys oa->metrics_idr without holding oa->metrics_lock.

[ ... ]

> @@ -2617,6 +2686,92 @@ static void xe_oa_unregister(void *arg)
>  	oa->metrics_kobj = NULL;
>  }
>  
> +static void xe_oa_quiesce_oa_unit(struct xe_oa_unit *u, bool hw_accessible)
> +{
> +	struct xe_oa_stream *stream = u->exclusive_stream;
> +	unsigned long flags;
> +
> +	if (!stream)
> +		return;
> +
> +	mutex_lock(&stream->stream_lock);
> +	mutex_lock(&stream->mmap_mutex);
> +	if (stream->enabled && hw_accessible)
> +		xe_oa_disable_locked(stream);
> +
> +	stream->enabled = false;
> +
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
> +	WRITE_ONCE(stream->dead, true);
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> +
> +	if (stream->sample)
> +		hrtimer_cancel(&stream->poll_check_timer);
> +
> +	wake_up_all(&stream->poll_wq);
> +
> +	/* Zap userspace mappings of this OA buffer only, others in the address_space stay */
> +	drm_vma_node_unmap(&stream->oa_buffer.bo->ttm.base.vma_node,
> +			   stream->oa->xe->drm.anon_inode->i_mapping);
> +
> +	/*
> +	 * Restore the HW state while the stream still holds its runtime PM ref and exec_q.
> +	 * The device is not reset on unbind, so this can't be left to a later reset.
> +	 */
> +	if (hw_accessible)
> +		xe_oa_stream_hw_teardown(stream);
> +
> +	xe_oa_stream_free_resources(stream);

[Severity: High]
Does holding stream->mmap_mutex across xe_oa_stream_hw_teardown() and
xe_oa_stream_free_resources() introduce a lock ordering inversion with
mm->mmap_lock?

In xe_oa_mmap(), stream->mmap_mutex is taken inside file->f_op->mmap under
mm->mmap_lock, which is why struct xe_oa_stream documents mmap_mutex as a
leaf lock.

However, xe_oa_quiesce_oa_unit() holds stream->mmap_mutex across:
- xe_oa_stream_hw_teardown() -> xe_oa_disable_metric_set() ->
  xe_oa_configure_oa_context() -> xe_oa_load_with_lri() ->
  xe_oa_submit_bb() -> xe_oa_lock_vma(), which takes down_read(&q->vm->lock)
  and xe_vm_lock(q->vm, false)
- xe_oa_stream_free_resources() -> xe_oa_free_oa_buffer() ->
  xe_bo_unpin_map_no_vm(), which locks the BO dma_resv

Since normal VM and fault paths establish vm->lock -> mm->mmap_lock ->
dma_resv, taking vm->lock and dma_resv under stream->mmap_mutex inverts the
lock hierarchy. Since xe_oa_mmap_locked() only checks READ_ONCE(stream->dead)
and maps stream->oa_buffer.bo, could stream->mmap_mutex be held only around
setting stream->dead = true?

[Severity: High]
When userspace has an active mmap VMA on the OA stream during unbind or PCI
slot reset, does stream->oa_buffer.bo miss both eviction and DMA unmapping?

In xe_oa_mmap_locked(), drm_gem_mmap_obj() takes a GEM reference on
stream->oa_buffer.bo that is held by vma->vm_private_data until munmap() or
process exit. During device removal:

1. xe_device_remove() calls xe_bo_pci_dev_remove_all(xe) before devm actions
   run. At that point stream->oa_buffer.bo is still pinned (pin_count == 1 on
   xe->pinned.early.kernel_bo_present), so ttm_resource_manager_evict_all()
   skips it.
2. Next, devres runs xe_oa_fini() -> xe_oa_quiesce_oa_unit() ->
   xe_oa_stream_free_resources() -> xe_oa_free_oa_buffer() ->
   xe_bo_unpin_map_no_vm(stream->oa_buffer.bo). Because drm_vma_node_unmap()
   only zaps PTEs without closing the VMA, xe_bo_unpin_map_no_vm() unpins the
   BO, removes bo->pinned_link from xe->pinned.early.kernel_bo_present, and
   drops one reference, leaving the BO alive with refcount >= 1.
3. When the devm action xe_bo_pinned_fini() subsequently runs to call
   xe_bo_dma_unmap_pinned() on kernel_bo_present, stream->oa_buffer.bo is no
   longer on the list.

As a result, the OA BO remains allocated with its DMA mapping and TTM
resource active after PCI device removal until userspace closes the VMA.

[Severity: High]
In xe_oa_quiesce_oa_unit(), can the asynchronous batchbuffer submitted by
xe_oa_stream_hw_teardown() race with subsequent devres teardown of GuC
submission and fence IRQs?

When hw_accessible is true and stream->exec_q is set,
xe_oa_stream_hw_teardown() calls:

  xe_oa_disable_metric_set()
    -> xe_oa_configure_oa_context(stream, false)
      -> xe_oa_load_with_lri()

which submits a batchbuffer job onto stream->exec_q via xe_oa_submit_bb(),
hands the unsignaled fence to xe_bb_free(bb, fence), and returns immediately
without waiting for the fence to signal.

xe_oa_quiesce_oa_unit() then immediately drops forcewake, the runtime PM
reference, and stream->exec_q, and returns to devres_release_all(), which
proceeds to guc_submit_fini() and xe_gt_fini() -> xe_hw_fence_irq_finish().
If the scheduler arms job->fence onto gt->fence_irq[i].pending before GuC
teardown / TDR signals it, won't xe_hw_fence_irq_finish() trigger
XE_WARN_ON(!list_empty(&irq->pending)) or leave the context restore LRI
unexecuted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030223056.2266638-5-umesh.nerlige.ramappa@intel.com?part=3