[PATCH v1 1/2] drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini

[PATCH v1 1/2] drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini

[Pierre-Eric Pelloux-Prayer]

The commit referenced below restarts the CS if the validation is
still in progress. When debug_vm is enabled, all BOs from the CS
are invalidated so we will hit an infinite loop.

To avoid that, defer BO invalidation to amdgpu_cs_parser_fini.

Fixes: 83ac717d7837 ("drm/amdgpu: restart the CS if some parts of the VM are still invalidated")
Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 30 ++++++++++++++------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d63fbc7c568d..7e75c536dab3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1180,19 +1180,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
 	}
 
-	if (adev->debug_vm) {
-		/* Invalidate all BOs to test for userspace bugs */
-		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
-			struct amdgpu_bo *bo = e->bo;
-
-			/* ignore duplicates */
-			if (!bo)
-				continue;
-
-			amdgpu_vm_bo_invalidate(bo, false);
-		}
-	}
-
 	return 0;
 }
 
@@ -1382,6 +1369,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 /* Cleanup the parser structure */
 static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
 {
+	struct amdgpu_device *adev = parser->adev;
+	struct amdgpu_bo_list_entry *e;
 	unsigned int i;
 
 	amdgpu_sync_free(&parser->sync);
@@ -1397,8 +1386,21 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
 
 	if (parser->ctx)
 		amdgpu_ctx_put(parser->ctx);
-	if (parser->bo_list)
+	if (parser->bo_list) {
+		if (adev->debug_vm) {
+			/* Invalidate all BOs to test for userspace bugs */
+			amdgpu_bo_list_for_each_entry(e, parser->bo_list) {
+				struct amdgpu_bo *bo = e->bo;
+
+				/* ignore duplicates */
+				if (!bo)
+					continue;
+
+				amdgpu_vm_bo_invalidate(bo, false);
+			}
+		}
 		amdgpu_bo_list_put(parser->bo_list);
+	}
 
 	for (i = 0; i < parser->nchunks; i++)
 		kvfree(parser->chunks[i].kdata);
-- 
2.43.0

[PATCH v1 2/2] drm/amdgpu: remove left-over variable

[Pierre-Eric Pelloux-Prayer]

887db1e49a73 ("drm/amdgpu: Merge debug module parameters") removed amdgpu_vm_debug
handling but this variable was forgotten.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e2d4be3c111d..3b78503ec78e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -182,7 +182,6 @@ extern int amdgpu_vm_size;
 extern int amdgpu_vm_block_size;
 extern int amdgpu_vm_fragment_size;
 extern int amdgpu_vm_fault_stop;
-extern int amdgpu_vm_debug;
 extern int amdgpu_vm_update_mode;
 extern int amdgpu_exp_hw_support;
 extern int amdgpu_dc;
-- 
2.43.0

Re: [PATCH v1 1/2] drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving `amdgpu_vm_bo_invalidate()` to `amdgpu_cs_parser_fini()` accesses BO structures without holding their `dma_resv` reservation locks, leading to data races, potential use-after-free vulnerabilities, and lockdep assertions.
--

commit 130b94ccdb3ef241f2576b6d31544c37bb57192b
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini

The commit restarts the CS if the validation is still in progress, but when
debug_vm is enabled, all BOs are invalidated, causing an infinite loop. This
patch moves the BO invalidation to amdgpu_cs_parser_fini to avoid that.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index d63fbc7c568de..7e75c536dab3d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c

[ ... ]

> @@ -1382,6 +1369,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  /* Cleanup the parser structure */
>  static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  {
> +	struct amdgpu_device *adev = parser->adev;
> +	struct amdgpu_bo_list_entry *e;
>  	unsigned int i;
>  
>  	amdgpu_sync_free(&parser->sync);

[ ... ]

> @@ -1397,8 +1386,21 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  
>  	if (parser->ctx)
>  		amdgpu_ctx_put(parser->ctx);
> -	if (parser->bo_list)
> +	if (parser->bo_list) {
> +		if (adev->debug_vm) {
> +			/* Invalidate all BOs to test for userspace bugs */
> +			amdgpu_bo_list_for_each_entry(e, parser->bo_list) {
> +				struct amdgpu_bo *bo = e->bo;
> +
> +				/* ignore duplicates */
> +				if (!bo)
> +					continue;
> +
> +				amdgpu_vm_bo_invalidate(bo, false);

[Severity: High]
Does moving this invalidation here result in accessing the BO structures
without their reservation locks?

Earlier in amdgpu_cs_parser_fini(), drm_exec_fini() is called to drop
all dma_resv locks acquired during parsing:

amdgpu_cs_parser_fini() {
...
    amdgpu_sync_free(&parser->sync);
    drm_exec_fini(&parser->exec);
...

Since amdgpu_vm_bo_invalidate() iterates over the bo->vm_bo list, which
is protected by the BO's reservation lock, could this unlocked access
lead to a data race or a use-after-free if it races with concurrent
mapping operations like amdgpu_vm_bo_del()?

Additionally, if any of the BOs are always-valid, this will eventually call
amdgpu_vm_bo_moved(), which triggers an amdgpu_vm_assert_locked(vm) lockdep
assertion since the root PD lock was already dropped.

> +			}
> +		}
>  		amdgpu_bo_list_put(parser->bo_list);
> +	}
>  
>  	for (i = 0; i < parser->nchunks; i++)
>  		kvfree(parser->chunks[i].kdata);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622092859.3600-1-pierre-eric.pelloux-prayer@amd.com?part=1

Re: [PATCH v1 1/2] drm/amdgpu: move debug_vm handling to amdgpu_cs_parser_fini

[Christian König]

On 6/22/26 11:28, Pierre-Eric Pelloux-Prayer wrote:
> The commit referenced below restarts the CS if the validation is
> still in progress. When debug_vm is enabled, all BOs from the CS
> are invalidated so we will hit an infinite loop.
> 
> To avoid that, defer BO invalidation to amdgpu_cs_parser_fini.
> 
> Fixes: 83ac717d7837 ("drm/amdgpu: restart the CS if some parts of the VM are still invalidated")
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com> for both patches.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 30 ++++++++++++++------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index d63fbc7c568d..7e75c536dab3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1180,19 +1180,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  		job->vm_pd_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>  	}
>  
> -	if (adev->debug_vm) {
> -		/* Invalidate all BOs to test for userspace bugs */
> -		amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> -			struct amdgpu_bo *bo = e->bo;
> -
> -			/* ignore duplicates */
> -			if (!bo)
> -				continue;
> -
> -			amdgpu_vm_bo_invalidate(bo, false);
> -		}
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1382,6 +1369,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  /* Cleanup the parser structure */
>  static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  {
> +	struct amdgpu_device *adev = parser->adev;
> +	struct amdgpu_bo_list_entry *e;
>  	unsigned int i;
>  
>  	amdgpu_sync_free(&parser->sync);
> @@ -1397,8 +1386,21 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  
>  	if (parser->ctx)
>  		amdgpu_ctx_put(parser->ctx);
> -	if (parser->bo_list)
> +	if (parser->bo_list) {
> +		if (adev->debug_vm) {
> +			/* Invalidate all BOs to test for userspace bugs */
> +			amdgpu_bo_list_for_each_entry(e, parser->bo_list) {
> +				struct amdgpu_bo *bo = e->bo;
> +
> +				/* ignore duplicates */
> +				if (!bo)
> +					continue;
> +
> +				amdgpu_vm_bo_invalidate(bo, false);
> +			}
> +		}
>  		amdgpu_bo_list_put(parser->bo_list);
> +	}
>  
>  	for (i = 0; i < parser->nchunks; i++)
>  		kvfree(parser->chunks[i].kdata);