Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
On Thu, Sep 19, 2019 at 3:03 AM Koenig, Christian wrote: > > Am 18.09.19 um 21:05 schrieb Navid Emamdoost: > > In acp_hw_init there are some allocations that needs to be released in > > case of failure: > > > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > > 2- all of those allocations should be released if pm_genpd_add_device > > fails. > > > > v2: moved the released into goto error handlings > > > > Signed-off-by: Navid Emamdoost > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 + > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > index eba42c752bca..c0db75b71859 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char > > *device_name, int r) > >*/ > > static int acp_hw_init(void *handle) > > { > > - int r, i; > > + int r, i, ret; > > uint64_t acp_base; > > u32 val = 0; > > u32 count = 0; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > > GFP_KERNEL); > > > > - if (adev->acp.acp_cell == NULL) > > - return -ENOMEM; > > + if (adev->acp.acp_cell == NULL) { > > + ret = -ENOMEM; > > + goto out1; > > + } > > > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > > if (adev->acp.acp_res == NULL) { > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto out2; > > } > > > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > > if (i2s_pdata == NULL) { > > - kfree(adev->acp.acp_res); > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto out3; > > } > > > > switch (adev->asic_type) { > > @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) > > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > > if (r) { > > dev_err(dev, "Failed to add dev to genpd\n"); > > - return r; > > + ret = r; > > + goto out4; > > } > > } > > > > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > > return 0; > > + > > +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. > I fixed this by just using failure label. > Christian. > > > + return ret; > > } > > > > /** > In addition to previous cases, I covered 3 more error handling cases that seemed need to goto failure. One where mfd_add_hotplug_devices fails and the other two cases where time out values expire. -- Navid.
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
> v2: moved the released into goto error handlings A better version comment should be moved below the triple dashes. Will the tag “Fixes” be added? > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +out4: > + kfree(i2s_pdata); > +out3: > + kfree(adev->acp.acp_res); > +out2: > + kfree(adev->acp.acp_cell); > +out1: > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** I suggest to reconsider the label selection according to the Linux coding style. Regards, Markus
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
Am 19.09.19 um 16:28 schrieb Sven Van Asbroeck: > Hi Christian, > > On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian > 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
Hi Christian, On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian 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: 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
Am 18.09.19 um 21:05 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if pm_genpd_add_device > fails. > > v2: moved the released into goto error handlings > > Signed-off-by: Navid Emamdoost > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 + > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..c0db75b71859 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char > *device_name, int r) >*/ > static int acp_hw_init(void *handle) > { > - int r, i; > + int r, i, ret; > uint64_t acp_base; > u32 val = 0; > u32 count = 0; > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + ret = -ENOMEM; > + goto out1; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out2; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out3; > } > > switch (adev->asic_type) { > @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + ret = r; > + goto out4; > } > } > > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +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. Christian. > + return ret; > } > > /**
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
On Wed, Sep 18, 2019 at 3:09 PM Navid Emamdoost wrote: > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out3; > } I don't see a corresponding kfree() for i2s_pdata in acp_hw_fini(). Could this be a memory leak?
[PATCH v2] drm/amdgpu: fix multiple memory leaks
In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if pm_genpd_add_device fails. v2: moved the released into goto error handlings Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 + 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..c0db75b71859 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) */ static int acp_hw_init(void *handle) { - int r, i; + int r, i, ret; uint64_t acp_base; u32 val = 0; u32 count = 0; @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) - return -ENOMEM; + if (adev->acp.acp_cell == NULL) { + ret = -ENOMEM; + goto out1; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto out2; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { - kfree(adev->acp.acp_res); - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto out3; } switch (adev->asic_type) { @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); - return r; + ret = r; + goto out4; } } @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); return 0; + +out4: + kfree(i2s_pdata); +out3: + kfree(adev->acp.acp_res); +out2: + kfree(adev->acp.acp_cell); +out1: + kfree(adev->acp.acp_genpd); + return ret; } /** -- 2.17.1