| Message ID | 20191225131715.3527-1-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: remove duplicated kfree in v3d_submit_cl_ioctl | expand |
> v3d_submit_cl_ioctl call kfree() with variable 'bin' twice. I would prefer a wording like “kfree() was called for the same variable twice within an if branch.”. > Fix it by removing the latter one. I find the wording “Delete a duplicate function call.” more appropriate. Please add the tag “Fixes” to your change description. Regards, Markus
On 2019/12/29 4:45, Markus Elfring wrote: >> v3d_submit_cl_ioctl call kfree() with variable 'bin' twice. > > I would prefer a wording like “kfree() was called for the same variable twice > within an if branch.”. > > >> Fix it by removing the latter one. > > I find the wording “Delete a duplicate function call.” more appropriate. > Thank you for your advise, I'll make changes in V2 patch. > Please add the tag “Fixes” to your change description. I got the results from "git blame": git blame -L 570,575 drivers/gpu/drm/v3d/v3d_gem.c a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 570) if (ret) { 0d352a3a8a1f2 (Iago Toral Quiroga 2019-09-16 09:11:25 +0200 571) kfree(bin); a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 572) v3d_job_put(&render->base); 29cd13cfd7624 (Navid Emamdoost 2019-10-21 13:52:49 -0500 573) kfree(bin); a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 574) return ret; a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 575) } The first kfree belong to the patch 0d352a3a8a1f2 : commit 0d352a3a8a1f26168d09f7073e61bb4b328e3bb9 Author: Iago Toral Quiroga <itoral@igalia.com> Date: Mon Sep 16 09:11:25 2019 +0200 drm/v3d: don't leak bin job if v3d_job_init fails. If the initialization of the job fails we need to kfree() it before returning. Signed-off-by: Iago Toral Quiroga <itoral@igalia.com> Signed-off-by: Eric Anholt <eric@anholt.net> Link: https://patchwork.freedesktop.org/patch/msgid/20190916071125.5255-1-itoral@igalia.com Fixes: a783a09ee76d ("drm/v3d: Refactor job management.") Reviewed-by: Eric Anholt <eric@anholt.net> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 5d80507b539b..fb32cda18ffe 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -563,6 +563,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, ret = v3d_job_init(v3d, file_priv, &bin->base, v3d_job_free, args->in_sync_bcl); if (ret) { + kfree(bin); v3d_job_put(&render->base); return ret; } And the second belong to 29cd13cfd7624: commit 29cd13cfd7624726d9e6becbae9aa419ef35af7f Author: Navid Emamdoost <navid.emamdoost@gmail.com> Date: Mon Oct 21 13:52:49 2019 -0500 drm/v3d: Fix memory leak in v3d_submit_cl_ioctl In the impelementation of v3d_submit_cl_ioctl() there are two memory leaks. One is when allocation for bin fails, and the other is when bin initialization fails. If kcalloc fails to allocate memory for bin then render->base should be put. Also, if v3d_job_init() fails to initialize bin->base then allocated memory for bin should be released. Fixes: a783a09ee76d ("drm/v3d: Refactor job management.") Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20191021185250.26130-1-navid.emamdoost@gmail.com diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 5d80507b539b..19c092d75266 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -557,13 +557,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, if (args->bcl_start != args->bcl_end) { bin = kcalloc(1, sizeof(*bin), GFP_KERNEL); - if (!bin) + if (!bin) { + v3d_job_put(&render->base); return -ENOMEM; + } ret = v3d_job_init(v3d, file_priv, &bin->base, v3d_job_free, args->in_sync_bcl); if (ret) { v3d_job_put(&render->base); + kfree(bin); return ret; } It seems the two patches fix the same memory leak, but I have no idea how thet get together without conflict. Thanks Yu Kuai
>> Please add the tag “Fixes” to your change description. > > I got the results from "git blame": > git blame -L 570,575 drivers/gpu/drm/v3d/v3d_gem.c … > 0d352a3a8a1f2 (Iago Toral Quiroga 2019-09-16 09:11:25 +0200 571) kfree(bin); > a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 572) v3d_job_put(&render->base); > 29cd13cfd7624 (Navid Emamdoost 2019-10-21 13:52:49 -0500 573) kfree(bin); > a783a09ee76d6 (Eric Anholt 2019-04-16 15:58:53 -0700 574) return ret; … > … > commit 29cd13cfd7624726d9e6becbae9aa419ef35af7f > Author: Navid Emamdoost <navid.emamdoost@gmail.com> > Date: Mon Oct 21 13:52:49 2019 -0500 > > drm/v3d: Fix memory leak in v3d_submit_cl_ioctl > > In the impelementation of v3d_submit_cl_ioctl() there are two memory … It seems that this patch got insufficient code review attention before. https://lore.kernel.org/dri-devel/20191021185250.26130-1-navid.emamdoost@gmail.com/ https://lore.kernel.org/patchwork/comment/1342088/ https://lore.kernel.org/patchwork/patch/1142603/ Thus the fix chain will be extended. Regards, Markus
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 549dde83408b..37515e47b47e 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -568,7 +568,6 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, ret = v3d_job_init(v3d, file_priv, &bin->base, v3d_job_free, args->in_sync_bcl); if (ret) { - kfree(bin); v3d_job_put(&render->base); kfree(bin); return ret;
v3d_submit_cl_ioctl call kfree() with variable 'bin' twice. Fix it by removing the latter one. Signed-off-by: yu kuai <yukuai3@huawei.com> --- drivers/gpu/drm/v3d/v3d_gem.c | 1 - 1 file changed, 1 deletion(-)