[PATCH AUTOSEL 7.0-6.18] drm/amdgpu: fix some more bug in amdgpu_gem_va_ioctl

[Sasha Levin <sashal@kernel.org>]

From: Christian König <christian.koenig@amd.com>

[ Upstream commit 68bd4f6b8310f309eb63b41e15088690c9cec0a9 ]

Some illegal combination of input flags were not checked and we need to
take the PDEs into account when returning the fence as well.

Signed-off-by: Christian König <christian.koenig@amd.com>
Acked-by: Sunil Khatri <sunil.khatri@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have all the information needed. Here is my complete analysis.

---

## PHASE 1: COMMIT MESSAGE FORENSICS

### Step 1.1: Subject Line
- **Subsystem**: drm/amdgpu
- **Action verb**: "fix" (explicit bug fix)
- **Summary**: Fix multiple bugs in the `amdgpu_gem_va_ioctl` function
  related to input flag validation and fence handling for PDE updates.

Record: [drm/amdgpu] [fix] [Multiple bugs in gem VA ioctl: illegal flag
combinations and missing PDE fence]

### Step 1.2: Tags
- **Signed-off-by**: Christian König (author, AMD DRM subsystem co-
  maintainer)
- **Acked-by**: Sunil Khatri (AMD developer)
- **Signed-off-by**: Alex Deucher (AMD DRM maintainer, committer)
- No Fixes: tag, no Reported-by, no Link: tag

Record: Author is Christian König, one of the primary amdgpu/drm
maintainers. Acked by AMD colleague, committed by the AMD DRM
maintainer.

### Step 1.3: Commit Body
The body says: "Some illegal combination of input flags were not checked
and we need to take the PDEs into account when returning the fence as
well."

Two distinct bugs identified:
1. Missing input validation for contradictory flag combinations
2. Missing PDE (Page Directory Entry) fence in the returned fence to
   userspace

Record: [Bug 1: invalid flag combinations not rejected] [Bug 2: PDE
updates missing from returned fence, could cause premature GPU memory
access] [No version info given] [Root cause: incomplete validation and
incomplete fence merging]

### Step 1.4: Hidden Bug Fix Detection
This is an explicit "fix" commit, not disguised as cleanup.

Record: This is explicitly labeled as a bug fix. No hidden intent.

---

## PHASE 2: DIFF ANALYSIS

### Step 2.1: Inventory
- **File**: `drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c` (1 file)
- **Added**: `#include <linux/dma-fence-unwrap.h>` (1 line)
- **Functions modified**: `amdgpu_gem_va_update_vm()`,
  `amdgpu_gem_va_ioctl()`
- **Scope**: ~35 lines removed, ~30 lines added in
  `amdgpu_gem_va_update_vm`; ~10 lines changed in `amdgpu_gem_va_ioctl`

Record: [1 file, ~45 lines changed, 2 functions modified] [Single-file
contained fix]

### Step 2.2: Code Flow Changes

**Hunk 1 - `amdgpu_gem_va_update_vm` - VM-not-ready path**:
- Before: `fence = dma_fence_get(vm->last_update)` then if not ready,
  return that fence
- After: If not ready, return `dma_fence_get_stub()` immediately
- Effect: Cleaner early return; stub fence is sufficient when VM isn't
  ready

**Hunk 2 - `amdgpu_vm_clear_freed` argument**:
- Before: `amdgpu_vm_clear_freed(adev, vm, &fence)` (local variable)
- After: `amdgpu_vm_clear_freed(adev, vm, &vm->last_update)` (VM state
  directly)
- Effect: `vm->last_update` is kept current after clearing freed
  mappings, so subsequent `amdgpu_vm_update_pdes` properly syncs

**Hunk 3 - Fence return logic**:
- Before: Switch/case returning either `vm->last_update` or
  `bo_va->last_pt_update` (but NOT both)
- After: For non-always-valid MAP/REPLACE, merges both `vm->last_update`
  and `bo_va->last_pt_update` using `dma_fence_unwrap_merge()`; includes
  OOM fallback; for other cases returns `vm->last_update`
- Effect: Returned fence now accounts for both PTE and PDE updates

**Hunk 4 - Error path**:
- Before: Falls through from normal path to error label, always returns
  local fence
- After: Normal path returns fence via explicit `return`; error path
  returns `dma_fence_get(vm->last_update)`
- Effect: Cleaner separation of normal and error paths

