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

[Christian König]

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
>