Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] dma-fence: use-after-free on extern_lock in dma_fence_driver_name() - [High] dma-fence: uninitialized locals in dma_fence_describe() when signaled - [High] dma-fence: recursive spinlock deadlock via trace_dma_fence_destroy() - [High] drm/i915: rq->lock ABBA deadlock in i915_request_show() - [Low] dma-fence: stack VLAs in dma_fence and i915 fence name callers - [Low] dma-fence: leftover rcu_read_lock() and outdated kernel-doc in name helpers --
commit 9ca3a4e9e4ca7aa50b6bddada4c64fc3ed8e2861 Author: Philipp Stanner <[email protected]> dma-fence: Support unload for ops->get_XXX_name() This commit replaces the RCU-protected return values of dma_fence_driver_name() and dma_fence_timeline_name() with callers passing destination buffers that are populated under the fence lock. > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index e4350bce9cf9b..d473d4ea601a7 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -555,23 +555,22 @@ void dma_fence_release(struct kref *kref) > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > const struct dma_fence_ops *ops = fence->ops; > + const unsigned short len = 64; > + char timeline[len]; > + char driver[len]; [Severity: Low] In C11/gnu11, a const-qualified variable is not an integer constant expression, so does declaring timeline[len] and driver[len] create stack VLAs that trigger compiler warnings (and build failures with CONFIG_WERROR or CONFIG_DRM_I915_WERROR) under -Wvla-larger-than=1 in scripts/Makefile.warn? The same pattern appears in dma_fence_describe(), intel_gt_watchdog_work(), i915_request_show(), and timer_i915_sw_fence_wake(). Also, in dma_fence_release(), these buffers were moved out of the unlikely error block to the top of the function. > > - rcu_read_lock(); > trace_dma_fence_destroy(fence); [ ... ] > @@ -1017,23 +1015,20 @@ EXPORT_SYMBOL(dma_fence_set_deadline); > */ > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) > { > - const char __rcu *timeline = (const char __rcu *)""; > - const char __rcu *driver = (const char __rcu *)""; > - const char *signaled = ""; > - > - rcu_read_lock(); > + const unsigned short len = 64; > + char timeline[len]; > + char driver[len]; > + char *signaled; > > if (!dma_fence_is_signaled(fence)) { > - timeline = dma_fence_timeline_name(fence); > - driver = dma_fence_driver_name(fence); > + dma_fence_timeline_name(fence, timeline, len); > + dma_fence_driver_name(fence, driver, len); > signaled = "un"; > } > > seq_printf(seq, "%llu:%llu %s %s %ssignalled\n", > fence->context, fence->seqno, timeline, driver, > signaled); > - > - rcu_read_unlock(); > } > EXPORT_SYMBOL(dma_fence_describe); [Severity: High] What happens in dma_fence_describe() when dma_fence_is_signaled(fence) returns true? Previously, timeline, driver, and signaled were initialized to "" at declaration. Now they are only assigned inside the if (!dma_fence_is_signaled(fence)) block. When the fence is already signaled, won't seq_printf() dereference the uninitialized stack pointer signaled and read the uninitialized stack arrays timeline and driver? Also, if the fence signals concurrently right after !dma_fence_is_signaled(fence) evaluates to true, dma_fence_timeline_name() and dma_fence_driver_name() will populate "signaled-timeline" and "detached-driver" while signaled is set to "un". > @@ -1129,20 +1124,22 @@ EXPORT_SYMBOL(dma_fence_init64); > * memory potentially accessed by the &dma_fence.ops functions, is forbidden > * after the fence has been signalled. Drivers are allowed to free that data, > * and some do. > - * > - * To allow safe access drivers are mandated to guarantee a RCU grace period > - * between signalling the fence and freeing said data. > - * > - * As such access to the driver name is only valid inside a RCU locked > section. > - * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block > guarded > - * by the &rcu_read_lock and &rcu_read_unlock pair. > */ > -const char __rcu *dma_fence_driver_name(struct dma_fence *fence) > +void dma_fence_driver_name(struct dma_fence *fence, char *s, unsigned short > len) > { > + unsigned long flags; > + const char *tmp; > + > + dma_fence_lock_irqsave(fence, flags); > + > if (!dma_fence_test_signaled_flag(fence)) > - return (const char __rcu *)fence->ops->get_driver_name(fence); > + tmp = fence->ops->get_driver_name(fence); > else > - return (const char __rcu *)"detached-driver"; > + tmp = "detached-driver"; > + > + strscpy(s, tmp, len); > + > + dma_fence_unlock_irqrestore(fence, flags); > } > EXPORT_SYMBOL(dma_fence_driver_name); > > @@ -1158,19 +1155,21 @@ EXPORT_SYMBOL(dma_fence_driver_name); > * memory potentially accessed by the &dma_fence.ops functions, is forbidden > * after the fence has been signalled. Drivers are allowed to free that data, > * and some do. > - * > - * To allow safe access drivers are mandated to guarantee a RCU grace period > - * between signalling the fence and freeing said data. > - * > - * As such access to the driver name is only valid inside a RCU locked > section. > - * The pointer MUST be both queried and USED ONLY WITHIN a SINGLE block > guarded > - * by the &rcu_read_lock and &rcu_read_unlock pair. > */ > -const char __rcu *dma_fence_timeline_name(struct dma_fence *fence) > +void dma_fence_timeline_name(struct dma_fence *fence, char *s, unsigned > short len) > { > + unsigned long flags; > + const char *tmp; > + > + dma_fence_lock_irqsave(fence, flags); > + > if (!dma_fence_test_signaled_flag(fence)) > - return (const char __rcu *)fence->ops->get_timeline_name(fence); > + tmp = fence->ops->get_timeline_name(fence); > else > - return (const char __rcu *)"signaled-timeline"; > + tmp = "signaled-timeline"; > + > + strscpy(s, tmp, len); > + > + dma_fence_unlock_irqrestore(fence, flags); > } > EXPORT_SYMBOL(dma_fence_timeline_name); [Severity: Critical] Is it safe for dma_fence_driver_name() and dma_fence_timeline_name() to unconditionally acquire dma_fence_lock_irqsave(fence, flags) before checking if the fence is already signaled? As noted in the kernel-doc comment above these functions, external memory not embedded in struct dma_fence (including fence->extern_lock) must not be accessed after the fence has signaled because drivers are allowed to free the structure containing the lock once the fence is signaled: drivers/gpu/drm/panthor/panthor_sched.c:group_free_queue() dma_fence_put(queue->fence_ctx.last_fence); kfree(queue); /* frees queue->fence_ctx.lock */ drivers/gpu/drm/msm/msm_fence.c:msm_fence_context_free() kfree(fctx); /* frees fctx->spinlock */ drivers/gpu/drm/xe/xe_hw_fence.c:xe_hw_fence_irq_finish() synchronize_rcu(); /* before freeing irq containing irq->lock */ If a signaled fence is still held (for example in a sync_file) after the driver frees the structure containing fence->extern_lock, and userspace calls SYNC_IOC_FILE_INFO -> sync_fill_fence_info() -> dma_fence_timeline_name(), won't dma_fence_lock_irqsave() dereference and modify the freed spinlock? [Severity: High] Can acquiring dma_fence_lock_irqsave() inside dma_fence_driver_name() and dma_fence_timeline_name() also cause a recursive spinlock deadlock when trace_dma_fence_destroy is enabled? In dma_fence_release(), trace_dma_fence_destroy(fence) is invoked when the refcount drops to zero, which calls dma_fence_driver_name() and dma_fence_timeline_name() in TP_fast_assign(): dma_fence_put(fence) -> dma_fence_release() -> trace_dma_fence_destroy(fence) -> dma_fence_driver_name() -> dma_fence_lock_irqsave(fence, flags) However, several paths in the kernel call dma_fence_put() while already holding the fence's spinlock: 1. dma_fence_signal_timestamp_locked() holds dma_fence_spinlock(fence) while invoking callbacks such as dma_fence_chain_cb(), which calls dma_fence_put(f). 2. Drivers with shared external fence locks call dma_fence_put() while holding that lock, such as virtio_gpu_fence_event_process() (holding drv->lock), nouveau_fence_signal() (holding fctx->lock), and hw_fence_irq_run_cb() (holding irq->lock). If the last reference is dropped on any of these paths while the dma_fence_destroy tracepoint is active, won't dma_fence_lock_irqsave() deadlock trying to acquire the already-held lock on the same CPU? [Severity: Low] Also, should the kernel-doc comments for dma_fence_driver_name() and dma_fence_timeline_name() in drivers/dma-buf/dma-fence.c be updated to document the new @s and @len parameters and void return, as well as the get_driver_name and get_timeline_name comments in struct dma_fence_ops in include/linux/dma-fence.h (which still state that the returned string is RCU protected) and the leftover rcu_read_lock() wrappers around trace_dma_fence_wait_start() and trace_dma_fence_wait_end() in dma_fence_wait_timeout()? > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 2166bbdf7e4a4..0f08713b3b5e2 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c [ ... ] > @@ -268,19 +269,11 @@ static long sync_file_ioctl_merge(struct sync_file > *sync_file, > static int sync_fill_fence_info(struct dma_fence *fence, > struct sync_fence_info *info) > { > - const char __rcu *timeline; > - const char __rcu *driver; > + dma_fence_timeline_name(fence, info->obj_name, sizeof(info->obj_name)); > + dma_fence_driver_name(fence, info->driver_name, > sizeof(info->driver_name)); > > rcu_read_lock(); > > - driver = dma_fence_driver_name(fence); > - timeline = dma_fence_timeline_name(fence); > - > - strscpy(info->obj_name, rcu_dereference(timeline), > - sizeof(info->obj_name)); > - strscpy(info->driver_name, rcu_dereference(driver), > - sizeof(info->driver_name)); > - > info->status = dma_fence_get_status(fence); > info->timestamp_ns = > dma_fence_is_signaled(fence) ? [Severity: Low] In sync_fill_fence_info(), now that dma_fence_timeline_name() and dma_fence_driver_name() have been moved outside the RCU critical section, is the remaining rcu_read_lock() / rcu_read_unlock() pair around dma_fence_get_status() and dma_fence_is_signaled() still needed? > diff --git a/drivers/gpu/drm/i915/i915_request.c > b/drivers/gpu/drm/i915/i915_request.c > index f66f8efc70629..ce9aff50c0885 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -2183,7 +2183,8 @@ void i915_request_show(struct drm_printer *m, > const char *prefix, > int indent) > { > - const char __rcu *timeline; > + const unsigned short len = 64; > + char timeline[len]; > char buf[80] = ""; > int x = 0; > > @@ -2219,8 +2220,7 @@ void i915_request_show(struct drm_printer *m, > > x = print_sched_attr(&rq->sched.attr, buf, x, sizeof(buf)); > > - rcu_read_lock(); > - timeline = dma_fence_timeline_name((struct dma_fence *)&rq->fence); > + dma_fence_timeline_name((struct dma_fence *)&rq->fence, timeline, len); > drm_printf(m, "%s%.*s%c %llx:%lld%s%s %s @ %dms: %s\n", > prefix, indent, " ", > queue_status(rq), > @@ -2229,8 +2229,7 @@ void i915_request_show(struct drm_printer *m, > fence_status(rq), > buf, > jiffies_to_msecs(jiffies - rq->emitted_jiffies), > - rcu_dereference(timeline)); > - rcu_read_unlock(); > + timeline); > } > > static bool engine_match_ring(struct intel_engine_cs *engine, struct > i915_request *rq) [Severity: High] Does calling dma_fence_timeline_name() in i915_request_show() introduce a lock ordering inversion between rq->lock and sched_engine->lock (or ce->guc_state.lock)? Each i915_request initializes rq->fence with &rq->lock. During request signaling in i915_request_retire() and signal_irq_work(), rq->lock is held while invoking fence callbacks: i915_request_retire() / signal_irq_work() [holds rq->lock] -> dma_fence_signal_locked(&rq->fence) -> dma_i915_sw_fence_wake() -> submit_notify() -> execlists_submit_request() [acquires sched_engine->lock] -> guc_submit_request() [acquires ce->guc_state.lock] However, i915_request_show() is called while holding sched_engine->lock or ce->guc_state.lock in paths such as: intel_execlists_show_requests() / intel_execlists_dump_active_requests() [holds sched_engine->lock] -> i915_request_show() -> dma_fence_timeline_name() [acquires rq->lock] intel_guc_dump_active_requests() [holds ce->guc_state.lock] -> intel_engine_dump_active_requests() -> engine_dump_request() -> i915_request_show() -> dma_fence_timeline_name() [acquires rq->lock] Can this cause an AB-BA deadlock when engine state is dumped concurrently with request retirement or breadcrumb signaling? [ ... ] > diff --git a/include/trace/events/dma_fence.h > b/include/trace/events/dma_fence.h > index 5b10a9e06fb4e..f3b929b98cf42 100644 > --- a/include/trace/events/dma_fence.h > +++ b/include/trace/events/dma_fence.h > @@ -9,6 +9,8 @@ > > struct dma_fence; > > +#define FENCE_NAME_LEN 64 > + > DECLARE_EVENT_CLASS(dma_fence, > > TP_PROTO(struct dma_fence *fence), > @@ -16,21 +18,21 @@ DECLARE_EVENT_CLASS(dma_fence, > TP_ARGS(fence), > > TP_STRUCT__entry( > - __string(driver, dma_fence_driver_name(fence)) > - __string(timeline, dma_fence_timeline_name(fence)) > + __array(char, driver, FENCE_NAME_LEN) > + __array(char, timeline, FENCE_NAME_LEN) > __field(unsigned int, context) > __field(unsigned int, seqno) > ), > > TP_fast_assign( > - __assign_str(driver); > - __assign_str(timeline); > + dma_fence_driver_name(fence, __entry->driver, FENCE_NAME_LEN); > + dma_fence_timeline_name(fence, __entry->timeline, > FENCE_NAME_LEN); > __entry->context = fence->context; > __entry->seqno = fence->seqno; > ), [Severity: High] As noted above in dma_fence_driver_name(), calling dma_fence_driver_name() and dma_fence_timeline_name() inside TP_fast_assign() acquires the fence spinlock during trace_dma_fence_destroy(fence), which can deadlock when dma_fence_put() is called under the fence lock. > > TP_printk("driver=%s timeline=%s context=%u seqno=%u", > - __get_str(driver), __get_str(timeline), __entry->context, > + __entry->driver, __entry->timeline, __entry->context, > __entry->seqno) > ); > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
