| Message ID | 20191001173726.21846-1-vishal.l.verma@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7438ce43d0371a3b90f333a93a85dcfa2799b4d9 |
| Headers | show |
| Series | [ndctl] libdaxctl: fix memory leaks with daxctl_memory objects | expand |
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
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.
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
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:
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(-)