Re: [PATCH] drm/xe/oa: Quiesce OA streams on device remove and PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/xe/oa: wrong mapping unmapped and fault race in xe_oa_quiesce()
- [High] drm/xe/oa: xe_oa_quiesce_oa_unit() skips xe_oa_stream_hw_teardown()
- [High] drm/xe/oa: xe_oa_quiesce() races with xe_oa_stream_open_ioctl()
- [High] drm/xe/oa: mmap_lock and stream_lock inversion in xe_oa_mmap()
- [Medium] drm/xe/oa: xe_oa_mmap() accesses stream->oa->xe before checking stream->dead
--

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

drm/xe/oa: Quiesce OA streams on device remove and PCI reset

This commit adds xe_oa_quiesce() to disable active OA streams, stop their
poll hrtimers, unmap OA buffers, and release device-bound resources during
device removal and PCI slot resets. Once marked dead, operations on the OA
stream fd return -ENODEV or EPOLLERR | EPOLLHUP, and xe_oa_release() frees
the remaining stream struct when the fd is closed.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index dc3f46a044c83..62693c7b843ce 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1106,6 +1106,8 @@ int xe_device_probe(struct xe_device *xe)
>  
>  void xe_device_remove(struct xe_device *xe)
>  {
> +	xe_oa_quiesce(xe);
> +
>  	xe_display_unregister(xe);
>  
>  	drm_dev_unplug(&xe->drm);

[Severity: High]
Can xe_oa_quiesce() race with a concurrent DRM_XE_OBSERVATION_OP_STREAM_OPEN
ioctl here?

In xe_device_remove(), xe_oa_quiesce() runs before xe_display_unregister()
and drm_dev_unplug(). In xe_oa_quiesce_oa_unit(), u->exclusive_stream is
cleared to NULL and gt->oa.gt_lock is released without recording that OA
has been quiesced, while xe->oa.xe remains non-NULL until devm cleanup runs
xe_oa_fini().

In drivers/gpu/drm/xe/xe_oa.c, xe_oa_stream_open_ioctl_locked() only checks
param->oa_unit->exclusive_stream under gt->oa.gt_lock:

	if (param->oa_unit->exclusive_stream) {
		drm_dbg(&oa->xe->drm, "OA unit already in use\n");
		ret = -EBUSY;
		goto exit;
	}

If userspace invokes xe_oa_stream_open_ioctl() while xe_display_unregister()
is still running (or after passing drm_ioctl()'s unplugged check while
waiting on gt->oa.gt_lock), won't it see exclusive_stream == NULL and create
and enable a new stream with stream->dead == false after xe_oa_quiesce() has
already finished?

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 838802482adaf..db9f7d66e5fd0 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c

[ ... ]

> @@ -1711,16 +1752,14 @@ static int xe_oa_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma)
> +static int xe_oa_mmap_locked(struct xe_oa_stream *stream, struct file *file,
> +			     struct vm_area_struct *vma)
>  {
> -	struct xe_oa_stream *stream = file->private_data;
> -	int ret = xe_observation_paranoid_check();
>  	struct xe_bo *bo = stream->oa_buffer.bo;
> +	int ret;
>  
> -	if (ret) {
> -		drm_dbg(&stream->oa->xe->drm, "Insufficient privilege to map OA buffer\n");
> -		return ret;
> -	}
> +	if (stream->dead)
> +		return -ENODEV;
>  
>  	/* Can mmap the entire OA buffer or nothing (no partial OA buffer mmaps) */
>  	if (vma->vm_end - vma->vm_start != xe_bo_size(bo)) {
> @@ -1739,7 +1778,29 @@ static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma)
>  	vm_flags_mod(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_DONTCOPY,
>  		     VM_MAYWRITE | VM_MAYEXEC);
>  
> -	return drm_gem_mmap_obj(&bo->ttm.base, xe_bo_size(bo), vma);
> +	ret = drm_gem_mmap_obj(&bo->ttm.base, xe_bo_size(bo), vma);
> +	if (!ret)
> +		stream->mapping = file->f_mapping;
> +
> +	return ret;
> +}
> +
> +static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct xe_oa_stream *stream = file->private_data;
> +	int ret = xe_observation_paranoid_check();
> +
> +	if (ret) {
> +		drm_dbg(&stream->oa->xe->drm, "Insufficient privilege to map OA buffer\n");
> +		return ret;
> +	}

