lkml.org 
[lkml]   [2019]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] drm/amdgpu: fix multiple memory leaks
    Date
    Am 19.09.19 um 16:28 schrieb Sven Van Asbroeck:
    > Hi Christian,
    >
    > On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian
    > <Christian.Koenig@amd.com> wrote:
    >>> +out4:
    >>> + kfree(i2s_pdata);
    >>> +out3:
    >>> + kfree(adev->acp.acp_res);
    >>> +out2:
    >>> + kfree(adev->acp.acp_cell);
    >>> +out1:
    >>> + kfree(adev->acp.acp_genpd);
    >> kfree on a NULL pointer is harmless, so a single error label should be
    >> sufficient.
    > That is true, but I notice that the adev structure comes from outside this
    > driver:

    adev is a very integral part of the driver and always zero initialized:

    adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);

    Regards,
    Christian.

    >
    > static int acp_hw_init(void *handle)
    > {
    > ...
    > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
    > ...
    > }
    >
    > As far as I can tell, the init() does not explicitly set these to NULL:
    > adev->acp.acp_res
    > adev->acp.acp_cell
    > adev->acp.acp_genpd
    >
    > I am assuming that core code sets these to NULL, before calling
    > acp_hw_init(). But is that guaranteed or simply a happy accident?
    > Ie. if they are NULL today, are they likely to remain NULL tomorrow?
    >
    > Because calling kfree() on a stale pointer would be, well
    > not good. Especially not on an error path, which typically
    > does not receive much testing, if any.
    >
    > My apologies if I have misinterpreted this, I am not familiar with
    > this code base.
    >
    > Sven

    \
     
     \ /
      Last update: 2019-09-19 18:50    [W:4.708 / U:0.356 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site