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(>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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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(>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;
> }
>
> /**
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
