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

[sashiko-bot@kernel.org]

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