On 4/22/25 16:15, Dan Carpenter wrote:
> 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.
Yeah that is true but I think that is only halve of the story, the logic in the
analyzer looks flawed to me.
See what is reported here is this case:
if (A) B=A->B else B=NULL;
...
if (!B) x=A->other_member;
The analyzer concludes that since A was checked before and when B is set to
NULL then A must also be NULL in the second usage, but that is incorrect.
Correct would be if B is NULL then it might be because A is NULL, but it
doesn't have to be.
I would double check the Smatch logic,
Regards,
Christian.
>
> Anyway, yes, please don't silence static checker false positives, just
> ignore them.
>
> regards,
> dan carpenter
>
