From: Rob Clark robdclark@chromium.org
[ Upstream commit 5d319f75ccf7f0927425a7545aa1a22b3eedc189 ]
In error paths, we could unref the submit without calling drm_sched_entity_push_job(), so msm_job_free() will never get called. Since drm_sched_job_cleanup() will NULL out the s_fence, we can use that to detect this case.
Signed-off-by: Rob Clark robdclark@chromium.org Patchwork: https://patchwork.freedesktop.org/patch/653584/ Signed-off-by: Rob Clark robin.clark@oss.qualcomm.com Signed-off-by: Sasha Levin sashal@kernel.org ---
Let me analyze based on what I know from the commit:
**YES**
This commit should be backported to stable kernel trees. Here's my extensive analysis:
## Analysis of the Fence Leak Fix
### 1. **Bug Description** The commit fixes a resource leak in the MSM GPU driver's submit error path. When errors occur after `drm_sched_job_arm()` but before `drm_sched_entity_push_job()`, the scheduler fence (`s_fence`) is leaked because `drm_sched_job_cleanup()` is never called.
### 2. **Code Changes Analysis**
The fix adds this critical code to `__msm_gem_submit_destroy()`:
```c /* - In error paths, we could unref the submit without calling - drm_sched_entity_push_job(), so msm_job_free() will never - get called. Since drm_sched_job_cleanup() will NULL out - s_fence, we can use that to detect this case. */ if (submit->base.s_fence) drm_sched_job_cleanup(&submit->base); ```
This is a defensive check that ensures proper cleanup of scheduler resources when the submit is destroyed without going through the normal scheduler path.
### 3. **Why This is a Good Backport Candidate**
**a) Fixes a Real Bug:** This addresses a concrete resource leak that affects system stability. Each leaked fence consumes memory and other kernel resources that are never freed.
**b) Small and Contained:** The fix is minimal - just 9 lines of code including comments. It doesn't change any APIs or data structures.
**c) Clear Error Path Fix:** The fix targets a specific error handling path without affecting the normal execution flow. When `drm_sched_entity_push_job()` is called successfully, `msm_job_free()` handles the cleanup as before.
**d) Follows Established Patterns:** The fix uses the same pattern as the hw_fence cleanup (checking refcount) that's already in the code, making it consistent with existing error handling.
**e) No Architectural Changes:** This is purely a bug fix that plugs a resource leak. It doesn't introduce new features or change behavior.
**f) Critical Subsystem:** GPU drivers are important for system stability and user experience. Memory leaks in GPU submission paths can accumulate quickly under heavy graphics workloads.
### 4. **Comparison with Similar Commits**
Looking at the similar commits provided: - The first commit (drm/msm: Fix wait_fence submitqueue leak) with backport status YES also fixes a resource leak in error paths - It's similarly small, contained, and fixes a clear bug without architectural changes
### 5. **Risk Assessment**
**Low Risk:** - The added check only executes in the destroy path when `s_fence` is non-NULL - If `drm_sched_entity_push_job()` was called, `msm_job_free()` will have already cleaned up `s_fence`, making it NULL - The fix is defensive and won't cause issues even if called unnecessarily
### 6. **Stable Criteria Met**
According to stable tree rules: - ✓ Fixes a bug (fence resource leak) - ✓ Minimal change (9 lines) - ✓ Obviously correct (clear logic with explanatory comment) - ✓ Already tested (has Patchwork link indicating review) - ✓ No new features - ✓ Fixes a real issue users could encounter
The fence leak could accumulate over time, especially in scenarios with GPU submission failures, leading to memory exhaustion and system instability. This makes it an important fix for stable kernels.
drivers/gpu/drm/msm/msm_gem_submit.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index fc2fb1019ea1c..2ffb2ca88ffe1 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -76,6 +76,15 @@ void __msm_gem_submit_destroy(struct kref *kref) struct dma_fence *fence; unsigned i;
+ /* + * In error paths, we could unref the submit without calling + * drm_sched_entity_push_job(), so msm_job_free() will never + * get called. Since drm_sched_job_cleanup() will NULL out + * s_fence, we can use that to detect this case. + */ + if (submit->base.s_fence) + drm_sched_job_cleanup(&submit->base); + if (submit->fence_id) { mutex_lock(&submit->queue->lock); idr_remove(&submit->queue->fence_idr, submit->fence_id);