**Hunk 5 - `amdgpu_gem_va_ioctl` - flag validation**:
- Added check: `AMDGPU_VM_DELAY_UPDATE && vm_timeline_syncobj_out`
  returns -EINVAL
- Effect: Rejects contradictory flags (delay + immediate fence request)

**Hunk 6 - `amdgpu_gem_va_ioctl` - update condition**:
- Before: `!adev->debug_vm`
- After: `(!adev->debug_vm || timeline_syncobj)`
- Effect: When timeline syncobj is requested, update happens even in
  debug mode

Record: [6 distinct hunks, all fixing correctness issues]

### Step 2.3: Bug Mechanism
This is a **logic/correctness fix** with two aspects:
1. **Missing fence merge**: `amdgpu_vm_update_pdes()` stores its fence
   into `vm->last_update` (verified at `amdgpu_vm.c:1006`). For non-
   always-valid BOs on MAP/REPLACE, the old code returned only
   `bo_va->last_pt_update`, missing the PDE fence. Userspace could start
   using the mapping before PDE updates complete.
2. **Input validation gap**: DELAY_UPDATE + syncobj_out is contradictory
   and wasn't rejected.

Record: [Logic/correctness fix] [Missing PDE fence could cause premature
GPU memory access; missing input validation for contradictory flags]

### Step 2.4: Fix Quality
- The fix uses `dma_fence_unwrap_merge()` which is the correct API for
  merging fences
- OOM fallback with `dma_fence_wait` + `dma_fence_get_stub()` is
  reasonable
- The flag validation check is trivially correct
- The debug_vm condition change is obviously correct
- Low regression risk: the fence merge is strictly more conservative
  (waits for more work)

Record: [Fix is well-designed with proper fallback] [Low regression risk
- waits for MORE work, not less]

---

## PHASE 3: GIT HISTORY INVESTIGATION

### Step 3.1: Blame
The code being modified was primarily introduced by:
- `efdc66fe12b07` (2026-01-09) - "Refactor amdgpu_gem_va_ioctl v7"
- `bd8150a1b3370` (2025-12-11) - "Refactor amdgpu_gem_va_ioctl v4"

Both are in v7.0-rc1/v7.0. The bugs were introduced in the v7.0 cycle.

Record: [Buggy code from efdc66fe12b07 and bd8150a1b3370, both v7.0
cycle]

### Step 3.2: Fixes tag
No Fixes: tag present. The bugs were introduced by `bd8150a1b3370` and
partially addressed by `efdc66fe12b07`, but this commit fixes remaining
issues in `efdc66fe12b07`.

Record: [No explicit Fixes: tag; fixes bugs remaining from
efdc66fe12b07]

### Step 3.3: File History
44 commits changed this file since v6.12. The specific area
(va_update_vm, va_ioctl) has been actively modified in the v7.0 cycle
with bd8150a1b3370 and efdc66fe12b07.

Record: [Active development area; this is a follow-up fix for recent
refactoring]

### Step 3.4: Author
Christian König is one of the primary amdgpu/drm TTM maintainers. He
authored many critical fixes in this subsystem. He also
suggested/reviewed the v7 refactor that this commit fixes further. His
fixes carry high weight.

Record: [Author is subsystem co-maintainer with deep expertise]

### Step 3.5: Dependencies
- Depends on `efdc66fe12b07` (present in tree as v7.0)
- Requires `dma_fence_unwrap_merge` (present in tree via
  `include/linux/dma-fence-unwrap.h`)
- Requires `amdgpu_vm_is_bo_always_valid` (present in tree)
- The patch context matches the current tree state exactly - clean apply
  expected

Record: [All dependencies present in 7.0 tree; should apply cleanly]

---

## PHASE 4: MAILING LIST RESEARCH

### Step 4.1: Original Discussion
lore.kernel.org blocked automated access. However, I found via b4 dig
that the predecessor commit (`efdc66fe12b07`) was submitted as v7 of the
refactoring series, meaning the fence handling went through 7 iterations
of review. The current fix is by Christian König himself who guided the
entire refactoring.

Record: [Unable to access lore directly; predecessor went through 7
review iterations]

### Step 4.2: Reviewers
- Christian König (author) - AMD DRM co-maintainer
- Sunil Khatri (acker) - AMD developer
- Alex Deucher (committer) - AMD DRM maintainer

Record: [Reviewed by top AMD DRM maintainers]

