Message-ID: <CAEKBCKNesZRZx-PafM5+dfuLSgOQb7_0h5DvTT+YcfBmvj1ovQ@mail.gmail.com>
Date: Mon, 23 Sep 2024 22:33:49 +0545
From: Dipendra Khadka <kdipendra88@...il.com>
To: Christian König <christian.koenig@....com>
Cc: Felix.Kuehling@....com, alexander.deucher@....com, Xinhui.Pan@....com, 
	airlied@...il.com, daniel@...ll.ch, amd-gfx@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: drivers/gpu/drm/amd/amdgpu: Fix null pointer
 deference in amdkfd_fence_get_timeline_name

Hi Christian,

On Mon, 23 Sept 2024 at 18:57, Christian König <christian.koenig@....com> wrote:
>
> Am 21.09.24 um 06:25 schrieb Dipendra Khadka:
> > On Sat, 21 Sept 2024 at 00:43, Christian König <christian.koenig@....com> wrote:
> >> Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
> >>> On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@....com> 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.
>

I am learning driver development and was not sure how it works. Now, I got it.

> 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.
>

Thank you so much for the insight and your time. I will make sure to
see the kernel doc as well as try to think more.

Best Regards,
Dipendra

> 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@...il.com>
> >>>>> ---
> >>>>>     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
>

Powered by blists - more mailing lists