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 --- drivers/gpu/drm/v3d/v3d_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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; }
In the impelementation of v3d_submit_cl_ioctl() there are two memory leaks.
Please avoid another typo also in this change description.
… 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.
Will an “imperative mood” be more appropriate for such wordings? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
…
+++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -557,13 +557,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
…
if (ret) { v3d_job_put(&render->base);
kfree(bin);
…
Can it be helpful to move the added function call before the other in this if branch (if you prefer to avoid the addition of a jump target here)?
Regards, Markus
…
+++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -557,13 +557,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
…
if (ret) { v3d_job_put(&render->base);
kfree(bin);…
Can it be helpful to move the added function call before the other in this if branch (if you prefer to avoid the addition of a jump target here)?
I got into the mood to take another look at these implementation details. I suggest then to look at the commit 0d352a3a8a1f26168d09f7073e61bb4b328e3bb9 ("drm/v3d: don't leak bin job if v3d_job_init fails." from 2019-09-18) once more.
With which software versions did you perform your source code analysis?
Regards, Markus
On Mon, Oct 21, 2019 at 01:52:49PM -0500, Navid Emamdoost wrote:
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
drivers/gpu/drm/v3d/v3d_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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);
The job isn't initialized yet, this doesn't work.
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);
v3d_job_put will call kfree, if you chase the callchain long enough (in v3d_job_free). So no bug here, this would lead to a double kfree and crash. -Daniel
kfree(bin); return ret;-- 2.17.1
On Tue, Oct 22, 2019 at 4:36 AM Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 21, 2019 at 01:52:49PM -0500, Navid Emamdoost wrote:
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
drivers/gpu/drm/v3d/v3d_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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);The job isn't initialized yet, this doesn't work.
Do you mean we have to release render via kfree() here?
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);v3d_job_put will call kfree, if you chase the callchain long enough (in v3d_job_free). So no bug here, this would lead to a double kfree and crash.
Yes, v3d_job_put() takes care of render,
-Daniel
kfree(bin);
but how about leaking bin?
return ret; }-- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Oct 22, 2019 at 10:53:57PM -0500, Navid Emamdoost wrote:
On Tue, Oct 22, 2019 at 4:36 AM Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 21, 2019 at 01:52:49PM -0500, Navid Emamdoost wrote:
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
drivers/gpu/drm/v3d/v3d_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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);The job isn't initialized yet, this doesn't work.
Do you mean we have to release render via kfree() here?
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);v3d_job_put will call kfree, if you chase the callchain long enough (in v3d_job_free). So no bug here, this would lead to a double kfree and crash.
Yes, v3d_job_put() takes care of render,
-Daniel
kfree(bin);but how about leaking bin?
Sorry, I totally missed that this is bin, no render. Patch looks correct to me.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return ret; }-- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Navid.
On Wed, Oct 23, 2019 at 11:16:01AM +0200, Daniel Vetter wrote:
On Tue, Oct 22, 2019 at 10:53:57PM -0500, Navid Emamdoost wrote:
On Tue, Oct 22, 2019 at 4:36 AM Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Oct 21, 2019 at 01:52:49PM -0500, Navid Emamdoost wrote:
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
drivers/gpu/drm/v3d/v3d_gem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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);The job isn't initialized yet, this doesn't work.
Do you mean we have to release render via kfree() here?
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);v3d_job_put will call kfree, if you chase the callchain long enough (in v3d_job_free). So no bug here, this would lead to a double kfree and crash.
Yes, v3d_job_put() takes care of render,
-Daniel
kfree(bin);but how about leaking bin?
Sorry, I totally missed that this is bin, no render. Patch looks correct to me.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Double-checked with Eric and applied to drm-misc-fixes. -Daniel
return ret; }-- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Navid.
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Markus Elfring -
Navid Emamdoost