### Step 4.3-4.5: Bug Reports / Related Patches / Stable Discussion
The predecessor commits (`bd8150a1b3370`) had a documented crash
signature (refcount underflow, use-after-free, kernel panic). While
`efdc66fe12b07` fixed the worst of it, this commit addresses remaining
correctness issues.

Record: [Predecessor had kernel panic crash signature; this fixes
remaining issues]

---

## PHASE 5: CODE SEMANTIC ANALYSIS

### Step 5.1: Key Functions
- `amdgpu_gem_va_update_vm()` - updates VM page tables after VA
  operation
- `amdgpu_gem_va_ioctl()` - userspace-facing ioctl handler

### Step 5.2: Callers
`amdgpu_gem_va_ioctl` is the DRM ioctl handler called via
`DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, ...)` at `amdgpu_drv.c:3082`. It's
callable by any process with DRM_AUTH|DRM_RENDER_ALLOW. This is a hot
path for all AMD GPU userspace (Mesa, ROCm, etc.).

`amdgpu_gem_va_update_vm` is called only from `amdgpu_gem_va_ioctl`.

Record: [Directly callable from userspace; affects all AMD GPU users]

### Step 5.4: Reachability
The buggy code path is reachable from any unprivileged process that
opens a DRM render node and performs VM address space management
(standard GPU operation).

Record: [Reachable from unprivileged userspace; common GPU operation
path]

---

## PHASE 6: STABLE TREE ANALYSIS

### Step 6.1: Code in Stable Trees
`bd8150a1b3370` first appears in v7.0-rc1. It does NOT exist in v6.14 or
earlier stable trees. The timeline syncobj support
(`vm_timeline_syncobj_out`, `AMDGPU_VM_DELAY_UPDATE` in this context) is
v7.0-only functionality. The fix is relevant ONLY for the 7.0.y stable
tree.

Record: [Buggy code only in 7.0.y; not in 6.12.y or earlier]

### Step 6.2: Backport Complications
The diff context matches the current tree state perfectly. Clean apply
expected.

Record: [Clean apply expected for 7.0.y]

### Step 6.3: Related Fixes Already in Stable
`efdc66fe12b07` (the v7 refactor fix) is already in the 7.0 tree, but
this commit fixes remaining issues.

Record: [Predecessor fix present; this addresses additional bugs]

---

## PHASE 7: SUBSYSTEM CONTEXT

### Step 7.1: Subsystem
- **Subsystem**: drivers/gpu/drm/amd/amdgpu (GPU driver)
- **Criticality**: IMPORTANT - AMD GPU driver is used by millions of
  users (desktop Linux, server compute, embedded)

Record: [GPU driver, IMPORTANT criticality, affects all AMD GPU users]

### Step 7.2: Subsystem Activity
Very actively developed - 44 changes to this file since v6.12. The VA
ioctl area specifically is being stabilized after recent refactoring.

Record: [Very active; area being stabilized after v7.0 refactoring]

---

## PHASE 8: IMPACT AND RISK ASSESSMENT

### Step 8.1: Who Is Affected
All users of AMD GPUs on kernel 7.0.y. This includes desktop,
workstation, and compute users.

Record: [All AMD GPU users on 7.0.y]

### Step 8.2: Trigger Conditions
The missing PDE fence bug triggers during MAP/REPLACE operations on non-
always-valid BOs, which is the normal case for application-managed GPU
memory. This is the common path for all GPU applications. The invalid
flag combination requires specific userspace to pass contradictory
flags.

Record: [PDE fence bug: common GPU operation; flag validation: requires
specific bad input]

### Step 8.3: Failure Mode Severity
- **Missing PDE fence**: Userspace sees mapping as ready before PDE
  updates complete. Could lead to GPU page faults, incorrect GPU memory
  access, or visual corruption. Severity: **HIGH**
- **Invalid flag combination**: Unexpected behavior with contradictory
  flags. Severity: **MEDIUM**
- **debug_vm condition**: Timeline syncobj never signals in debug mode.
  Severity: **MEDIUM** (only affects debug configs)

Record: [HIGH severity for PDE fence bug; MEDIUM for others]

### Step 8.4: Risk-Benefit
- **BENEFIT**: Prevents GPU page faults and incorrect memory access in
  common GPU operations; validates contradictory input flags
- **RISK**: Medium-sized change (~45 lines); restructures fence return
  logic; adds `dma_fence_unwrap_merge` dependency. However, the new
  logic is strictly more conservative (waits for more work) so
  regression risk is low.

