[PATCH AUTOSEL 6.12 01/19] drm/msm: Fix a fence leak in submit error path

[PATCH AUTOSEL 6.12 01/19] drm/msm: Fix a fence leak in submit error path

[Sasha Levin]

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 f775638d239a5..937c1f5d88cbb 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -85,6 +85,15 @@ void __msm_gem_submit_destroy(struct kref *kref)
 			container_of(kref, struct msm_gem_submit, ref);
 	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) {
 		spin_lock(&submit->queue->idr_lock);
 		idr_remove(&submit->queue->fence_idr, submit->fence_id);
-- 
2.39.5

[PATCH AUTOSEL 6.12 02/19] drm/msm: Fix another leak in the submit error path

[Sasha Levin]

From: Rob Clark <robdclark@chromium.org>

[ Upstream commit f681c2aa8676a890eacc84044717ab0fd26e058f ]

put_unused_fd() doesn't free the installed file, if we've already done
fd_install().  So we need to also free the sync_file.

Signed-off-by: Rob Clark <robdclark@chromium.org>
Patchwork: https://patchwork.freedesktop.org/patch/653583/
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

Based on my analysis, **YES**, this commit should be backported to
stable kernel trees.

## Extensive Explanation:

This commit fixes a **resource leak** in the MSM DRM driver's submit
error path, which is a clear bug that affects users and meets the stable
tree criteria.

### The Bug:

Looking at the code changes in `msm_gem_submit.c`:

1. **Before the fix**: When an error occurs after `fd_install()` has
   been called (line 865 in the context), the error path only calls
   `put_unused_fd(out_fence_fd)` at line 895-896.

2. **The problem**: As noted in the commit message, `put_unused_fd()`
   doesn't free the installed file if `fd_install()` has already been
   executed. The `sync_file` structure created by `sync_file_create()`
   (line 861) contains a file reference that needs to be released with
   `fput()`.

3. **The fix**: The patch adds proper cleanup by:
   - Moving `sync_file` declaration to function scope (line 661)
   - Adding proper cleanup in the error path (lines 895-898) that calls
     both `put_unused_fd()` AND `fput(sync_file->file)` when needed

### Why This Is a Good Backport Candidate:

1. **Clear Bug Fix**: This fixes a resource leak that can accumulate
   over time as applications hit error conditions during fence
   submission.

2. **Small and Contained**: The fix is minimal - just 5 lines of actual
   code changes that properly clean up resources.

3. **No Architectural Changes**: This doesn't introduce new features or
   change any APIs - it simply fixes error handling.

4. **Similar to Previous Backported Fixes**: Looking at similar commit
   #3 ("drm/msm: Fix submit error-path leaks") which was marked for
   backporting, this follows the same pattern of fixing resource leaks
   in error paths.

5. **Low Risk**: The change only affects error paths and adds proper
   cleanup that was missing. It cannot break the normal success path.

6. **Documentation Confirms**: The sync_file.c documentation (line
   62-63) explicitly states that sync_files should be released with
   `fput(sync_file->file)`, confirming this is the correct fix.

### Pattern Analysis:

Comparing with the similar commits provided:
- Similar commits #1, #2, and #4 were NOT backported (they were general
  code improvements or non-critical changes)
- Similar commits #3 and #5 WERE backported (they fixed actual resource
  leaks or initialization failures)

This commit clearly falls into the same category as #3 and #5 - fixing
an actual resource leak that can impact system stability over time.

The leak occurs whenever:
1. A sync_file is successfully created
2. fd_install() succeeds
3. But then a subsequent error occurs before the submit completes

Without this fix, each occurrence leaks a file descriptor and associated
kernel memory, which could eventually exhaust resources in systems with
heavy GPU usage that encounter errors.

 drivers/gpu/drm/msm/msm_gem_submit.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 937c1f5d88cbb..4b3a8ee8e278f 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -667,6 +667,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct msm_ringbuffer *ring;
 	struct msm_submit_post_dep *post_deps = NULL;
 	struct drm_syncobj **syncobjs_to_reset = NULL;
+	struct sync_file *sync_file = NULL;
 	int out_fence_fd = -1;
 	unsigned i;
 	int ret;
@@ -877,7 +878,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 	}
 
 	if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
-		struct sync_file *sync_file = sync_file_create(submit->user_fence);
+		sync_file = sync_file_create(submit->user_fence);
 		if (!sync_file) {
 			ret = -ENOMEM;
 		} else {
@@ -911,8 +912,11 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 out_unlock:
 	mutex_unlock(&queue->lock);
 out_post_unlock:
-	if (ret && (out_fence_fd >= 0))
+	if (ret && (out_fence_fd >= 0)) {
 		put_unused_fd(out_fence_fd);
+		if (sync_file)
+			fput(sync_file->file);
+	}
 
 	if (!IS_ERR_OR_NULL(submit)) {
 		msm_gem_submit_put(submit);
-- 
2.39.5