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

[Dan Carpenter]

On Wed, Apr 23, 2025 at 02:52:22PM +0200, Christian König wrote:
> 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,

It's not saying it's NULL, it's saying it was checked for NULL.  It's a
check followed by a dereference without checking.  If I add some debug
code:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index f03fc3cf4d50..2ff18a2e2e4c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -112,6 +112,7 @@ amdgpu_gem_update_timeline_node(struct drm_file *filp,
 	return 0;
 }
 
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
 static void
 amdgpu_gem_update_bo_mapping(struct drm_file *filp,
 			     struct amdgpu_bo_va *bo_va,
@@ -133,10 +134,13 @@ amdgpu_gem_update_bo_mapping(struct drm_file *filp,
 	switch (operation) {
 	case AMDGPU_VA_OP_MAP:
 	case AMDGPU_VA_OP_REPLACE:
-		if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv))
+		if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv)) {
+			__smatch_implied((unsigned long)bo_va);
 			last_update = vm->last_update;
-		else
+		} else {
+			__smatch_implied((unsigned long)bo_va);
 			last_update = bo_va->last_pt_update;
+		}
 		break;
 	case AMDGPU_VA_OP_UNMAP:
 	case AMDGPU_VA_OP_CLEAR:

The output is:

drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:138 amdgpu_gem_update_bo_mapping() implied: bo_va = '4096-18446744073709547520'
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:141 amdgpu_gem_update_bo_mapping() implied: bo_va = '0,4096-18446744073709547520'

Because if "bo" is non-NULL then "bo_va" must be non-NULL as well.  But
on the else side it could NULL or it could be valid.

regards,
dan carpenter