Record: [High benefit, low-medium risk; net positive]

---

## PHASE 9: FINAL SYNTHESIS

### Step 9.1: Evidence Compilation

**FOR backporting**:
- Fixes real correctness bugs: missing PDE fence, invalid input not
  rejected
- Author is Christian König, AMD DRM co-maintainer with deep expertise
- Acked by AMD developer, committed by AMD DRM maintainer
- Single file change, well-contained
- Userspace-facing ioctl - affects all AMD GPU applications
- The missing PDE fence could cause GPU page faults or incorrect memory
  access
- All dependencies present in 7.0 tree
- Clean apply expected
- The fence merge is strictly more conservative (lower regression risk)

**AGAINST backporting**:
- Medium-sized change (~45 lines), not a tiny surgical fix
- Restructures fence return logic in `amdgpu_gem_va_update_vm`
- Adds new API usage (`dma_fence_unwrap_merge`)
- Bugs only exist in 7.0 code (limited affected trees)
- Area is under active development and may see more changes

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **Yes** - from subsystem maintainer,
   well-reviewed
2. Fixes a real bug? **Yes** - missing PDE fence, invalid input not
   rejected
3. Important issue? **Yes** - GPU page faults, incorrect memory access
   (HIGH severity)
4. Small and contained? **Yes** - single file, ~45 lines, 2 functions
5. No new features or APIs? **Correct** - no new features
6. Can apply to stable? **Yes** - context matches exactly for 7.0.y

### Step 9.3: Exception Categories
Not an exception category; standard bug fix.

### Step 9.4: Decision
The bugs are real and affect all AMD GPU users on 7.0.y. The missing PDE
fence is a correctness issue that can cause GPU page faults during
normal operations. The fix is from the subsystem maintainer, well-
reviewed, contained to a single file, and all dependencies are met.

---

## Verification

- [Phase 1] Parsed tags: Author Christian König (AMD DRM co-maintainer),
  Acked-by Sunil Khatri, committed by Alex Deucher
- [Phase 2] Diff analysis: ~45 lines changed in `amdgpu_gem.c`, modifies
  `amdgpu_gem_va_update_vm` and `amdgpu_gem_va_ioctl`
- [Phase 2] Verified `amdgpu_vm_update_pdes` stores its fence in
  `vm->last_update` (amdgpu_vm.c:1006) - confirms PDE fence is missed
  without the merge
- [Phase 2] Verified `amdgpu_vm_clear_freed` replaces `*fence` when work
  is done (amdgpu_vm.c:1583-1588) - confirms changing argument from
  `&fence` to `&vm->last_update` keeps VM state current
- [Phase 3] git blame: buggy code from `efdc66fe12b07` (2026-01-09) and
  `bd8150a1b3370` (2025-12-11)
- [Phase 3] git tag --contains: both commits first appear in
  v7.0-rc1/v7.0
- [Phase 3] Author's recent commits: 15 commits in amdgpu, including
  multiple critical fixes
- [Phase 4] b4 dig -c efdc66fe12b07: found v7 submission thread
- [Phase 4] b4 dig -a: patch went through v6->v7 iterations
- [Phase 5] `amdgpu_gem_va_ioctl` is registered as DRM ioctl at
  amdgpu_drv.c:3082 (DRM_AUTH|DRM_RENDER_ALLOW)
- [Phase 6] `bd8150a1b3370` NOT in v6.14 (verified via git log
  v6.12..v6.14); only in v7.0+
- [Phase 6] `dma_fence_unwrap_merge` exists in tree (include/linux/dma-
  fence-unwrap.h:69)
- [Phase 6] Current file state matches diff context exactly - clean
  apply expected
- [Phase 8] PDE fence bug: triggers on all MAP/REPLACE of non-always-
  valid BOs (common path)
- UNVERIFIED: Could not verify lore.kernel.org discussion for the
  specific commit under analysis (bot protection blocked access)

**YES**

 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 76 +++++++++++--------------
 1 file changed, 34 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a6107109a2b86..c4839cf2dce37 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -30,6 +30,7 @@
 #include <linux/pagemap.h>
 #include <linux/pci.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-unwrap.h>
 
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_drv.h>
@@ -744,11 +745,10 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	struct dma_fence *fence;
 	int r = 0;
 
-	/* Always start from the VM's existing last update fence. */
-	fence = dma_fence_get(vm->last_update);
-
+	/* If the VM is not ready return only a stub. */
 	if (!amdgpu_vm_ready(vm))
-		return fence;
+		return dma_fence_get_stub();
+
 
 	/*
 	 * First clean up any freed mappings in the VM.
@@ -757,7 +757,7 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	 * schedules GPU work. If nothing needs clearing, @fence can remain as
 	 * the original vm->last_update.
 	 */
-	r = amdgpu_vm_clear_freed(adev, vm, &fence);
+	r = amdgpu_vm_clear_freed(adev, vm, &vm->last_update);
 	if (r)
 		goto error;
 
@@ -774,47 +774,34 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	if (r)
 		goto error;
 
-	/*
-	 * Decide which fence best represents the last update:
-	 *
-	 * MAP/REPLACE:
-	 *   - For always-valid mappings, use vm->last_update.
-	 *   - Otherwise, export bo_va->last_pt_update.
-	 *
-	 * UNMAP/CLEAR:
-	 *   Keep the fence returned by amdgpu_vm_clear_freed(). If no work was
-	 *   needed, it can remain as vm->last_pt_update.
-	 *
-	 * The VM and BO update fences are always initialized to a valid value.
-	 * vm->last_update and bo_va->last_pt_update always start as valid fences.
-	 * and are never expected to be NULL.
-	 */
-	switch (operation) {
-	case AMDGPU_VA_OP_MAP:
-	case AMDGPU_VA_OP_REPLACE:
+	if ((operation == AMDGPU_VA_OP_MAP ||
+	     operation == AMDGPU_VA_OP_REPLACE) &&
+	    !amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo)) {
+
 		/*
-		 * For MAP/REPLACE, return the page table update fence for the
-		 * mapping we just modified. bo_va is expected to be valid here.
+		 * For MAP/REPLACE of non per-VM BOs we need to sync to both the
+		 * bo_va->last_pt_update and vm->last_update or otherwise we
+		 * potentially miss the PDE updates.
 		 */
-		dma_fence_put(fence);
-
-		if (amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo))
-			fence = dma_fence_get(vm->last_update);
-		else
-			fence = dma_fence_get(bo_va->last_pt_update);
-		break;
-	case AMDGPU_VA_OP_UNMAP:
-	case AMDGPU_VA_OP_CLEAR:
-	default:
-		/* keep @fence as returned by amdgpu_vm_clear_freed() */
-		break;
+		fence = dma_fence_unwrap_merge(vm->last_update,
+					       bo_va->last_pt_update);
+		if (!fence) {
+			/* As fallback in OOM situations */
+			dma_fence_wait(vm->last_update, false);
+			dma_fence_wait(bo_va->last_pt_update, false);
+			fence = dma_fence_get_stub();
+		}
+	} else {
+		fence = dma_fence_get(vm->last_update);
 	}
 
+	return fence;
+
 error:
 	if (r && r != -ERESTARTSYS)
 		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
 
-	return fence;
+	return dma_fence_get(vm->last_update);
 }
 
 int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
@@ -835,7 +822,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	struct amdgpu_bo_va *bo_va;
 	struct drm_syncobj *timeline_syncobj = NULL;
 	struct dma_fence_chain *timeline_chain = NULL;
-	struct dma_fence *fence;
 	struct drm_exec exec;
 	uint64_t vm_size;
 	int r = 0;
@@ -887,6 +873,10 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (args->flags & AMDGPU_VM_DELAY_UPDATE &&
+	    args->vm_timeline_syncobj_out)
+		return -EINVAL;
+
 	if ((args->operation != AMDGPU_VA_OP_CLEAR) &&
 	    !(args->flags & AMDGPU_VM_PAGE_PRT)) {
 		gobj = drm_gem_object_lookup(filp, args->handle);
@@ -976,11 +966,13 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 	 * that represents the last relevant update for this mapping. This
 	 * fence can then be exported to the user-visible VM timeline.
 	 */
-	if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && !adev->debug_vm) {
+	if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) &&
+	    (!adev->debug_vm || timeline_syncobj)) {
+		struct dma_fence *fence;
+
 		fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
 						args->operation);
-
-		if (timeline_syncobj && fence) {
+		if (timeline_syncobj) {
 			if (!args->vm_timeline_point) {
 				/* Replace the existing fence when no point is given. */
 				drm_syncobj_replace_fence(timeline_syncobj,
-- 
2.53.0