Thanks a lot for the thorough review, and for pulling in the pvr folks. First, the important one. You flagged the ops-detach as dangerous, and after your and the bot's pointers I agree it is not viable as-is:
- amdgpu dereferences the helper unconditionally, e.g. amdgpu_cs_p2_dependencies() and amdgpu_ctx_fence_time() do to_drm_sched_fence(fence) and then touch ->scheduled without a NULL check. Once the finished fence detaches its ops on signalling, to_drm_sched_fence() returns NULL for it, so this is a deterministic NULL deref an unprivileged process can reach by submitting and then referencing completed jobs. That is the bot's [Critical], and it checks out. - pvr is worse in the way you described: pvr_queue_fence_is_native() uses the ops pointer as an *identity* test, so detaching ops makes it race between "native" and "foreign" for one and the same fence. So detaching the ops breaks the "identify a drm_sched_fence by its ops" contract that these drivers rely on, and papering over it would mean auditing every to_drm_sched_fence() caller. I don't think that is the right trade for a fix we want to backport. Christian's point that the finished/scheduled .release callbacks are "unproblematic for the problem at hand" matches this: the release callbacks do not need to be removed to fix the timeline-name UAF, so keeping them (and thus the ops attached, and to_drm_sched_fence() working) is fine. Given that, I'd like to fall back to the minimal caching fix and drop the ops/refcount rework entirely: - get_timeline_name() caches the name in drm_sched_fence_init() and returns the cached value, so it never dereferences ->sched. Everything else - both .release callbacks, the shared allocation, the call_rcu() free, to_drm_sched_fence() - stays exactly as today, so there is no amdgpu/pvr regression and nothing new for the backend to reason about. - This also addresses Christian's point that the reference must go from the finished to the scheduled fence, not the other way around: the caching fix keeps the existing finished->scheduled reference untouched and does not invert it, so the finished->scheduled conversions that rely on that keep working. - This makes most of the per-patch comments on v3 (the shared-allocation lifetime, the extra dma_fence_get(), the "last put" wording, moving call_rcu) moot, since that rework goes away. I'll keep the ones that still apply. On the specific points: - get_driver_name(): it returns the literal "drm_sched" and never touches ->sched, so unlike get_timeline_name() it isn't exposed. Only the timeline name needs the fix. - The "already-satisfied dependency / dependency-collapsing" wording and the whole to_drm_sched_fence()-returns-NULL discussion only existed to justify the ops-detach; with caching, to_drm_sched_fence() keeps working as today, so that reasoning (and the confusion around it) goes away entirely. - Caching only the pointer: the earlier objection was that it doesn't help drivers whose name is freed together with the scheduler. The mainline drivers that actually hit this (amdxdna, nouveau, msm VM_BIND) pass a name that lives as long as the scheduler, and panthor/xe (dynamically allocated names) are already fixed per-driver. If you'd rather close the dynamic-name case generically in the core too, I can kstrdup() the name into the fence at init and free it on fence release - one small alloc per fence. I'm happy to go pointer-cache or kstrdup, whichever you and Tvrtko prefer. - Cc: stable: will add "Cc: [email protected] # we don't know since when" and let the stable folks pick the backport depth, as you suggested. - Whitespace/doc reflow: will split into its own patch and keep the fix patch free of unrelated formatting churn. - kmemleak: the caching fix doesn't change any refcounts, but I'll re-run the KUnit suite under kmemleak as well as KASAN before resending. Unless someone would prefer to keep ops-detach and fix the two callers instead, I'll respin as the caching v4 once Tvrtko and the pvr folks have had a chance to look as well. Thanks again, Jonghyuk
