[PATCH 0/4] drm/amdgpu: fix bugs when amdgpu_vm_ptes_update fails

[PATCH 0/4] drm/amdgpu: fix bugs when amdgpu_vm_ptes_update fails

[Thadeu Lima de Souza Cascardo]

When amdgpu_vm_ptes_update fails, there might be jobs leak and mappings
might be kept in an on-stack list, which will lead to invalid list
walking later on.

One of the approaches that was attempted was introducing an abort
function to amdgpu_vm_update_funcs. But given the possibility that some
mappings have already been pushed and committed and some may have been
moved to tlb_flush_waitlist, do a partial commit instead and return the
error to amdgpu_vm_update_range caller.

Also, fix amdgpu_vm_clear_freed ignoring the result of
amdgpu_vm_update_range.

I just noticed a different fix for the latter was submitted at [1]. It
re-adds the mapping to the freed list instead of not removing it when
amdgpu_vm_update_range fails.

[1] https://lore.kernel.org/amd-gfx/20260905023151.90699-1-oushinnyo@163.com/

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
Thadeu Lima de Souza Cascardo (4):
      drm/amdgpu: make amdgpu_vm_sdma_commit handle NULL or empty job
      drm/amdgpu: make amdgpu_vm_update_funcs commit return void
      drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails
      drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails

 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 14 +++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  |  8 +++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 26 ++++++++++++++++----------
 5 files changed, 27 insertions(+), 27 deletions(-)
---
base-commit: 59ced288fcba9e91bd38e61a972ad782c4edb7d0
change-id: 20260914-amdgpu_vm_fixes-d64d1478657e

Best regards,
--  
Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

[PATCH 1/4] drm/amdgpu: make amdgpu_vm_sdma_commit handle NULL or empty job

[Thadeu Lima de Souza Cascardo]

When amdgpu_vm_update_range hits a failure at amdgpu_vm_ptes_update, it
will skip calling update_funcs commit, which might lead to a memory
leak.

Prepare the commit function to be called in such error paths, where job
might be NULL or empty.

This allows the following commits to prevent the job leak below in the
presence of such failures.

[ 2877.877264] kmemleak: unreferenced object 0xffff89b1f3cf0800 (size 1024):
[ 2877.877273] kmemleak:   comm "vm_always_valid", pid 3027, jiffies 4295714403
[ 2877.877275] kmemleak:   hex dump (first 32 bytes):
[ 2877.877277] kmemleak:     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[ 2877.877279] kmemleak:     00 03 da 52 b1 89 ff ff 38 25 5f cb b1 89 ff ff  ...R....8%_.....
[ 2877.877280] kmemleak:   backtrace (crc d52b9041):
[ 2877.877282] kmemleak:     __kmalloc_noprof+0x4b3/0x770
[ 2877.877288] kmemleak:     amdgpu_job_alloc+0x69/0x280 [amdgpu]
[ 2877.877668] kmemleak:     amdgpu_job_alloc_with_ib+0x55/0xf0 [amdgpu]
[ 2877.878023] kmemleak:     amdgpu_vm_sdma_prepare+0x4b/0xa0 [amdgpu]
[ 2877.878350] kmemleak:     amdgpu_vm_update_range+0x24f/0x940 [amdgpu]
[ 2877.878668] kmemleak:     amdgpu_vm_clear_freed+0x13b/0x290 [amdgpu]
[ 2877.878983] kmemleak:     amdgpu_gem_object_close+0x1ac/0x270 [amdgpu]
[ 2877.879300] kmemleak:     drm_gem_object_release_handle+0x35/0xd0
[ 2877.879305] kmemleak:     idr_for_each+0x70/0xe0
[ 2877.879310] kmemleak:     drm_gem_release+0x23/0x30
[ 2877.879311] kmemleak:     drm_file_free+0x217/0x2a0
[ 2877.879314] kmemleak:     drm_release+0x61/0xe0
[ 2877.879317] kmemleak:     amdgpu_drm_release+0x62/0xd0 [amdgpu]
[ 2877.879630] kmemleak:     __fput+0xfb/0x2d0
[ 2877.879634] kmemleak:     __x64_sys_close+0x3d/0x80
[ 2877.879637] kmemleak:     do_syscall_64+0x12d/0x6c0

Fixes: 81417bea8755 ("drm/amdgpu: explicitly sync VM update to PDs/PTs")
Fixes: c3546695830e ("drm/amdgpu: use the new VM backend for PTEs")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 50cc0779c340..975e26a48cd3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -90,6 +90,7 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 	if (r) {
 		p->num_dw_left = 0;
 		amdgpu_job_free(p->job);
+		p->job = NULL;
 	}
 	return r;
 }
@@ -106,14 +107,24 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 				 struct dma_fence **fence)
 {
-	struct amdgpu_ib *ib = p->job->ibs;
+	struct amdgpu_ib *ib;
 	struct amdgpu_ring *ring;
 	struct dma_fence *f;
 
+	if (!p->job)
+		return 0;
+
+	ib = p->job->ibs;
+
+	if (ib->length_dw == 0) {
+		amdgpu_job_free(p->job);
+		p->job = NULL;
+		return 0;
+	}
+
 	ring = container_of(p->vm->delayed.rq->sched, struct amdgpu_ring,
 			    sched);
 
-	WARN_ON(ib->length_dw == 0);
 	amdgpu_ring_pad_ib(ring, ib);
 
 	if (p->needs_flush)

-- 
2.47.3

[PATCH 2/4] drm/amdgpu: make amdgpu_vm_update_funcs commit return void

[Thadeu Lima de Souza Cascardo]

As both SDMA and CPU implementations always return 0, they can as well
just return void.

This is necessary to avoid clobbering the return code in
amdgpu_vm_update_range when amdgpu_vm_ptes_update fails.

Fixes: 81417bea8755 ("drm/amdgpu: explicitly sync VM update to PDs/PTs")
Fixes: c3546695830e ("drm/amdgpu: use the new VM backend for PTEs")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  8 ++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  |  8 +++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 15 +++++----------
 5 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index bb04101b0fb5..09037df76400 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1027,9 +1027,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 			goto error;
 	}
 
-	r = vm->update_funcs->commit(&params, &vm->last_update);
-	if (r)
-		goto error;
+	vm->update_funcs->commit(&params, &vm->last_update);
 
 	if (flush_tlb_needed)
 		atomic64_inc(&vm->tlb_seq);
@@ -1243,9 +1241,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		start = tmp;
 	}
 
