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

[Dan Carpenter]

On Tue, Apr 22, 2025 at 03:34:58PM +0200, Christian König wrote:
> Am 22.04.25 um 15:17 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
> 
> That commit message doesn't reflect the changes any more.
> 
> >
> > 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;
> 
> Please completely drop that. This conclusion is actually incorrect.
> 
> BO might be NULL here because bo_va->base.bo is NULL and *not* because bo_va is NULL.
> 
> @Dan your script seems to reports false positives here.
> 

I mean my analysis was only based on only looking at the function itself
without looking at the caller.

It turns out that it's a false positve because "bo_va" is only NULL when
the operation is AMDGPU_VA_OP_CLEAR.  You need to look at the caller and
also where fpriv->prt_va is set in amdgpu_driver_open_kms().  It's a bit
too complicated for Smatch to do this level of analysis.

Anyway, yes, please don't silence static checker false positives, just
ignore them.

regards,
dan carpenter