* [PATCH] nvme-loop: kfree(ctrl) on _create() error exit
@ 2016-10-26 20:20 Jay Freyensee
2016-10-27 12:13 ` Sagi Grimberg
2016-10-27 12:49 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Jay Freyensee @ 2016-10-26 20:20 UTC (permalink / raw)
ctrl is kzalloc'ed but looks like it's never kfree'ed
on the goto exit cases on nvme_loop_create_ctrl().
Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
drivers/nvme/target/loop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index d5df77d..b0f9931 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -673,6 +673,7 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
nvme_uninit_ctrl(&ctrl->ctrl);
out_put_ctrl:
nvme_put_ctrl(&ctrl->ctrl);
+ kfree(ctrl);
if (ret > 0)
ret = -EIO;
return ERR_PTR(ret);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] nvme-loop: kfree(ctrl) on _create() error exit
2016-10-26 20:20 [PATCH] nvme-loop: kfree(ctrl) on _create() error exit Jay Freyensee
@ 2016-10-27 12:13 ` Sagi Grimberg
2016-10-27 12:49 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2016-10-27 12:13 UTC (permalink / raw)
queued to the next round of rc fixes, thanks Jay..
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvme-loop: kfree(ctrl) on _create() error exit
2016-10-26 20:20 [PATCH] nvme-loop: kfree(ctrl) on _create() error exit Jay Freyensee
2016-10-27 12:13 ` Sagi Grimberg
@ 2016-10-27 12:49 ` Christoph Hellwig
2016-10-27 15:40 ` J Freyensee
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2016-10-27 12:49 UTC (permalink / raw)
On Wed, Oct 26, 2016@01:20:57PM -0700, Jay Freyensee wrote:
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index d5df77d..b0f9931 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -673,6 +673,7 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
> nvme_uninit_ctrl(&ctrl->ctrl);
> out_put_ctrl:
> nvme_put_ctrl(&ctrl->ctrl);
> + kfree(ctrl);
> if (ret > 0)
> ret = -EIO;
> return ERR_PTR(ret);
This will give us a double free. The final put in nvme_put_ctrl calls
->free_ctrl which will free the controller.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvme-loop: kfree(ctrl) on _create() error exit
2016-10-27 12:49 ` Christoph Hellwig
@ 2016-10-27 15:40 ` J Freyensee
2016-10-27 16:00 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: J Freyensee @ 2016-10-27 15:40 UTC (permalink / raw)
On Thu, 2016-10-27@05:49 -0700, Christoph Hellwig wrote:
> On Wed, Oct 26, 2016@01:20:57PM -0700, Jay Freyensee wrote:
> >
> > diff --git a/drivers/nvme/target/loop.c
> > b/drivers/nvme/target/loop.c
> > index d5df77d..b0f9931 100644
> > --- a/drivers/nvme/target/loop.c
> > +++ b/drivers/nvme/target/loop.c
> > @@ -673,6 +673,7 @@ static struct nvme_ctrl
> > *nvme_loop_create_ctrl(struct device *dev,
> > ? nvme_uninit_ctrl(&ctrl->ctrl);
> > ?out_put_ctrl:
> > ? nvme_put_ctrl(&ctrl->ctrl);
> > + kfree(ctrl);
> > ? if (ret > 0)
> > ? ret = -EIO;
> > ? return ERR_PTR(ret);
>
> This will give us a double free.??The final put in nvme_put_ctrl
> calls
> ->free_ctrl which will free the controller.
But doesn't nvme_put_ctrl() just take care of the nvme_ctrl? ?The
kfree(ctrl) is for nvme_loop_ctrl instance being kzalloc'ed at the
beginning of the function. ?I don't think this is any different than
what is done for the create_ctrl() implementation in host/rdma.c?:
host/rdma.c
===========
out_uninit_ctrl:
nvme_uninit_ctrl(&ctrl->ctrl);
nvme_put_ctrl(&ctrl->ctrl);
if (ret > 0)
ret = -EIO;
return ERR_PTR(ret);
out_free_ctrl:
kfree(ctrl); ? <-- this is for nvme_rdma_ctrl alloc
return ERR_PTR(ret);
target/loop.c
========
out_uninit_ctrl:
nvme_uninit_ctrl(&ctrl->ctrl);
out_put_ctrl:
nvme_put_ctrl(&ctrl->ctrl);
kfree(ctrl); ? <-- the proposed fix for nvme_loop_ctrl alloc
if (ret > 0)
ret = -EIO;
return ERR_PTR(ret);
}
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvme-loop: kfree(ctrl) on _create() error exit
2016-10-27 15:40 ` J Freyensee
@ 2016-10-27 16:00 ` Christoph Hellwig
2016-10-30 6:48 ` Sagi Grimberg
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2016-10-27 16:00 UTC (permalink / raw)
On Thu, Oct 27, 2016@08:40:13AM -0700, J Freyensee wrote:
> But doesn't nvme_put_ctrl() just take care of the nvme_ctrl? ?The
> kfree(ctrl) is for nvme_loop_ctrl instance being kzalloc'ed at the
> beginning of the function.
The nvmet_ctrl structure is embedded into the nvme_loop_ctrl structure,
it's not a separate allocation.
> ?I don't think this is any different than
> what is done for the create_ctrl() implementation in host/rdma.c?:
>
> host/rdma.c
> ===========
> out_uninit_ctrl:
> nvme_uninit_ctrl(&ctrl->ctrl);
> nvme_put_ctrl(&ctrl->ctrl);
> if (ret > 0)
> ret = -EIO;
> return ERR_PTR(ret);
This is the case after nvme_init_ctrl and we just do the put and then
return.
> out_free_ctrl:
> kfree(ctrl); ? <-- this is for nvme_rdma_ctrl alloc
> return ERR_PTR(ret);
And this is the case before we did nvme_init_ctrl and we just kfree the
ctrl.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-30 6:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-26 20:20 [PATCH] nvme-loop: kfree(ctrl) on _create() error exit Jay Freyensee
2016-10-27 12:13 ` Sagi Grimberg
2016-10-27 12:49 ` Christoph Hellwig
2016-10-27 15:40 ` J Freyensee
2016-10-27 16:00 ` Christoph Hellwig
2016-10-30 6:48 ` Sagi Grimberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.