Am 21.09.24 um 06:25 schrieb Dipendra Khadka:
On Sat, 21 Sept 2024 at 00:43, Christian König <christian.koenig@xxxxxxx> wrote:
Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@xxxxxxx> wrote:
Am 20.09.24 um 11:09 schrieb Dipendra Khadka:
'''
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:108:9: error: Null pointer dereference: fence [nullPointer]
return fence->timeline_name;
^
'''
The method to_amdgpu_amdkfd_fence can return NULL incase of empty f
or f->ops != &amdkfd_fence_ops.Hence, check has been added .
If fence is null , then null is returned.
Well NAK, completely nonsense. Calling the function with a NULL fence is
illegal.
Thanks for enlightening me .
Well sorry to be so direct, but what the heck did you tried to do here?
Hi Christian,
cppcheck reported null pointer dereference in the line " return
fence->timeline_name;" in the function "static const char
*amdkfd_fence_get_timeline_name(struct dma_fence *f)".
In the function , we are getting the value of fence like this :
"struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);"
When I went through the function " to_amdgpu_amdkfd_fence" whose definition is :
'''
struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f)
{
struct amdgpu_amdkfd_fence *fence;
if (!f)
return NULL;
fence = container_of(f, struct amdgpu_amdkfd_fence, base);
if (f->ops == &amdkfd_fence_ops)
return fence;
return NULL;
}
'''
Here, the function to_amdgpu_amdkfd_fence can return NULL when f is
empty or f->ops != &amdkfd_fence_ops .So the fence in function
"amdkfd_fence_get_timeline_name" can be NULL.
Hence , I thought dereferencing NULL fence like "return
fence->timeline_name" may result in the runtime crashing. So, I
proposed this fix. Sorry, I was not aware about the behaviour of the
fence.
I am interested in the development and tried to fix this .
Well it's in general a good idea that you looked into this, but you
should have put more thoughts into it.
That the fence can't be NULL is just implicit when you take a closer
look at the code.
amdkfd_fence_get_timeline_name() is only called through the pointer in
amdkfd_fence_ops. This makes the condition "f->ops == &amdkfd_fence_ops"
always true inside the function.
The only other possibility is that the f parameter is NULL, but that in
turn is impossible because the function is called like
f->ops->get_timeline_name(f) and so the caller would have crashed even
before entering the function.
And finally you didn't looked at the documentation. The kerneldoc for
get_timeline_name clearly states that the callback is mandatory and
therefore can't return NULL.
So to sum it up you suggested something which is not only unnecessary,
but results in documented illegal behavior.
The C language unfortunately doesn't have the necessary annotation
possibilities that a function can't return a NULL string (at least as
far as I know). So cppcheck can't know any of this.
Please don't trust the automated tool to much and put a bit more time
into patches like this.
Regards,
Christian.
I mean that is broken on so many different levels that I can't
understand why somebody is suggesting something like that.
Regards,
Christian.
Regards,
Christian.
Signed-off-by: Dipendra Khadka <kdipendra88@xxxxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
index 1ef758ac5076..2313babcc944 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
@@ -105,6 +105,9 @@ static const char *amdkfd_fence_get_timeline_name(struct dma_fence *f)
{
struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
+ if (!fence)
+ return NULL;
+
return fence->timeline_name;
}
Regards,
Dipendra Khadka
Regards,
Dipendra Khadka
