Re: [PATCH v5 7/7] 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 2 potential
issues to consider:
- [High] drm/xe/oa: UAF of tile->mem.ggtt when delayed OA BO destroy
  races drmm teardown
- [High] drm/xe/oa: missing unplug and wedged checks in stream fd
  xe_oa_ioctl()
--

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

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

This commit quiesces active OA streams during driver teardown and fences OA
operations against device unplug so open stream file descriptors can safely
outlive the device.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index d2bd9622109af..6808ccc442b56 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
[ ... ]
> @@ -1124,9 +1149,15 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config
>  	if (err == -ENOENT)
>  		xe_oa_config_cb(fence, &ofence->cb);
>  
> -	/* If nothing needs to be signaled we wait synchronously */
> +	/*
> +	 * If nothing needs to be signaled we wait synchronously. Stop waiting once the device
> +	 * is unplugged, since this holds stream_lock or gt_lock, which the OA quiesce needs,
> +	 * and may run in a drm_dev_enter() section, which drm_dev_unplug() waits for.
> +	 */
>  	if (!num_signal) {
> -		dma_fence_wait(&ofence->base, false);
> +		while (!dma_fence_wait_timeout(&ofence->base, false, HZ / 10))
> +			if (drm_dev_is_unplugged(&gt_to_xe(stream->gt)->drm))
> +				break;
>  		dma_fence_put(&ofence->base);
>  	}
>  
[ ... ]
> @@ -1700,6 +1732,9 @@ static long xe_oa_ioctl_locked(struct xe_oa_stream *stream,
>  			       unsigned int cmd,
>  			       unsigned long arg)
>  {
> +	if (stream->dead)
> +		return -ENODEV;
> +
>  	switch (cmd) {
>  	case DRM_XE_OBSERVATION_IOCTL_ENABLE:
>  		return xe_oa_enable_locked(stream);

[Severity: High]
Does the stream file descriptor's unlocked_ioctl callback (xe_oa_ioctl() in
drivers/gpu/drm/xe/xe_oa.c) also need a drm_dev_enter() section and a
xe_device_wedged() check?

Unlike the observation ioctl on the DRM device fd, the stream fd is an
anonymous inode that bypasses drm_ioctl() and xe_drm_ioctl().  During
xe_pci_remove(), drm_dev_unplug(), xe_bo_pci_dev_remove_all(), xe_pm_fini()
and later-registered devm actions such as xe_device_sanitize() all run
before xe_oa_fini() calls xe_oa_quiesce() to set stream->dead.  Without
drm_dev_enter() in the stream fd's xe_oa_ioctl(), drm_dev_unplug() does not
wait for in-flight stream ioctls, and new calls to xe_oa_enable_locked(),
xe_oa_disable_locked(), or xe_oa_config_locked() can still access MMIO,
GuC, and BO state during teardown.

Similarly, stream->dead is only set in xe_oa_quiesce_oa_unit() and is not
set when the device is declared wedged (including in
prepare_device_for_reset(), which also calls pci_disable_device() before
xe_pci_error_slot_reset() runs).  Without checking xe_device_wedged() in
the stream fd's xe_oa_ioctl() and in the uninterruptible wait loop in
xe_oa_emit_oa_config(), userspace can still issue hardware-touching stream
ioctls on a wedged or PCI-disabled device, and xe_oa_emit_oa_config() can
hang uninterruptibly under stream_lock or gt_lock if the device wedges
while waiting on a fence.

> @@ -1732,6 +1767,14 @@ static long xe_oa_ioctl(struct file *file,
>  
>  static void xe_oa_destroy_locked(struct xe_oa_stream *stream)
>  {
> +	if (stream->dead) {
> +		/* All OA buffer mmaps are gone by now, see xe_oa_quiesce_oa_unit() */
> +		xe_oa_free_oa_buffer(stream);
> +		mutex_destroy(&stream->stream_lock);
> +		kfree(stream);
> +		return;
> +	}
> +
>  	xe_pm_runtime_get(gt_to_xe(stream->gt));
>  
>  	/* A wedged device may be unresponsive, skip HW access */
> @@ -1748,17 +1791,30 @@ static void xe_oa_destroy_locked(struct xe_oa_stream *stream)
>  	kfree(stream);
>  }
>  
> +static void xe_oa_quiesce_oa_unit(struct xe_oa_unit *u, bool hw_accessible);
> +
>  static int xe_oa_release(struct inode *inode, struct file *file)
>  {
>  	struct xe_oa_stream *stream = file->private_data;
>  	struct xe_gt *gt = stream->gt;
> +	struct xe_device *xe = gt_to_xe(gt);
> +	int idx;
>  
>  	mutex_lock(&gt->oa.gt_lock);
> -	xe_oa_destroy_locked(stream);
> +	if (drm_dev_enter(&xe->drm, &idx)) {
> +		/* drm_dev_unplug() waits for this, so xe_oa_fini() can't run under us */
> +		xe_oa_destroy_locked(stream);
> +		drm_dev_exit(idx);
> +	} else {
> +		/* The device is going away, quiesce the stream like xe_oa_fini() does */
> +		if (!stream->dead)
> +			xe_oa_quiesce_oa_unit(stream->oa_unit, !xe_wedged(xe));
> +		xe_oa_destroy_locked(stream);
> +	}
>  	mutex_unlock(&gt->oa.gt_lock);
>  
>  	/* Release the reference the OA stream kept on the driver */
> -	drm_dev_put(&gt_to_xe(gt)->drm);
> +	drm_dev_put(&xe->drm);
>  
>  	return 0;
>  }

[Severity: High]
Can freeing stream->oa_buffer.bo in xe_oa_destroy_locked() race with drmm
teardown and cause a use-after-free on tile->mem.ggtt?

Because xe_oa_quiesce_oa_unit() keeps stream->oa_buffer.bo pinned across
xe_device_remove(), xe_bo_pci_dev_remove_all() skips it and
xe_bo_pinned_fini() only DMA-unmaps it, leaving bo->ttm.ttm and
bo->ggtt_node[] populated.  When the stream fd is later closed and holds
the last reference on xe->drm, xe_oa_free_oa_buffer() calls
xe_bo_unpin_map_no_vm() -> ttm_bo_put() -> ttm_bo_release().  Whenever
want_init_on_free() is true (or dma_resv_trylock() fails), ttm_bo_release()
queues bo->delayed_delete onto xe->ttm.wq instead of calling
xe_ttm_bo_destroy() synchronously:

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_release() {
    ...
		if (!dma_resv_test_signaled(&bo->base._resv,
					    DMA_RESV_USAGE_BOOKKEEP) ||
		    (want_init_on_free() && (bo->ttm != NULL)) ||
		    bo->type == ttm_bo_type_sg ||
		    !dma_resv_trylock(bo->base.resv)) {
			/* The BO is not idle, resurrect it for delayed destroy */
			ttm_bo_flush_all_fences(bo);
			bo->deleted = true;
    ...
			schedule_work(&bo->delayed_delete);
}

Immediately afterwards, xe_oa_release() drops the final drm_device
reference, and drm_managed_release() runs ggtt_fini_early() and frees
tile->mem.ggtt before xe_device_destroy() calls ttm_device_fini() to drain
xe->ttm.wq:

CPU0                                  kworker (xe->ttm.wq)
xe_oa_release()
  xe_oa_destroy_locked()
    xe_oa_free_oa_buffer()
      xe_bo_unpin_map_no_vm(bo)
        ttm_bo_release()
          schedule_work(&bo->delayed_delete)
  drm_dev_put(&xe->drm)
    drm_managed_release()
      ggtt_fini_early()
      kfree(tile->mem.ggtt)
                                      ttm_bo_delayed_delete()
                                        xe_ttm_bo_destroy()
                                          xe_ggtt_remove_bo(tile->mem.ggtt, bo)
                                            <-- use-after-free
      xe_device_destroy()
        ttm_device_fini(&xe->ttm)

When xe_ttm_bo_destroy() runs on the workqueue, it dereferences the
already-freed tile->mem.ggtt (and triggers a warning in drm_mm_takedown()
when ggtt_fini_early() sees the live GGTT node).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110190052.3514226-9-umesh.nerlige.ramappa@intel.com?part=7