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