-	r = vm->update_funcs->commit(&params, fence);
-	if (r)
-		goto error_free;
+	vm->update_funcs->commit(&params, fence);
 
 	if (params.needs_flush) {
 		amdgpu_vm_tlb_flush(&params, fence, tlb_cb);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d32183cd9e0f..3b237e78c8a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -332,8 +332,8 @@ struct amdgpu_vm_update_funcs {
 	int (*update)(struct amdgpu_vm_update_params *p,
 		      struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
 		      unsigned count, uint32_t incr, uint64_t flags);
-	int (*commit)(struct amdgpu_vm_update_params *p,
-		      struct dma_fence **fence);
+	void (*commit)(struct amdgpu_vm_update_params *p,
+		       struct dma_fence **fence);
 };
 
 struct amdgpu_vm_fault_info {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
index b31ff6f56f0d..5c17946632cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
@@ -116,8 +116,8 @@ static int amdgpu_vm_cpu_update(struct amdgpu_vm_update_params *p,
  *
  * Make sure that the hardware sees the page table updates.
  */
-static int amdgpu_vm_cpu_commit(struct amdgpu_vm_update_params *p,
-				struct dma_fence **fence)
+static void amdgpu_vm_cpu_commit(struct amdgpu_vm_update_params *p,
+				 struct dma_fence **fence)
 {
 	struct amdgpu_device *adev = p->adev;
 
@@ -127,12 +127,10 @@ static int amdgpu_vm_cpu_commit(struct amdgpu_vm_update_params *p,
 	mb();
 	/* A reset flushed the HDP anyway, so that here can be skipped when a reset is ongoing */
 	if (!down_read_trylock(&adev->reset_domain->sem))
-		return 0;
+		return;
 
 	amdgpu_device_flush_hdp(adev, NULL);
 	up_read(&adev->reset_domain->sem);
-
-	return 0;
 }
 
 const struct amdgpu_vm_update_funcs amdgpu_vm_cpu_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index e43a60d09808..e9622adf1246 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -422,7 +422,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto exit;
 
-	r = vm->update_funcs->commit(&params, NULL);
+	vm->update_funcs->commit(&params, NULL);
 exit:
 	drm_dev_exit(idx);
 	return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 975e26a48cd3..52dc67b50fae 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -101,25 +101,23 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
  * @p: see amdgpu_vm_update_params definition
  * @fence: resulting fence
  *
- * Returns:
- * Negativ errno, 0 for success.
  */
-static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
-				 struct dma_fence **fence)
+static void amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
+				  struct dma_fence **fence)
 {
 	struct amdgpu_ib *ib;
 	struct amdgpu_ring *ring;
 	struct dma_fence *f;
 
 	if (!p->job)
-		return 0;
+		return;
 
 	ib = p->job->ibs;
 
 	if (ib->length_dw == 0) {
 		amdgpu_job_free(p->job);
 		p->job = NULL;
-		return 0;
+		return;
 	}
 
 	ring = container_of(p->vm->delayed.rq->sched, struct amdgpu_ring,
@@ -153,7 +151,6 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 		swap(*fence, f);
 	}
 	dma_fence_put(f);
-	return 0;
 }
 
 /**
@@ -257,9 +254,7 @@ static int amdgpu_vm_sdma_update(struct amdgpu_vm_update_params *p,
 		ndw -= p->job->ibs->length_dw;
 
 		if (ndw < 32) {
-			r = amdgpu_vm_sdma_commit(p, NULL);
-			if (r)
-				return r;
+			amdgpu_vm_sdma_commit(p, NULL);
 
 			r = amdgpu_vm_sdma_alloc_job(p, count,
 						     AMDGPU_KERNEL_JOB_ID_VM_UPDATE);

-- 
2.47.3

[PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Thadeu Lima de Souza Cascardo]

If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
the clearing of that mapping will not be attempted again. Later on, when
an IB tries to read that mapping, the read succeeds, leading to a
potential info leak, or even data corruption, if it attempts to write to
it.

If the mapping is left in the freed list, then clearing will be
attempted again during amdgpu_cs_ioctl, which will either fail and not
submit the job or will succeed in clearing the mapping, preventing the
invalid access.

Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 09037df76400..5bde36754607 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 	while (!list_empty(&vm->freed)) {
 		mapping = list_first_entry(&vm->freed,
 			struct amdgpu_bo_va_mapping, list);
-		list_del(&mapping->list);
 
 		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
 					   &sync, mapping->start, mapping->last,
 					   0, 0, 0, NULL, NULL, &f);
-		amdgpu_vm_free_mapping(adev, vm, mapping, f);
 		if (r) {
 			dma_fence_put(f);
 			goto error_free;
 		}
+		list_del(&mapping->list);
+		amdgpu_vm_free_mapping(adev, vm, mapping, f);
 	}
 
 	if (fence && f) {

-- 
2.47.3

[PATCH 4/4] drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails

[Thadeu Lima de Souza Cascardo]

Otherwise, we might leak the SDMA job.

[ 2877.877264] kmemleak: unreferenced object 0xffff89b1f3cf0800 (size 1024):
[ 2877.877273] kmemleak:   comm "vm_always_valid", pid 3027, jiffies 4295714403
[ 2877.877275] kmemleak:   hex dump (first 32 bytes):
[ 2877.877277] kmemleak:     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[ 2877.877279] kmemleak:     00 03 da 52 b1 89 ff ff 38 25 5f cb b1 89 ff ff  ...R....8%_.....
[ 2877.877280] kmemleak:   backtrace (crc d52b9041):
[ 2877.877282] kmemleak:     __kmalloc_noprof+0x4b3/0x770
[ 2877.877288] kmemleak:     amdgpu_job_alloc+0x69/0x280 [amdgpu]
[ 2877.877668] kmemleak:     amdgpu_job_alloc_with_ib+0x55/0xf0 [amdgpu]
[ 2877.878023] kmemleak:     amdgpu_vm_sdma_prepare+0x4b/0xa0 [amdgpu]
[ 2877.878350] kmemleak:     amdgpu_vm_update_range+0x24f/0x940 [amdgpu]
[ 2877.878668] kmemleak:     amdgpu_vm_clear_freed+0x13b/0x290 [amdgpu]
[ 2877.878983] kmemleak:     amdgpu_gem_object_close+0x1ac/0x270 [amdgpu]
[ 2877.879300] kmemleak:     drm_gem_object_release_handle+0x35/0xd0
[ 2877.879305] kmemleak:     idr_for_each+0x70/0xe0
[ 2877.879310] kmemleak:     drm_gem_release+0x23/0x30
[ 2877.879311] kmemleak:     drm_file_free+0x217/0x2a0
[ 2877.879314] kmemleak:     drm_release+0x61/0xe0
[ 2877.879317] kmemleak:     amdgpu_drm_release+0x62/0xd0 [amdgpu]
[ 2877.879630] kmemleak:     __fput+0xfb/0x2d0
[ 2877.879634] kmemleak:     __x64_sys_close+0x3d/0x80
[ 2877.879637] kmemleak:     do_syscall_64+0x12d/0x6c0

Also, flush the TLB and free the entries in the local stack
tlb_flush_waitlist. Otherwise we might touch old stack addresses when
removing the entries when we finish the VM.

[ 2791.162517] ------------[ cut here ]------------
[ 2791.162519] list_del corruption. next->prev should be ffff89b27cf0b778, but was 0000000000000000. (next=ffffcacdc12e77c8)
[ 2791.162522] WARNING: lib/list_debug.c:65 at __list_del_entry_valid_or_report+0xea/0x100, CPU#1: vm_always_valid/3027
[ 2791.162659] CPU: 1 UID: 1000 PID: 3027 Comm: vm_always_valid Tainted: G        W           7.2.0-02902-g432c2ced2942 #11 PREEMPT  2356ac135df40d3c9080402059c7492776a63ecd
[ 2791.162663] Tainted: [W]=WARN
[ 2791.162665] Hardware name: Valve Jupiter/Jupiter, BIOS F7A0133 08/05/2024
[ 2791.162668] RIP: 0010:__list_del_entry_valid_or_report+0xf4/0x100
[...]
[ 2791.162688] Call Trace:
[ 2791.162690]  <TASK>
[ 2791.162694]  amdgpu_vm_pt_free+0x5d/0xa0 [amdgpu 09338d95446a22cfd002ef2392fde61d2ef7b139]
[ 2791.163070]  amdgpu_vm_pt_free_root+0xe3/0x130 [amdgpu 09338d95446a22cfd002ef2392fde61d2ef7b139]
[ 2791.163392]  amdgpu_vm_fini+0x320/0x5f0 [amdgpu 09338d95446a22cfd002ef2392fde61d2ef7b139]
[ 2791.163709]  ? __xa_erase+0x57/0xa0
[ 2791.163714]  ? _raw_spin_unlock_irqrestore+0x34/0x60
[ 2791.163718]  ? _raw_spin_unlock_irqrestore+0x34/0x60
[ 2791.163720]  ? trace_hardirqs_on+0x16/0xd0
[ 2791.163725]  ? _raw_spin_unlock_irqrestore+0x3f/0x60
[ 2791.163728]  amdgpu_driver_postclose_kms+0x1d1/0x2d0 [amdgpu 09338d95446a22cfd002ef2392fde61d2ef7b139]
[ 2791.164042]  drm_file_free+0x23a/0x2a0
[ 2791.164049]  drm_release+0x61/0xe0
[ 2791.164052]  amdgpu_drm_release+0x62/0xd0 [amdgpu 09338d95446a22cfd002ef2392fde61d2ef7b139]
[ 2791.164362]  __fput+0xfb/0x2d0
[ 2791.164367]  __x64_sys_close+0x3d/0x80
[ 2791.164371]  do_syscall_64+0x12d/0x6c0

This can be caused by VRAM memory pressure, where a PT BO allocation
fails during amdgpu_gem_va_ioctl.

Since the failure from amdgpu_vm_update_range is still returned,
amdgpu_vm_bo_update will leave the mappings at the invalids list and
amdgpu_vm_clear_freed will keep them at the freed list. This will allow
amdgpu_cs_ioctl to retry later.

Fixes: 81417bea8755 ("drm/amdgpu: explicitly sync VM update to PDs/PTs")
Fixes: c3546695830e ("drm/amdgpu: use the new VM backend for PTEs")
Fixes: b6c4f90b3819 ("drm/amdgpu: sync page table freeing with tlb flush")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 5bde36754607..1495adaaacd8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1235,7 +1235,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		tmp = start + num_entries;
 		r = amdgpu_vm_ptes_update(&params, start, tmp, addr, flags);
 		if (r)
-			goto error_free;
+			break;
 
 		amdgpu_res_next(&cursor, num_entries * AMDGPU_GPU_PAGE_SIZE);
 		start = tmp;

-- 
2.47.3

Re: [PATCH 4/4] drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Ignored return value of update_funcs->update leads to NULL pointer dereference or silent GPU page faults.
- [High] Dangling pointer / Use-After-Free in amdgpu_vm_pt_alloc when amdgpu_vm_pt_clear fails.
--

commit 7a14ea1f8dd61c2e4083bcbdd9192e590552290f
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

drm/amdgpu: commit pending work when amdgpu_vm_ptes_update fails

This commit changes the error path when amdgpu_vm_ptes_update() fails to
break the loop and commit pending work instead of jumping to error_free.
This fixes an SDMA job memory leak and ensures TLB entries are flushed
and local stack waitlists are cleared properly.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 5bde36754607f..1495adaaacd89 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1235,7 +1235,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		tmp = start + num_entries;
>  		r = amdgpu_vm_ptes_update(&params, start, tmp, addr, flags);
>  		if (r)
> -			goto error_free;
> +			break;

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free in the
page table allocation path called from amdgpu_vm_ptes_update():

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:amdgpu_vm_pt_alloc() {
    ...
    error_free_pt:
        amdgpu_bo_unref(&pt_bo);
        return r;
}

If amdgpu_vm_pt_clear() fails (e.g., due to memory allocation failure or
fence wait timeout), it jumps to error_free_pt and frees the newly
allocated pt_bo. However, this BO has already been assigned to entry->bo
and added to VM tracking lists by the preceding call to
amdgpu_vm_bo_base_init(). Is it possible that freeing pt_bo here leaves
entry->bo pointing to the freed memory?

[Severity: Critical]
This is a pre-existing issue, but there appears to be an unhandled error
path during PTE updates called from amdgpu_vm_ptes_update():

drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:amdgpu_vm_pte_update_flags() {
    ...
    params->vm->update_funcs->update(params, pt, pe, addr, count, incr,
                                     flags);
}

The update function's return value is completely ignored. If the update
fails (for example, if amdgpu_vm_sdma_update() fails to allocate a job due
to ENOMEM), the job pointer (p->job) is set to NULL. Since the error is
dropped, amdgpu_vm_ptes_update() continues iterating and calls
amdgpu_vm_pte_update_flags() again.

Will this cause amdgpu_vm_sdma_update() to dereference the NULL p->job on
the next iteration, leading to a kernel panic? Or if it's a CPU update,
will ignoring the failure lead to invalid GPU page tables and subsequent
GPU page faults?

>  
>  		amdgpu_res_next(&cursor, num_entries * AMDGPU_GPU_PAGE_SIZE);
>  		start = tmp;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-amdgpu_vm_fixes-v1-0-d6d835591e0f@igalia.com?part=4

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] amdgpu_vm_clear_freed errors are ignored in KFD BO unmap and GEM close paths, leaving active PTEs mapped to freed memory. A running user-mode queue shader can continue to access the memory after it has been reallocated.
--

commit 16b841a63bb4a2522f39993f74bd498158fed54e
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

This commit modifies amdgpu_vm_clear_freed to avoid freeing a mapping and
removing it from the freed list if amdgpu_vm_update_range fails. It assumes
that leaving the mapping in the freed list will allow a subsequent
amdgpu_cs_ioctl to retry clearing it, preventing invalid access.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 09037df76400d..5bde36754607f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>  	while (!list_empty(&vm->freed)) {
>  		mapping = list_first_entry(&vm->freed,
>  			struct amdgpu_bo_va_mapping, list);
> -		list_del(&mapping->list);
>  
>  		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>  					   &sync, mapping->start, mapping->last,
>  					   0, 0, 0, NULL, NULL, &f);
> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  		if (r) {
>  			dma_fence_put(f);
>  			goto error_free;
>  		}

[Severity: Critical]
This is a pre-existing issue, but does leaving the mapping in the freed list
actually guarantee it will be cleared later?

The commit message assumes that a later amdgpu_cs_ioctl will retry the
clearing. However, are there paths that bypass the CS ioctl where this error
is ignored?

For instance, in KFD unmap_bo_from_gpuvm in
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c, the error from
amdgpu_vm_clear_freed is explicitly ignored:

	(void)amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update);

This marks the BO as unmapped in software, allowing it to be subsequently
freed via amdgpu_amdkfd_gpuvm_free_memory_of_gpu without waiting for the
PTEs to be cleared. Because KFD uses hardware user-mode queues, the clearing
is never retried by a CS ioctl. A running user-mode queue shader could
continue to access the freed memory.

Similarly, in amdgpu_gem_object_close in
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c, the error is ignored and it bails
out without attaching a fence:

	r = amdgpu_vm_clear_freed(adev, vm, &fence);
	if (unlikely(r < 0) && !drm_dev_is_unplugged(adev_to_drm(adev)))
		dev_err(adev->dev, "failed to clear page "
			"tables on GEM object close (%ld)\n", r);
	if (r || !fence)
		goto out_unlock;

Could this leave active PTEs mapped to freed memory, leading to a potential
information leak or memory corruption if amdgpu_vm_update_range fails
(e.g., due to an early -ENOMEM)?

> +		list_del(&mapping->list);
> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  	}
>  
>  	if (fence && f) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-amdgpu_vm_fixes-v1-0-d6d835591e0f@igalia.com?part=3

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Christian König]

On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
> the clearing of that mapping will not be attempted again. Later on, when
> an IB tries to read that mapping, the read succeeds, leading to a
> potential info leak, or even data corruption, if it attempts to write to
> it.
> 
> If the mapping is left in the freed list, then clearing will be
> attempted again during amdgpu_cs_ioctl, which will either fail and not
> submit the job or will succeed in clearing the mapping, preventing the
> invalid access.

Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.

amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.

Regards,
Christian.

> 
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 09037df76400..5bde36754607 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>  	while (!list_empty(&vm->freed)) {
>  		mapping = list_first_entry(&vm->freed,
>  			struct amdgpu_bo_va_mapping, list);
> -		list_del(&mapping->list);
>  
>  		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>  					   &sync, mapping->start, mapping->last,
>  					   0, 0, 0, NULL, NULL, &f);
> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  		if (r) {
>  			dma_fence_put(f);
>  			goto error_free;
>  		}
> +		list_del(&mapping->list);
> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>  	}
>  
>  	if (fence && f) {
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Natalie Vock]

On 9/15/26 10:33, Christian König wrote:
> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
>> the clearing of that mapping will not be attempted again. Later on, when
>> an IB tries to read that mapping, the read succeeds, leading to a
>> potential info leak, or even data corruption, if it attempts to write to
>> it.
>>
>> If the mapping is left in the freed list, then clearing will be
>> attempted again during amdgpu_cs_ioctl, which will either fail and not
>> submit the job or will succeed in clearing the mapping, preventing the
>> invalid access.
> 
> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> 
> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.

Actually, how so? The SDMA update path has lots of allocations that 
happen at random points throughout the update process (think the SDMA IB 
being exhausted, then vm_funcs->update() will allocate a new one and can 
fail on that, or various fence allocations. Some paths allocate from the 
delayed IB pool which is just regular GFP_KERNEL, why couldn't these 
allocations fail?

Aside from that, the more trivial failure path that I think Thadeu is 
also talking about here is that amdgpu_vm_ptes_update will call 
amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and 
these allocations can fail. I'm not even sure if drm_exec can save this 
one because we need to be ready to drop locks and retry.

I get that failing is unacceptable for the cases inside a pagefault 
handler (though I'm not sure if anything of what I mentioned applies 
here, pagefault VM updates are immediate so they get GFP_ATOMIC, but 
maybe the pagefault handler could still allocate PTs?) but for anything 
else, failure seems like a possibility and maybe we should think about 
handling it in some way.

I'm not sure I see a problem in keeping BOs invalidated and backing off 
later, since new submissions should be rejected unless the VM manages to 
complete all updates successfully.

Best,
Natalie

> 
> Regards,
> Christian.
> 
>>
>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 09037df76400..5bde36754607 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>   	while (!list_empty(&vm->freed)) {
>>   		mapping = list_first_entry(&vm->freed,
>>   			struct amdgpu_bo_va_mapping, list);
>> -		list_del(&mapping->list);
>>   
>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>   					   &sync, mapping->start, mapping->last,
>>   					   0, 0, 0, NULL, NULL, &f);
>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>   		if (r) {
>>   			dma_fence_put(f);
>>   			goto error_free;
>>   		}
>> +		list_del(&mapping->list);
>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>   	}
>>   
>>   	if (fence && f) {
>>
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Thadeu Lima de Souza Cascardo]

On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
> On 9/15/26 10:33, Christian König wrote:
> > On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
> > > If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
> > > the clearing of that mapping will not be attempted again. Later on, when
> > > an IB tries to read that mapping, the read succeeds, leading to a
> > > potential info leak, or even data corruption, if it attempts to write to
> > > it.
> > > 
> > > If the mapping is left in the freed list, then clearing will be
> > > attempted again during amdgpu_cs_ioctl, which will either fail and not
> > > submit the job or will succeed in clearing the mapping, preventing the
> > > invalid access.
> > 
> > Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> > 
> > amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
> 
> Actually, how so? The SDMA update path has lots of allocations that happen
> at random points throughout the update process (think the SDMA IB being
> exhausted, then vm_funcs->update() will allocate a new one and can fail on
> that, or various fence allocations. Some paths allocate from the delayed IB
> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
> 
> Aside from that, the more trivial failure path that I think Thadeu is also
> talking about here is that amdgpu_vm_ptes_update will call
> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
> allocations can fail. I'm not even sure if drm_exec can save this one
> because we need to be ready to drop locks and retry.
> 
> I get that failing is unacceptable for the cases inside a pagefault handler
> (though I'm not sure if anything of what I mentioned applies here, pagefault
> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
> handler could still allocate PTs?) but for anything else, failure seems like
> a possibility and maybe we should think about handling it in some way.
> 
> I'm not sure I see a problem in keeping BOs invalidated and backing off
> later, since new submissions should be rejected unless the VM manages to
> complete all updates successfully.
> 
> Best,
> Natalie
> 

Thanks, Natalie, those are exactly the possible scenarios here:
amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.

Christian,

I have started this investigation once I observed the kmemleak that is
fixed by the other 3 patches. Natalie brings up the exact case where this
might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.

Here is a log showing up that something failed during the update:
[ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)

The investigation then led to a reproducer, which is able to trigger the
list corruption that is mentioned in patch 4.

And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
trace, that should clearly be where the failure has happened. Looking at
the function, then I noticed it ignores the return code and still removes
the mappings from the list. I was able to write a second reproducer that
triggers the failure, then releases some memory and demonstrate that
amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
empty.

Regards.
Cascardo.

> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > index 09037df76400..5bde36754607 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> > >   	while (!list_empty(&vm->freed)) {
> > >   		mapping = list_first_entry(&vm->freed,
> > >   			struct amdgpu_bo_va_mapping, list);
> > > -		list_del(&mapping->list);
> > >   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> > >   					   &sync, mapping->start, mapping->last,
> > >   					   0, 0, 0, NULL, NULL, &f);
> > > -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> > >   		if (r) {
> > >   			dma_fence_put(f);
> > >   			goto error_free;
> > >   		}
> > > +		list_del(&mapping->list);
> > > +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> > >   	}
> > >   	if (fence && f) {
> > > 
> > 
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Christian König]

On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
>> On 9/15/26 10:33, Christian König wrote:
>>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
>>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
>>>> the clearing of that mapping will not be attempted again. Later on, when
>>>> an IB tries to read that mapping, the read succeeds, leading to a
>>>> potential info leak, or even data corruption, if it attempts to write to
>>>> it.
>>>>
>>>> If the mapping is left in the freed list, then clearing will be
>>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
>>>> submit the job or will succeed in clearing the mapping, preventing the
>>>> invalid access.
>>>
>>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
>>>
>>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
>>
>> Actually, how so? The SDMA update path has lots of allocations that happen
>> at random points throughout the update process (think the SDMA IB being
>> exhausted, then vm_funcs->update() will allocate a new one and can fail on
>> that, or various fence allocations. Some paths allocate from the delayed IB
>> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
>>
>> Aside from that, the more trivial failure path that I think Thadeu is also
>> talking about here is that amdgpu_vm_ptes_update will call
>> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
>> allocations can fail. I'm not even sure if drm_exec can save this one
>> because we need to be ready to drop locks and retry.
>>
>> I get that failing is unacceptable for the cases inside a pagefault handler
>> (though I'm not sure if anything of what I mentioned applies here, pagefault
>> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
>> handler could still allocate PTs?) but for anything else, failure seems like
>> a possibility and maybe we should think about handling it in some way.
>>
>> I'm not sure I see a problem in keeping BOs invalidated and backing off
>> later, since new submissions should be rejected unless the VM manages to
>> complete all updates successfully.
>>
>> Best,
>> Natalie
>>
> 
> Thanks, Natalie, those are exactly the possible scenarios here:
> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
> 
> Christian,
> 
> I have started this investigation once I observed the kmemleak that is
> fixed by the other 3 patches. Natalie brings up the exact case where this
> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.

Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
> Here is a log showing up that something failed during the update:
> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)

That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.

> 
> The investigation then led to a reproducer, which is able to trigger the
> list corruption that is mentioned in patch 4.
> 
> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
> trace, that should clearly be where the failure has happened. Looking at
> the function, then I noticed it ignores the return code and still removes
> the mappings from the list. I was able to write a second reproducer that
> triggers the failure, then releases some memory and demonstrate that
> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
> empty.

Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.

So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).

I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.

When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.

Regards,
Christian.

> 
> Regards.
> Cascardo.
> 
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 09037df76400..5bde36754607 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>   	while (!list_empty(&vm->freed)) {
>>>>   		mapping = list_first_entry(&vm->freed,
>>>>   			struct amdgpu_bo_va_mapping, list);
>>>> -		list_del(&mapping->list);
>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>>>   					   &sync, mapping->start, mapping->last,
>>>>   					   0, 0, 0, NULL, NULL, &f);
>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>   		if (r) {
>>>>   			dma_fence_put(f);
>>>>   			goto error_free;
>>>>   		}
>>>> +		list_del(&mapping->list);
>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>   	}
>>>>   	if (fence && f) {
>>>>
>>>
>>

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Thadeu Lima de Souza Cascardo]

On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
> On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
> >> On 9/15/26 10:33, Christian König wrote:
> >>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
> >>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
> >>>> the clearing of that mapping will not be attempted again. Later on, when
> >>>> an IB tries to read that mapping, the read succeeds, leading to a
> >>>> potential info leak, or even data corruption, if it attempts to write to
> >>>> it.
> >>>>
> >>>> If the mapping is left in the freed list, then clearing will be
> >>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
> >>>> submit the job or will succeed in clearing the mapping, preventing the
> >>>> invalid access.
> >>>
> >>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> >>>
> >>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
> >>
> >> Actually, how so? The SDMA update path has lots of allocations that happen
> >> at random points throughout the update process (think the SDMA IB being
> >> exhausted, then vm_funcs->update() will allocate a new one and can fail on
> >> that, or various fence allocations. Some paths allocate from the delayed IB
> >> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
> >>
> >> Aside from that, the more trivial failure path that I think Thadeu is also
> >> talking about here is that amdgpu_vm_ptes_update will call
> >> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
> >> allocations can fail. I'm not even sure if drm_exec can save this one
> >> because we need to be ready to drop locks and retry.
> >>
> >> I get that failing is unacceptable for the cases inside a pagefault handler
> >> (though I'm not sure if anything of what I mentioned applies here, pagefault
> >> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
> >> handler could still allocate PTs?) but for anything else, failure seems like
> >> a possibility and maybe we should think about handling it in some way.
> >>
> >> I'm not sure I see a problem in keeping BOs invalidated and backing off
> >> later, since new submissions should be rejected unless the VM manages to
> >> complete all updates successfully.
> >>
> >> Best,
> >> Natalie
> >>
> > 
> > Thanks, Natalie, those are exactly the possible scenarios here:
> > amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
> > 
> > Christian,
> > 
> > I have started this investigation once I observed the kmemleak that is
> > fixed by the other 3 patches. Natalie brings up the exact case where this
> > might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
> > they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
> 
> Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
> > Here is a log showing up that something failed during the update:
> > [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)
> 
> That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.
> 

Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
the failure here and leaves the mappings in the invalids list, which the
next CS will try to map.

> > 
> > The investigation then led to a reproducer, which is able to trigger the
> > list corruption that is mentioned in patch 4.
> > 
> > And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
> > trace, that should clearly be where the failure has happened. Looking at
> > the function, then I noticed it ignores the return code and still removes
> > the mappings from the list. I was able to write a second reproducer that
> > triggers the failure, then releases some memory and demonstrate that
> > amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
> > empty.
> 
> Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.
> 
> So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).
> 
> I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.
> 
> When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.
> 

What I understand that happens in the case of clear_freed is that we are
breaking a PDB/hugepage and descending, as the clear happened in the middle
of the hugepage. With the specific reproducer I had to go through lengths
of making sure that the original mapping landed on a hugepage-aligned
address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
the middle of it.

The other reproducer that I have required that I used REPLACE instead of
MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
instead of MAP, so that explains that we can see this in production.

Regards.
Cascardo.

> Regards,
> Christian.
> 
> > 
> > Regards.
> > Cascardo.
> > 
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>>
> >>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> >>>> ---
> >>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
> >>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> index 09037df76400..5bde36754607 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> >>>>   	while (!list_empty(&vm->freed)) {
> >>>>   		mapping = list_first_entry(&vm->freed,
> >>>>   			struct amdgpu_bo_va_mapping, list);
> >>>> -		list_del(&mapping->list);
> >>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> >>>>   					   &sync, mapping->start, mapping->last,
> >>>>   					   0, 0, 0, NULL, NULL, &f);
> >>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>   		if (r) {
> >>>>   			dma_fence_put(f);
> >>>>   			goto error_free;
> >>>>   		}
> >>>> +		list_del(&mapping->list);
> >>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>   	}
> >>>>   	if (fence && f) {
> >>>>
> >>>
> >>
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Christian König]

On 9/15/26 13:34, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
>> On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
>>> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
>>>> On 9/15/26 10:33, Christian König wrote:
>>>>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
>>>>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
>>>>>> the clearing of that mapping will not be attempted again. Later on, when
>>>>>> an IB tries to read that mapping, the read succeeds, leading to a
>>>>>> potential info leak, or even data corruption, if it attempts to write to
>>>>>> it.
>>>>>>
>>>>>> If the mapping is left in the freed list, then clearing will be
>>>>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
>>>>>> submit the job or will succeed in clearing the mapping, preventing the
>>>>>> invalid access.
>>>>>
>>>>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
>>>>>
>>>>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
>>>>
>>>> Actually, how so? The SDMA update path has lots of allocations that happen
>>>> at random points throughout the update process (think the SDMA IB being
>>>> exhausted, then vm_funcs->update() will allocate a new one and can fail on
>>>> that, or various fence allocations. Some paths allocate from the delayed IB
>>>> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
>>>>
>>>> Aside from that, the more trivial failure path that I think Thadeu is also
>>>> talking about here is that amdgpu_vm_ptes_update will call
>>>> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
>>>> allocations can fail. I'm not even sure if drm_exec can save this one
>>>> because we need to be ready to drop locks and retry.
>>>>
>>>> I get that failing is unacceptable for the cases inside a pagefault handler
>>>> (though I'm not sure if anything of what I mentioned applies here, pagefault
>>>> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
>>>> handler could still allocate PTs?) but for anything else, failure seems like
>>>> a possibility and maybe we should think about handling it in some way.
>>>>
>>>> I'm not sure I see a problem in keeping BOs invalidated and backing off
>>>> later, since new submissions should be rejected unless the VM manages to
>>>> complete all updates successfully.
>>>>
>>>> Best,
>>>> Natalie
>>>>
>>>
>>> Thanks, Natalie, those are exactly the possible scenarios here:
>>> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
>>>
>>> Christian,
>>>
>>> I have started this investigation once I observed the kmemleak that is
>>> fixed by the other 3 patches. Natalie brings up the exact case where this
>>> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
>>> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
>>
>> Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
>>> Here is a log showing up that something failed during the update:
>>> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)
>>
>> That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.
>>
> 
> Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
> the failure here and leaves the mappings in the invalids list, which the
> next CS will try to map.
> 
>>>
>>> The investigation then led to a reproducer, which is able to trigger the
>>> list corruption that is mentioned in patch 4.
>>>
>>> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
>>> trace, that should clearly be where the failure has happened. Looking at
>>> the function, then I noticed it ignores the return code and still removes
>>> the mappings from the list. I was able to write a second reproducer that
>>> triggers the failure, then releases some memory and demonstrate that
>>> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
>>> empty.
>>
>> Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.
>>
>> So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).
>>
>> I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.
>>
>> When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.
>>
> 
> What I understand that happens in the case of clear_freed is that we are
> breaking a PDB/hugepage and descending, as the clear happened in the middle
> of the hugepage. With the specific reproducer I had to go through lengths
> of making sure that the original mapping landed on a hugepage-aligned
> address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
> the middle of it.

Yeah that is exactly the broken case I'm already working on.

In that case the clear should just unmap the whole hugepage and not allocate new PDs.

Re-mapping the two fragments before and after the unmapped area is supposed to allocate the new PDs.

Without that we run into tons of problems with userptrs and MMU notifiers.

I will CC you on the patches I have so far.

Regards,
Christian.

> 
> The other reproducer that I have required that I used REPLACE instead of
> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
> instead of MAP, so that explains that we can see this in production.
> 
> Regards.
> Cascardo.
> 
>> Regards,
>> Christian.
>>
>>>
>>> Regards.
>>> Cascardo.
>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> index 09037df76400..5bde36754607 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>>>   	while (!list_empty(&vm->freed)) {
>>>>>>   		mapping = list_first_entry(&vm->freed,
>>>>>>   			struct amdgpu_bo_va_mapping, list);
>>>>>> -		list_del(&mapping->list);
>>>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>>>>>   					   &sync, mapping->start, mapping->last,
>>>>>>   					   0, 0, 0, NULL, NULL, &f);
>>>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>   		if (r) {
>>>>>>   			dma_fence_put(f);
>>>>>>   			goto error_free;
>>>>>>   		}
>>>>>> +		list_del(&mapping->list);
>>>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>   	}
>>>>>>   	if (fence && f) {
>>>>>>
>>>>>
>>>>
>>

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Thadeu Lima de Souza Cascardo]

On Tue, Sep 15, 2026 at 03:56:00PM +0200, Christian König wrote:
> On 9/15/26 13:34, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
> >> On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
> >>> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
> >>>> On 9/15/26 10:33, Christian König wrote:
> >>>>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
> >>>>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
> >>>>>> the clearing of that mapping will not be attempted again. Later on, when
> >>>>>> an IB tries to read that mapping, the read succeeds, leading to a
> >>>>>> potential info leak, or even data corruption, if it attempts to write to
> >>>>>> it.
> >>>>>>
> >>>>>> If the mapping is left in the freed list, then clearing will be
> >>>>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
> >>>>>> submit the job or will succeed in clearing the mapping, preventing the
> >>>>>> invalid access.
> >>>>>
> >>>>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> >>>>>
> >>>>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
> >>>>
> >>>> Actually, how so? The SDMA update path has lots of allocations that happen
> >>>> at random points throughout the update process (think the SDMA IB being
> >>>> exhausted, then vm_funcs->update() will allocate a new one and can fail on
> >>>> that, or various fence allocations. Some paths allocate from the delayed IB
> >>>> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
> >>>>
> >>>> Aside from that, the more trivial failure path that I think Thadeu is also
> >>>> talking about here is that amdgpu_vm_ptes_update will call
> >>>> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
> >>>> allocations can fail. I'm not even sure if drm_exec can save this one
> >>>> because we need to be ready to drop locks and retry.
> >>>>
> >>>> I get that failing is unacceptable for the cases inside a pagefault handler
> >>>> (though I'm not sure if anything of what I mentioned applies here, pagefault
> >>>> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
> >>>> handler could still allocate PTs?) but for anything else, failure seems like
> >>>> a possibility and maybe we should think about handling it in some way.
> >>>>
> >>>> I'm not sure I see a problem in keeping BOs invalidated and backing off
> >>>> later, since new submissions should be rejected unless the VM manages to
> >>>> complete all updates successfully.
> >>>>
> >>>> Best,
> >>>> Natalie
> >>>>
> >>>
> >>> Thanks, Natalie, those are exactly the possible scenarios here:
> >>> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
> >>>
> >>> Christian,
> >>>
> >>> I have started this investigation once I observed the kmemleak that is
> >>> fixed by the other 3 patches. Natalie brings up the exact case where this
> >>> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
> >>> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
> >>
> >> Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
> >>> Here is a log showing up that something failed during the update:
> >>> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)
> >>
> >> That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.
> >>
> > 
> > Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
> > the failure here and leaves the mappings in the invalids list, which the
> > next CS will try to map.
> > 
> >>>
> >>> The investigation then led to a reproducer, which is able to trigger the
> >>> list corruption that is mentioned in patch 4.
> >>>
> >>> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
> >>> trace, that should clearly be where the failure has happened. Looking at
> >>> the function, then I noticed it ignores the return code and still removes
> >>> the mappings from the list. I was able to write a second reproducer that
> >>> triggers the failure, then releases some memory and demonstrate that
> >>> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
> >>> empty.
> >>
> >> Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.
> >>
> >> So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).
> >>
> >> I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.
> >>
> >> When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.
> >>
> > 
> > What I understand that happens in the case of clear_freed is that we are
> > breaking a PDB/hugepage and descending, as the clear happened in the middle
> > of the hugepage. With the specific reproducer I had to go through lengths
> > of making sure that the original mapping landed on a hugepage-aligned
> > address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
> > the middle of it.
> 
> Yeah that is exactly the broken case I'm already working on.
> 
> In that case the clear should just unmap the whole hugepage and not allocate new PDs.
> 

Agreed that we should also do this. Because my patchset does not prevent
the issue and if there is no relief on memory pressure, the failure will
end up in amdgpu_cs_ioctl, resulting in a submission failure.

In any case, given that other causes could still result in the similar
symptoms (memory leak, list corruption), would you consider applying this
patchset?

> Re-mapping the two fragments before and after the unmapped area is supposed to allocate the new PDs.
> 
> Without that we run into tons of problems with userptrs and MMU notifiers.
> 
> I will CC you on the patches I have so far.
> 

Please, do. I will be happy to test it and review it.

Thanks.
Cascardo.

> Regards,
> Christian.
> 
> > 
> > The other reproducer that I have required that I used REPLACE instead of
> > MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
> > amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
> > instead of MAP, so that explains that we can see this in production.
> > 
> > Regards.
> > Cascardo.
> > 
> >> Regards,
> >> Christian.
> >>
> >>>
> >>> Regards.
> >>> Cascardo.
> >>>
> >>>>>
> >>>>> Regards,
> >>>>> Christian.
> >>>>>
> >>>>>>
> >>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
> >>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>> index 09037df76400..5bde36754607 100644
> >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> >>>>>>   	while (!list_empty(&vm->freed)) {
> >>>>>>   		mapping = list_first_entry(&vm->freed,
> >>>>>>   			struct amdgpu_bo_va_mapping, list);
> >>>>>> -		list_del(&mapping->list);
> >>>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> >>>>>>   					   &sync, mapping->start, mapping->last,
> >>>>>>   					   0, 0, 0, NULL, NULL, &f);
> >>>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>>>   		if (r) {
> >>>>>>   			dma_fence_put(f);
> >>>>>>   			goto error_free;
> >>>>>>   		}
> >>>>>> +		list_del(&mapping->list);
> >>>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>>>   	}
> >>>>>>   	if (fence && f) {
> >>>>>>
> >>>>>
> >>>>
> >>
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Christian König]

On 9/23/26 15:56, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Sep 15, 2026 at 03:56:00PM +0200, Christian König wrote:
>> On 9/15/26 13:34, Thadeu Lima de Souza Cascardo wrote:
>>> On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
>>>> On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
>>>>> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
>>>>>> On 9/15/26 10:33, Christian König wrote:
>>>>>>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
>>>>>>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
>>>>>>>> the clearing of that mapping will not be attempted again. Later on, when
>>>>>>>> an IB tries to read that mapping, the read succeeds, leading to a
>>>>>>>> potential info leak, or even data corruption, if it attempts to write to
>>>>>>>> it.
>>>>>>>>
>>>>>>>> If the mapping is left in the freed list, then clearing will be
>>>>>>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
>>>>>>>> submit the job or will succeed in clearing the mapping, preventing the
>>>>>>>> invalid access.
>>>>>>>
>>>>>>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
>>>>>>>
>>>>>>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
>>>>>>
>>>>>> Actually, how so? The SDMA update path has lots of allocations that happen
>>>>>> at random points throughout the update process (think the SDMA IB being
>>>>>> exhausted, then vm_funcs->update() will allocate a new one and can fail on
>>>>>> that, or various fence allocations. Some paths allocate from the delayed IB
>>>>>> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
>>>>>>
>>>>>> Aside from that, the more trivial failure path that I think Thadeu is also
>>>>>> talking about here is that amdgpu_vm_ptes_update will call
>>>>>> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
>>>>>> allocations can fail. I'm not even sure if drm_exec can save this one
>>>>>> because we need to be ready to drop locks and retry.
>>>>>>
>>>>>> I get that failing is unacceptable for the cases inside a pagefault handler
>>>>>> (though I'm not sure if anything of what I mentioned applies here, pagefault
>>>>>> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
>>>>>> handler could still allocate PTs?) but for anything else, failure seems like
>>>>>> a possibility and maybe we should think about handling it in some way.
>>>>>>
>>>>>> I'm not sure I see a problem in keeping BOs invalidated and backing off
>>>>>> later, since new submissions should be rejected unless the VM manages to
>>>>>> complete all updates successfully.
>>>>>>
>>>>>> Best,
>>>>>> Natalie
>>>>>>
>>>>>
>>>>> Thanks, Natalie, those are exactly the possible scenarios here:
>>>>> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
>>>>>
>>>>> Christian,
>>>>>
>>>>> I have started this investigation once I observed the kmemleak that is
>>>>> fixed by the other 3 patches. Natalie brings up the exact case where this
>>>>> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
>>>>> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
>>>>
>>>> Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
>>>>> Here is a log showing up that something failed during the update:
>>>>> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)
>>>>
>>>> That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.
>>>>
>>>
>>> Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
>>> the failure here and leaves the mappings in the invalids list, which the
>>> next CS will try to map.
>>>
>>>>>
>>>>> The investigation then led to a reproducer, which is able to trigger the
>>>>> list corruption that is mentioned in patch 4.
>>>>>
>>>>> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
>>>>> trace, that should clearly be where the failure has happened. Looking at
>>>>> the function, then I noticed it ignores the return code and still removes
>>>>> the mappings from the list. I was able to write a second reproducer that
>>>>> triggers the failure, then releases some memory and demonstrate that
>>>>> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
>>>>> empty.
>>>>
>>>> Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.
>>>>
>>>> So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).
>>>>
>>>> I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.
>>>>
>>>> When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.
>>>>
>>>
>>> What I understand that happens in the case of clear_freed is that we are
>>> breaking a PDB/hugepage and descending, as the clear happened in the middle
>>> of the hugepage. With the specific reproducer I had to go through lengths
>>> of making sure that the original mapping landed on a hugepage-aligned
>>> address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
>>> the middle of it.
>>
>> Yeah that is exactly the broken case I'm already working on.
>>
>> In that case the clear should just unmap the whole hugepage and not allocate new PDs.
>>
> 
> Agreed that we should also do this. Because my patchset does not prevent
> the issue and if there is no relief on memory pressure, the failure will
> end up in amdgpu_cs_ioctl, resulting in a submission failure.
> 
> In any case, given that other causes could still result in the similar
> symptoms (memory leak, list corruption), would you consider applying this
> patchset?

No, the code changed by this patch here at least is actually fully correct.

Regards,
Christian.

> 
>> Re-mapping the two fragments before and after the unmapped area is supposed to allocate the new PDs.
>>
>> Without that we run into tons of problems with userptrs and MMU notifiers.
>>
>> I will CC you on the patches I have so far.
>>
> 
> Please, do. I will be happy to test it and review it.
> 
> Thanks.
> Cascardo.
> 
>> Regards,
>> Christian.
>>
>>>
>>> The other reproducer that I have required that I used REPLACE instead of
>>> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
>>> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
>>> instead of MAP, so that explains that we can see this in production.
>>>
>>> Regards.
>>> Cascardo.
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards.
>>>>> Cascardo.
>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>> index 09037df76400..5bde36754607 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>>>>>   	while (!list_empty(&vm->freed)) {
>>>>>>>>   		mapping = list_first_entry(&vm->freed,
>>>>>>>>   			struct amdgpu_bo_va_mapping, list);
>>>>>>>> -		list_del(&mapping->list);
>>>>>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>>>>>>>   					   &sync, mapping->start, mapping->last,
>>>>>>>>   					   0, 0, 0, NULL, NULL, &f);
>>>>>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>   		if (r) {
>>>>>>>>   			dma_fence_put(f);
>>>>>>>>   			goto error_free;
>>>>>>>>   		}
>>>>>>>> +		list_del(&mapping->list);
>>>>>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>   	}
>>>>>>>>   	if (fence && f) {
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Thadeu Lima de Souza Cascardo]

On Wed, Sep 23, 2026 at 03:58:27PM +0200, Christian König wrote:
> On 9/23/26 15:56, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Sep 15, 2026 at 03:56:00PM +0200, Christian König wrote:
> >> On 9/15/26 13:34, Thadeu Lima de Souza Cascardo wrote:
> >>> On Tue, Sep 15, 2026 at 01:22:48PM +0200, Christian König wrote:
> >>>> On 9/15/26 12:37, Thadeu Lima de Souza Cascardo wrote:
> >>>>> On Tue, Sep 15, 2026 at 10:53:43AM +0200, Natalie Vock wrote:
> >>>>>> On 9/15/26 10:33, Christian König wrote:
> >>>>>>> On 9/14/26 22:25, Thadeu Lima de Souza Cascardo wrote:
> >>>>>>>> If amdgpu_vm_update_range fails when called by amdgpu_vm_clear_freed,
> >>>>>>>> the clearing of that mapping will not be attempted again. Later on, when
> >>>>>>>> an IB tries to read that mapping, the read succeeds, leading to a
> >>>>>>>> potential info leak, or even data corruption, if it attempts to write to
> >>>>>>>> it.
> >>>>>>>>
> >>>>>>>> If the mapping is left in the freed list, then clearing will be
> >>>>>>>> attempted again during amdgpu_cs_ioctl, which will either fail and not
> >>>>>>>> submit the job or will succeed in clearing the mapping, preventing the
> >>>>>>>> invalid access.
> >>>>>>>
> >>>>>>> Same as I replied to oushinnyo <oushinnyo@163.com>, absolutely clear NAK to that.
> >>>>>>>
> >>>>>>> amdgpu_vm_update_range() can only fail when the device is hot removed and we don't care about clearing page tables in that case.
> >>>>>>
> >>>>>> Actually, how so? The SDMA update path has lots of allocations that happen
> >>>>>> at random points throughout the update process (think the SDMA IB being
> >>>>>> exhausted, then vm_funcs->update() will allocate a new one and can fail on
> >>>>>> that, or various fence allocations. Some paths allocate from the delayed IB
> >>>>>> pool which is just regular GFP_KERNEL, why couldn't these allocations fail?
> >>>>>>
> >>>>>> Aside from that, the more trivial failure path that I think Thadeu is also
> >>>>>> talking about here is that amdgpu_vm_ptes_update will call
> >>>>>> amdgpu_vm_pt_alloc (so that happens in the middle of VM updates) and these
> >>>>>> allocations can fail. I'm not even sure if drm_exec can save this one
> >>>>>> because we need to be ready to drop locks and retry.
> >>>>>>
> >>>>>> I get that failing is unacceptable for the cases inside a pagefault handler
> >>>>>> (though I'm not sure if anything of what I mentioned applies here, pagefault
> >>>>>> VM updates are immediate so they get GFP_ATOMIC, but maybe the pagefault
> >>>>>> handler could still allocate PTs?) but for anything else, failure seems like
> >>>>>> a possibility and maybe we should think about handling it in some way.
> >>>>>>
> >>>>>> I'm not sure I see a problem in keeping BOs invalidated and backing off
> >>>>>> later, since new submissions should be rejected unless the VM manages to
> >>>>>> complete all updates successfully.
> >>>>>>
> >>>>>> Best,
> >>>>>> Natalie
> >>>>>>
> >>>>>
> >>>>> Thanks, Natalie, those are exactly the possible scenarios here:
> >>>>> amdgpu_job_alloc and amdgpu_vm_pt_alloc might fail.
> >>>>>
> >>>>> Christian,
> >>>>>
> >>>>> I have started this investigation once I observed the kmemleak that is
> >>>>> fixed by the other 3 patches. Natalie brings up the exact case where this
> >>>>> might happen: amdgpu_vm_pt_alloc fails as all other VRAM BOs are locked as
> >>>>> they are either ALWAYS_VALID BOs or PT BOs, thus sharing the same dma_resv.
> >>>>
> >>>> Yeah and on a clear there is never a single page table allocated. What could be is that temporary memory allocations fail, but we need to make sure that never happens by using the emergency reserves and/or pools.
> >>>>> Here is a log showing up that something failed during the update:
> >>>>> [ 3839.556311] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-12)
> >>>>
> >>>> That is failed memory allocation while trying to map something which we also ignore and delegate to the next CS.
> >>>>
> >>>
> >>> Yep, that is a possibility as well and amdgpu_vm_bo_update is not ignoring
> >>> the failure here and leaves the mappings in the invalids list, which the
> >>> next CS will try to map.
> >>>
> >>>>>
> >>>>> The investigation then led to a reproducer, which is able to trigger the
> >>>>> list corruption that is mentioned in patch 4.
> >>>>>
> >>>>> And given the kmemleak stack trace shows amdgpu_vm_clear_freed in the stack
> >>>>> trace, that should clearly be where the failure has happened. Looking at
> >>>>> the function, then I noticed it ignores the return code and still removes
> >>>>> the mappings from the list. I was able to write a second reproducer that
> >>>>> triggers the failure, then releases some memory and demonstrate that
> >>>>> amdgpu_cs_ioctl does not clear the freed page tables as the freed list is
> >>>>> empty.
> >>>>
> >>>> Unmap/clear operations are also used in MMU notifiers who can't give an error back to higher levels but rather need to unmap the area no matter what.
> >>>>
> >>>> So basically that is an operation which can never fail in the first place (except for device is completely gone and then we don't care any more).
> >>>>
> >>>> I'm about to split the map/unmap into separate functions to make it clear that unmap operations can't fail.
> >>>>
> >>>> When you have a reproducer and can pinpoint where exactly the memory allocation fails that would be rather helpful here.
> >>>>
> >>>
> >>> What I understand that happens in the case of clear_freed is that we are
> >>> breaking a PDB/hugepage and descending, as the clear happened in the middle
> >>> of the hugepage. With the specific reproducer I had to go through lengths
> >>> of making sure that the original mapping landed on a hugepage-aligned
> >>> address, then used CLEAR/amdgpu_vm_clear_mappings to unmap a single page in
> >>> the middle of it.
> >>
> >> Yeah that is exactly the broken case I'm already working on.
> >>
> >> In that case the clear should just unmap the whole hugepage and not allocate new PDs.
> >>
> > 
> > Agreed that we should also do this. Because my patchset does not prevent
> > the issue and if there is no relief on memory pressure, the failure will
> > end up in amdgpu_cs_ioctl, resulting in a submission failure.
> > 
> > In any case, given that other causes could still result in the similar
> > symptoms (memory leak, list corruption), would you consider applying this
> > patchset?
> 
> No, the code changed by this patch here at least is actually fully correct.
> 
> Regards,
> Christian.
> 

What about the other 3 patches? If there is less controversy around them, I
can submit them as a new patchset version.

Thanks.
Cascardo.

> > 
> >> Re-mapping the two fragments before and after the unmapped area is supposed to allocate the new PDs.
> >>
> >> Without that we run into tons of problems with userptrs and MMU notifiers.
> >>
> >> I will CC you on the patches I have so far.
> >>
> > 
> > Please, do. I will be happy to test it and review it.
> > 
> > Thanks.
> > Cascardo.
> > 
> >> Regards,
> >> Christian.
> >>
> >>>
> >>> The other reproducer that I have required that I used REPLACE instead of
> >>> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
> >>> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
> >>> instead of MAP, so that explains that we can see this in production.
> >>>
> >>> Regards.
> >>> Cascardo.
> >>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>>
> >>>>> Regards.
> >>>>> Cascardo.
> >>>>>
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Christian.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>>>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> >>>>>>>> ---
> >>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
> >>>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>>>> index 09037df76400..5bde36754607 100644
> >>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >>>>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> >>>>>>>>   	while (!list_empty(&vm->freed)) {
> >>>>>>>>   		mapping = list_first_entry(&vm->freed,
> >>>>>>>>   			struct amdgpu_bo_va_mapping, list);
> >>>>>>>> -		list_del(&mapping->list);
> >>>>>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> >>>>>>>>   					   &sync, mapping->start, mapping->last,
> >>>>>>>>   					   0, 0, 0, NULL, NULL, &f);
> >>>>>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>>>>>   		if (r) {
> >>>>>>>>   			dma_fence_put(f);
> >>>>>>>>   			goto error_free;
> >>>>>>>>   		}
> >>>>>>>> +		list_del(&mapping->list);
> >>>>>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >>>>>>>>   	}
> >>>>>>>>   	if (fence && f) {
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
> 

Re: [PATCH 3/4] drm/amdgpu: don't free mapping when amdgpu_vm_update_range fails

[Christian König]

On 9/23/26 16:18, Thadeu Lima de Souza Cascardo wrote:
...
>>
> 
> What about the other 3 patches? If there is less controversy around them, I
> can submit them as a new patchset version.

Patch #1 is simply not the right direction we need, but see comments on patch #4 as well.

Patch #2 is a good cleanup, that could potentially be applied immediately.

Patch #3 is what I'm already working on. We actually need to change the code so that failures simply can't occur (except for device hot plug).

If any failure occurs here the only real doable recovery is to kill all ongoing work and disconnect the device.

Patch #4 is an absolute no-go, committing partial page table update can cause tons of problems.

Essentially we need to rework the code so that page table allocation happens before we kick of any HW update.

I already discussed that with Natalie quite a while ago on our weekly meeting, you are basically the third or forth person to stumble over that problem already.

Regards,
Christian.

> 
> Thanks.
> Cascardo.
> 
>>>
>>>> Re-mapping the two fragments before and after the unmapped area is supposed to allocate the new PDs.
>>>>
>>>> Without that we run into tons of problems with userptrs and MMU notifiers.
>>>>
>>>> I will CC you on the patches I have so far.
>>>>
>>>
>>> Please, do. I will be happy to test it and review it.
>>>
>>> Thanks.
>>> Cascardo.
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> The other reproducer that I have required that I used REPLACE instead of
>>>>> MAP in order to trigger the tlb_flush_waitlist issue. REPLACE does call
>>>>> amdgpu_vm_clear_mappings too. Notice that RADV Mesa driver uses REPLACE,
>>>>> instead of MAP, so that explains that we can see this in production.
>>>>>
>>>>> Regards.
>>>>> Cascardo.
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Regards.
>>>>>>> Cascardo.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
>>>>>>>>>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++--
>>>>>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> index 09037df76400..5bde36754607 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>>>>> @@ -1594,16 +1594,16 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>>>>>>>>>   	while (!list_empty(&vm->freed)) {
>>>>>>>>>>   		mapping = list_first_entry(&vm->freed,
>>>>>>>>>>   			struct amdgpu_bo_va_mapping, list);
>>>>>>>>>> -		list_del(&mapping->list);
>>>>>>>>>>   		r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>>>>>>>>>>   					   &sync, mapping->start, mapping->last,
>>>>>>>>>>   					   0, 0, 0, NULL, NULL, &f);
>>>>>>>>>> -		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>>>   		if (r) {
>>>>>>>>>>   			dma_fence_put(f);
>>>>>>>>>>   			goto error_free;
>>>>>>>>>>   		}
>>>>>>>>>> +		list_del(&mapping->list);
>>>>>>>>>> +		amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>>>>>>>>   	}
>>>>>>>>>>   	if (fence && f) {
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>