Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing drm_dev_enter/drm_dev_exit protection in OA stream file operations leads to NULL pointer dereferences and UAF on device unbind. - [High] Sync entries and memory are permanently leaked when xe_oa_emit_oa_config() fails in xe_oa_config_locked(). - [High] Concurrent execution of xe_oa_fini and xe_oa_add_config_ioctl can cause radix tree corruption and a memory leak because the ioctl lacks drm_dev_enter() protection. -- --- Patch [1]: [PATCH] drm/xe/oa: Check managed mutex initialization errors --- commit 6dc1dc862e4088df59048042e54f422ac372d344 Author: Linmao Li <[email protected]> drm/xe/oa: Check managed mutex initialization errors drmm_mutex_init() can fail while registering its managed cleanup action. On failure, the reset path destroys the mutex, so continuing OA setup leaves an unusable lock that later paths may acquire. Return the error from per-GT OA initialization and abort device-wide OA initialization if the metrics lock cannot be initialized. Fixes: a9f905ae7b6f ("drm/xe/oa/uapi: Initialize OA units") Fixes: cdf02fe1a94a ("drm/xe/oa/uapi: Add/remove OA config perf ops") > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c > index b3acbcd678b7c..302a2ef4494be 100644 > --- a/drivers/gpu/drm/xe/xe_oa.c > +++ b/drivers/gpu/drm/xe/xe_oa.c > @@ -2718,9 +2718,7 @@ static int xe_oa_init_gt(struct xe_gt *gt) > > __xe_oa_init_oa_units(gt); > > - drmm_mutex_init(>_to_xe(gt)->drm, >->oa.gt_lock); > - > - return 0; > + return drmm_mutex_init(>_to_xe(gt)->drm, >->oa.gt_lock); > } > > static void xe_oa_print_gt_oa_units(struct xe_gt *gt) > @@ -2860,7 +2858,10 @@ int xe_oa_init(struct xe_device *xe) > oa->xe = xe; > oa->oa_formats = oa_formats; > > - drmm_mutex_init(&oa->xe->drm, &oa->metrics_lock); > + ret = drmm_mutex_init(&oa->xe->drm, &oa->metrics_lock); > + if (ret) > + goto exit; > + > idr_init_base(&oa->metrics_idr, 1); > > ret = xe_oa_init_oa_units(oa); While reviewing this patch, I noticed a few pre-existing issues in this file: [ ... ] > static ssize_t xe_oa_read(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > struct xe_oa_stream *stream = file->private_data; [ ... ] > mutex_lock(&stream->stream_lock); > ret = __xe_oa_read(stream, buf, count, &offset); ^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but does xe_oa_read() lack drm_dev_enter/drm_dev_exit protection? File operations like xe_oa_read() and xe_oa_ioctl() operate on an independent file descriptor. When the device unbinds, devm-managed mappings like stream->gt->mmio are released. If a local user calls read() or ioctl() on the open stream file descriptor after the device is hot-unplugged or unbound, can this cause a NULL pointer dereference or a kernel crash when accessing unmapped MMIO memory? [ ... ] > stream->num_syncs = param.num_syncs; > stream->syncs = param.syncs; > > err = xe_oa_emit_oa_config(stream, config); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but if xe_oa_emit_oa_config() fails, does this permanently leak the stream->syncs memory and the dma_fence references? If the user provides valid sync entries but xe_oa_emit_oa_config() fails due to memory pressure or batch buffer submission failures, the error is propagated without cleaning up stream->syncs. Since xe_oa_emit_oa_config() only frees stream->syncs on the success path, and xe_oa_stream_destroy() does not free stream->syncs, is there any path that cleans up these references after a failure? [ ... ] > static void xe_oa_fini(void *arg) > { > struct xe_device *xe = arg; > struct xe_oa *oa = &xe->oa; > > if (!oa->xe) > return; > > idr_for_each(&oa->metrics_idr, destroy_config, oa); > idr_destroy(&oa->metrics_idr); > > oa->xe = NULL; > } > int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file > *file) > { [ ... ] > if (!oa->xe) { > drm_dbg(&xe->drm, "xe oa interface not available for this > system\n"); > return -ENODEV; > } [ ... ] > id = idr_alloc(&oa->metrics_idr, oa_config, 2, 0, GFP_KERNEL); ^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but does concurrent execution of xe_oa_fini() and xe_oa_add_config_ioctl() cause radix tree corruption? xe_oa_add_config_ioctl() checks if (!oa->xe) locklessly and does not use drm_dev_enter() to guarantee the hardware device remains bound. Concurrently, driver unbind invokes xe_oa_fini(), which calls idr_destroy(&oa->metrics_idr) before setting oa->xe = NULL. If the ioctl evaluates !oa->xe before xe_oa_fini() nullifies it, could it proceed to call idr_alloc() on a concurrently destroyed IDR structure? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
