[PATCH] drm/amdgpu: Add NULL check for 'bo_va' in update_bo_mapping

[Christian König]

Am 11.04.25 um 17:00 schrieb Srinivasan Shanmugam:
> This change adds a check to ensure that 'bo_va' is not null before
> dereferencing it. If 'bo_va' is null, the function returns early,
> preventing any potential crashes or undefined behavior
>
> Fixes the below:
> 	drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:139 amdgpu_gem_update_bo_mapping()
> 	error: we previously assumed 'bo_va' could be null (see line 124)
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>     115 static void
>     116 amdgpu_gem_update_bo_mapping(struct drm_file *filp,
>     117                              struct amdgpu_bo_va *bo_va,
>     118                              uint32_t operation,
>     119                              uint64_t point,
>     120                              struct dma_fence *fence,
>     121                              struct drm_syncobj *syncobj,
>     122                              struct dma_fence_chain *chain)
>     123 {
>     124         struct amdgpu_bo *bo = bo_va ? bo_va->base.bo : NULL;
>                                   ^^^^^^^^^^ If bo_va is NULL then bo is also NULL
>
> 	...
>     135         case AMDGPU_VA_OP_REPLACE:
>     136                 if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv))
>                             ^^
>
>     137                         last_update = vm->last_update;
>     138                 else
> --> 139                         last_update = bo_va->last_pt_update;
>                                               ^^^^^ This pointer is dereferenced without being checked.
>
>     140                 break;
>
> Fixes: 16856d135622 ("drm/amdgpu: update userqueue BOs and PDs")
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Reported-by: Dan Carpenter <dan.carpenter at linaro.org>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index c1d8cee7894b..247fbd014b7f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -134,8 +134,10 @@ amdgpu_gem_update_bo_mapping(struct drm_file *filp,
>  	case AMDGPU_VA_OP_REPLACE:
>  		if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv))
>  			last_update = vm->last_update;
> -		else
> +		else if (bo_va)
>  			last_update = bo_va->last_pt_update;
> +		else
> +			return;

That warning is a false positive. The bo_va is only NULL on clear operations.

I think the whole switch/case here is superfluous. What we should do instead is update the fence in amdgpu_gem_va_update_vm() instead:

        if (operation == AMDGPU_VA_OP_MAP ||
            operation == AMDGPU_VA_OP_REPLACE) {
                r = amdgpu_vm_bo_update(adev, bo_va, false);
                if (r)
                        goto error;
E.g. here.

        }

Regards,
Christian.

>  		break;
>  	case AMDGPU_VA_OP_UNMAP:
>  	case AMDGPU_VA_OP_CLEAR: