Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
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 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 > > > > --- > > > > 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(>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, >base, > > > > v3d_job_free, args->in_sync_bcl); > > > > if (ret) { > > > > v3d_job_put(>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 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
Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
On Tue, Oct 22, 2019 at 10:53:57PM -0500, Navid Emamdoost wrote: > On Tue, Oct 22, 2019 at 4:36 AM Daniel Vetter 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 > > > --- > > > 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(>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, >base, > > > v3d_job_free, args->in_sync_bcl); > > > if (ret) { > > > v3d_job_put(>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 > > > > 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 mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
> … >> +++ 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(>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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
On Tue, Oct 22, 2019 at 4:36 AM Daniel Vetter 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 > > --- > > 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(>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, >base, > > v3d_job_free, args->in_sync_bcl); > > if (ret) { > > v3d_job_put(>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.
Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
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 > --- > 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(>base); The job isn't initialized yet, this doesn't work. > return -ENOMEM; > + } > > ret = v3d_job_init(v3d, file_priv, >base, > v3d_job_free, args->in_sync_bcl); > if (ret) { > v3d_job_put(>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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: Fix memory leak in v3d_submit_cl_ioctl
> 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(>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
[PATCH] 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 --- 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(>base); return -ENOMEM; + } ret = v3d_job_init(v3d, file_priv, >base, v3d_job_free, args->in_sync_bcl); if (ret) { v3d_job_put(>base); + kfree(bin); return ret; } -- 2.17.1