* [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects
@ 2019-10-01 17:37 Vishal Verma
2019-10-01 23:19 ` Ira Weiny
0 siblings, 1 reply; 4+ messages in thread
From: Vishal Verma @ 2019-10-01 17:37 UTC (permalink / raw)
To: linux-nvdimm; +Cc: Michal Biesek
The daxctl_dev_alloc_mem() helper which is used to instantiate a new
memory object did so, but neglected to attach the memory object to the
parent 'dev' object. As a result, every invocation of
'daxctl_dev_get_memory() resulted in a new, orphan memory object being
created, which also resulted in libdaxctl leaking memory.
Fix the parent association for 'mem' objects, and in free_mem, remove
the check for 'dev' being present - mem objects will always associated
with a dev.
Additionally, we were neglecting to free 'mem->mem_buf' in free_mem, so
fix this up as well.
Fixes: e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")
Link: https://github.com/pmem/ndctl/issues/112
Reported-by: Michal Biesek <michal.biesek@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
daxctl/lib/libdaxctl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 8abfd64..639224c 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
static void free_mem(struct daxctl_dev *dev)
{
- if (dev && dev->mem) {
+ if (dev->mem) {
free(dev->mem->node_path);
+ free(dev->mem->mem_buf);
free(dev->mem);
dev->mem = NULL;
}
@@ -450,6 +451,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
goto err_node;
mem->buf_len = strlen(node_base) + 256;
+ dev->mem = mem;
return mem;
err_node:
--
2.20.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects
2019-10-01 17:37 [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects Vishal Verma
@ 2019-10-01 23:19 ` Ira Weiny
2019-10-03 0:01 ` Verma, Vishal L
0 siblings, 1 reply; 4+ messages in thread
From: Ira Weiny @ 2019-10-01 23:19 UTC (permalink / raw)
To: Vishal Verma; +Cc: linux-nvdimm, Michal Biesek
On Tue, Oct 01, 2019 at 11:37:26AM -0600, 'Vishal Verma' wrote:
> The daxctl_dev_alloc_mem() helper which is used to instantiate a new
> memory object did so, but neglected to attach the memory object to the
> parent 'dev' object. As a result, every invocation of
> 'daxctl_dev_get_memory() resulted in a new, orphan memory object being
> created, which also resulted in libdaxctl leaking memory.
>
> Fix the parent association for 'mem' objects, and in free_mem, remove
> the check for 'dev' being present - mem objects will always associated
> with a dev.
>
> Additionally, we were neglecting to free 'mem->mem_buf' in free_mem, so
> fix this up as well.
>
> Fixes: e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")
> Link: https://github.com/pmem/ndctl/issues/112
> Reported-by: Michal Biesek <michal.biesek@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Minor NIT below.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
> daxctl/lib/libdaxctl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 8abfd64..639224c 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
>
> static void free_mem(struct daxctl_dev *dev)
> {
> - if (dev && dev->mem) {
> + if (dev->mem) {
There is a comment in daxctl_dev_disable() which says:
/* If there is a memory object, first free that */
I'm 100% sure that dev can't be NULL there. So that comment no longer applies.
May want to remove that comment.
Ira
> free(dev->mem->node_path);
> + free(dev->mem->mem_buf);
> free(dev->mem);
> dev->mem = NULL;
> }
> @@ -450,6 +451,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
> goto err_node;
> mem->buf_len = strlen(node_base) + 256;
>
> + dev->mem = mem;
> return mem;
>
> err_node:
> --
> 2.20.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects
2019-10-01 23:19 ` Ira Weiny
@ 2019-10-03 0:01 ` Verma, Vishal L
2019-10-03 15:41 ` Ira Weiny
0 siblings, 1 reply; 4+ messages in thread
From: Verma, Vishal L @ 2019-10-03 0:01 UTC (permalink / raw)
To: Weiny, Ira; +Cc: linux-nvdimm, Biesek, Michal
On Tue, 2019-10-01 at 16:19 -0700, Ira Weiny wrote:
>
> > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> > index 8abfd64..639224c 100644
> > --- a/daxctl/lib/libdaxctl.c
> > +++ b/daxctl/lib/libdaxctl.c
> > @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
> >
> > static void free_mem(struct daxctl_dev *dev)
> > {
> > - if (dev && dev->mem) {
> > + if (dev->mem) {
>
> There is a comment in daxctl_dev_disable() which says:
>
> /* If there is a memory object, first free that */
>
> I'm 100% sure that dev can't be NULL there. So that comment no longer applies.
>
> May want to remove that comment.
>
Hm, I'm not sure I follow.
Yes, dev can't be null there, but 'mem' can me. Hence the comment saying
if /mem/ is present, free it.
The check in free_mem() only checks for non-NULL mem too.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects
2019-10-03 0:01 ` Verma, Vishal L
@ 2019-10-03 15:41 ` Ira Weiny
0 siblings, 0 replies; 4+ messages in thread
From: Ira Weiny @ 2019-10-03 15:41 UTC (permalink / raw)
To: Verma, Vishal L; +Cc: linux-nvdimm, Biesek, Michal
On Wed, Oct 02, 2019 at 05:01:24PM -0700, 'Vishal Verma' wrote:
> On Tue, 2019-10-01 at 16:19 -0700, Ira Weiny wrote:
> >
> > > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> > > index 8abfd64..639224c 100644
> > > --- a/daxctl/lib/libdaxctl.c
> > > +++ b/daxctl/lib/libdaxctl.c
> > > @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
> > >
> > > static void free_mem(struct daxctl_dev *dev)
> > > {
> > > - if (dev && dev->mem) {
> > > + if (dev->mem) {
> >
> > There is a comment in daxctl_dev_disable() which says:
> >
> > /* If there is a memory object, first free that */
> >
> > I'm 100% sure that dev can't be NULL there. So that comment no longer applies.
> >
> > May want to remove that comment.
> >
> Hm, I'm not sure I follow.
>
> Yes, dev can't be null there, but 'mem' can me. Hence the comment saying
> if /mem/ is present, free it.
>
> The check in free_mem() only checks for non-NULL mem too.
Ah yea ok. Sorry
Ira
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-03 15:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 17:37 [ndctl PATCH] libdaxctl: fix memory leaks with daxctl_memory objects Vishal Verma
2019-10-01 23:19 ` Ira Weiny
2019-10-03 0:01 ` Verma, Vishal L
2019-10-03 15:41 ` Ira Weiny
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).