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