[Severity: Medium]
What happens if userspace calls mmap() on a dead OA stream fd after
xe_oa_fini() has already set stream->oa->xe = NULL, and
xe_observation_paranoid_check() fails?

Because xe_oa_mmap() runs xe_observation_paranoid_check() before acquiring
stream->stream_lock and checking stream->dead in xe_oa_mmap_locked(), the
failure path evaluates &stream->oa->xe->drm when stream->oa->xe is NULL
(triggering a UBSAN null-pointer check and passing a NULL pointer to
drm_dbg()) and returns -EACCES instead of -ENODEV.

> +
> +	/* Serialize against xe_oa_quiesce() freeing the OA buffer */
> +	mutex_lock(&stream->stream_lock);
> +	ret = xe_oa_mmap_locked(stream, file, vma);
> +	mutex_unlock(&stream->stream_lock);
> +
> +	return ret;
>  }

[Severity: High]
Does taking stream->stream_lock inside xe_oa_mmap() introduce a lock
inversion with mm->mmap_lock?

During mmap(), the MM layer calls xe_oa_mmap() with mm->mmap_lock held in
write mode, establishing:

  mm->mmap_lock -> stream->stream_lock

However, xe_oa_read() and xe_oa_ioctl() hold stream->stream_lock while
calling into user copy helpers:

  xe_oa_read()
    mutex_lock(&stream->stream_lock)
    __xe_oa_read()
      xe_oa_append_reports()
        xe_oa_append_report()
          xe_oa_copy_to_user()
            copy_to_user()

and:

  xe_oa_ioctl()
    mutex_lock(&stream->stream_lock)
    xe_oa_ioctl_locked()
      xe_oa_config_locked() / xe_oa_status_locked() / xe_oa_info_locked()
        copy_from_user() / copy_to_user()

When copy_to_user() or copy_from_user() faults on userspace memory, the page
fault handler takes mm->mmap_lock, establishing:

  stream->stream_lock -> mm->mmap_lock

Won't concurrent mmap() and read() or ioctl() calls on the OA stream fd
deadlock?

> @@ -2549,6 +2610,76 @@ static void xe_oa_unregister(void *arg)
>  	oa->metrics_kobj = NULL;
>  }
>  
> +static void xe_oa_quiesce_oa_unit(struct xe_oa_unit *u)
> +{
> +	struct xe_oa_stream *stream = u->exclusive_stream;
> +	unsigned long flags;
> +
> +	if (!stream)
> +		return;
> +
> +	mutex_lock(&stream->stream_lock);
> +	if (stream->enabled)
> +		xe_oa_disable(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);
> +
> +	if (stream->mapping)
> +		unmap_mapping_range(stream->mapping, 0,
> +				    xe_bo_size(stream->oa_buffer.bo), 1);

[Severity: Critical]
Can userspace retain live PTEs to freed OA buffer pages after
xe_oa_quiesce_oa_unit() runs, and does unmap_mapping_range() target the
wrong address_space here?

First, the OA stream fd is created with anon_inode_getfd() in
xe_oa_stream_open_ioctl_locked(), so file->f_mapping is the kernel-wide
singleton anon_inode_inode->i_mapping shared by all anon_inode_getfd() users
rather than xe->drm.anon_inode->i_mapping. Calling unmap_mapping_range() on
stream->mapping zaps PTEs across unrelated anon_inode mappings system-wide.
Also, if an in-flight mmap() has just dropped stream->stream_lock in
xe_oa_mmap() but __mmap_new_vma() has not yet called vma_link_file() to
insert the VMA into file->f_mapping->i_mmap, unmap_mapping_range() will miss
that VMA.

Second, drm_gem_mmap_obj() takes a GEM object reference in
vma->vm_private_data and sets vma->vm_ops = &xe_gem_vm_ops (.fault =
xe_bo_cpu_fault). When xe_oa_stream_free_resources() calls
xe_bo_unpin_map_no_vm(stream->oa_buffer.bo), it unpins the BO and drops one
reference, but the VMA still holds its GEM reference.

Because xe_oa_quiesce() runs in xe_device_remove() before drm_dev_unplug():

  xe_device_remove()
    xe_oa_quiesce(xe)
    xe_display_unregister(xe)
    drm_dev_unplug(&xe->drm)
    xe_bo_pci_dev_remove_all(xe)

a userspace page fault on the VMA before drm_dev_unplug() enters
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);

