* [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl @ 2019-10-21 18:52 Navid Emamdoost 2019-10-21 20:40 ` Markus Elfring 2019-10-22 9:36 ` Daniel Vetter 0 siblings, 2 replies; 7+ messages in thread From: Navid Emamdoost @ 2019-10-21 18:52 UTC (permalink / raw) Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Eric Anholt, David Airlie, Daniel Vetter, dri-devel, linux-kernel 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; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-21 18:52 [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl Navid Emamdoost @ 2019-10-21 20:40 ` Markus Elfring 2019-10-22 7:30 ` Markus Elfring 2019-10-22 9:36 ` Daniel Vetter 1 sibling, 1 reply; 7+ messages in thread From: Markus Elfring @ 2019-10-21 20:40 UTC (permalink / raw) To: Navid Emamdoost, dri-devel Cc: kernel-janitors, linux-kernel, Navid Emamdoost, Kangjie Lu, Stephen McCamant, Daniel Vetter, David Airlie, Eric Anholt > 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/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151 … > +++ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-21 20:40 ` Markus Elfring @ 2019-10-22 7:30 ` Markus Elfring 0 siblings, 0 replies; 7+ messages in thread From: Markus Elfring @ 2019-10-22 7:30 UTC (permalink / raw) To: Navid Emamdoost, dri-devel Cc: kernel-janitors, linux-kernel, Navid Emamdoost, Kangjie Lu, Stephen McCamant, Daniel Vetter, David Airlie, Eric Anholt, Iago Toral Quiroga > … >> +++ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-21 18:52 [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl Navid Emamdoost 2019-10-21 20:40 ` Markus Elfring @ 2019-10-22 9:36 ` Daniel Vetter 2019-10-23 3:53 ` Navid Emamdoost 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2019-10-22 9:36 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, kjlu, smccaman, Eric Anholt, David Airlie, Daniel Vetter, dri-devel, linux-kernel 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-22 9:36 ` Daniel Vetter @ 2019-10-23 3:53 ` Navid Emamdoost 2019-10-23 9:16 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Navid Emamdoost @ 2019-10-23 3:53 UTC (permalink / raw) To: Daniel Vetter Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Eric Anholt, David Airlie, dri-devel, LKML 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 -- Navid. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-23 3:53 ` Navid Emamdoost @ 2019-10-23 9:16 ` Daniel Vetter 2019-10-23 18:59 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2019-10-23 9:16 UTC (permalink / raw) To: Navid Emamdoost Cc: Daniel Vetter, Navid Emamdoost, Kangjie Lu, Stephen McCamant, Eric Anholt, David Airlie, dri-devel, LKML 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. -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl 2019-10-23 9:16 ` Daniel Vetter @ 2019-10-23 18:59 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2019-10-23 18:59 UTC (permalink / raw) To: Navid Emamdoost, Navid Emamdoost, Kangjie Lu, Stephen McCamant, Eric Anholt, David Airlie, dri-devel, LKML 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-23 19:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-21 18:52 [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl Navid Emamdoost 2019-10-21 20:40 ` Markus Elfring 2019-10-22 7:30 ` Markus Elfring 2019-10-22 9:36 ` Daniel Vetter 2019-10-23 3:53 ` Navid Emamdoost 2019-10-23 9:16 ` Daniel Vetter 2019-10-23 18:59 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).