On normal device unbind (where the device is not wedged and not yet
unplugged), xe_bo_cpu_fault() succeeds and re-populates PTEs to
stream->oa_buffer.bo's pages.

When drm_dev_unplug() then runs, it unmaps dev->anon_inode->i_mapping (which
does not cover the OA VMA on anon_inode_inode->i_mapping), and
xe_bo_pci_dev_remove_all() evicts the now-unpinned BO via
ttm_bo_pipeline_gutting(), freeing its backing pages back to the page
allocator while userspace still has live PTEs mapped to them.

> +
> +	xe_oa_stream_free_resources(stream);
> +	if (stream->exec_q) {
> +		xe_exec_queue_put(stream->exec_q);
> +		stream->exec_q = NULL;
> +	}
> +	drm_syncobj_put(stream->ufence_syncobj);
> +
> +	WRITE_ONCE(u->exclusive_stream, NULL);
> +	mutex_unlock(&stream->stream_lock);
> +}

[Severity: High]
Does xe_oa_quiesce_oa_unit() leak stream->fw_ref and skip
xe_oa_stream_hw_teardown()?

When the stream was initialized in xe_oa_stream_init(), it acquired
stream->fw_ref via xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL),
enabled the metric set via xe_oa_enable_metric_set(), and (if stream->sample
was set) whitelisted OA registers via xe_reg_whitelist_oa_regs(). If
stream->no_preempt was set, xe_oa_enable_locked() also disabled preemption
and timeslicing on stream->exec_q.

In this patch, xe_oa_stream_destroy() is split into
xe_oa_stream_hw_teardown() and xe_oa_stream_free_resources():

static void xe_oa_stream_hw_teardown(struct xe_oa_stream *stream)
{
	if (stream->sample)
		xe_reg_dewhitelist_oa_regs(stream->gt);

	xe_oa_disable_metric_set(stream);
	xe_force_wake_put(gt_to_fw(stream->hwe->gt), stream->fw_ref);
}

However, xe_oa_quiesce_oa_unit() only calls xe_oa_disable() and
xe_oa_stream_free_resources(), and when the fd is later closed,
xe_oa_destroy_locked() returns early because stream->dead is true:

	if (stream->dead) {
		mutex_destroy(&stream->stream_lock);
		kfree(stream);
		return;
	}

As a result, xe_oa_stream_hw_teardown() is never called for a quiesced
stream, leaking stream->fw_ref across all forcewake domains on the GT (while
xe_oa_stream_free_resources() drops the runtime PM reference) and leaving
gt->oa.whitelist_count, the metric set registers, and exec_q preemption
settings un-restored on unbind. Conversely, during xe_pci_error_slot_reset()
after a slot reset, xe_oa_quiesce_oa_unit() still calls xe_oa_disable()
unconditionally, performing MMIO writes and waits against reset hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026232611.1587501-2-umesh.nerlige.ramappa@intel.com?part=1