[dpdk-dev,v2,03/15] eal: Fix memory leak of pci_uio_map_resource()

Message ID 1426155474-1596-4-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tetsuya Mukawa March 12, 2015, 10:17 a.m. UTC
  When pci_map_resource() is failed but path is allocated correctly,
path won't be freed. Also, when open() is failed, uio_res won't be freed.
This patch fixes these.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
  

Comments

Iremonger, Bernard March 13, 2015, 2:04 p.m. UTC | #1
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Thursday, March 12, 2015 10:18 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa
> Subject: [PATCH v2 03/15] eal: Fix memory leak of pci_uio_map_resource()
> 
> When pci_map_resource() is failed but path is allocated correctly, path won't be freed. Also, when
> open() is failed, uio_res won't be freed.
> This patch fixes these.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index 77bb5ed..901f277 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -350,6 +350,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  				loc->domain, loc->bus, loc->devid,
>  				loc->function, i);
> 
> +		/* allocate memory to keep path */
> +		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> +		if (maps[map_idx].path == NULL)
> +			goto fail0;
> +
>  		/*
>  		 * open resource file, to mmap it
>  		 */
> @@ -357,7 +362,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  		if (fd < 0) {
>  			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>  					devname, strerror(errno));
> -			return -1;
> +			goto fail0;
>  		}
> 
>  		/* try mapping somewhere close to the end of hugepages */ @@ -372,14 +377,9 @@
> pci_uio_map_resource(struct rte_pci_device *dev)
>  		pci_map_addr = RTE_PTR_ADD(mapaddr,
>  				(size_t)dev->mem_resource[i].len);
> 
> -		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> -		if (maps[map_idx].path == NULL)
> -			fail = 1;
> -
Hi Tetsuya,

Could close(fd) be called before if(fail) and the following two close(fd) calls removed ?

Regards,

Bernard.

>  		if (fail) {
> -			rte_free(uio_res);
>  			close(fd);
> -			return -1;
> +			goto fail1;
>  		}
>  		close(fd);
> 
> @@ -397,6 +397,12 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  	TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
> 
>  	return 0;
> +
> +fail1:
> +	rte_free(maps[map_idx].path);
> +fail0:
> +	rte_free(uio_res);
> +	return -1;
>  }
> 
>  #ifdef RTE_LIBRTE_EAL_HOTPLUG
> --
> 1.9.1
  
Tetsuya Mukawa March 17, 2015, 9:30 a.m. UTC | #2
This patch set cleans up pci uio implementation. These clean up are
for consolidating pci uio implementation of linuxapp and bsdapp, and
moving consolidated functions in eal common.
Because of above, this patch set tries to implement linuxapp and bsdapp
almost same.
Actual consolidations will be done later patch set.

Changes:
 - This patch set is derived from below.
   "[PATCH v2] eal: Port Hotplug support for BSD"
 - Set cfg_fd as -1, when cfg_fd is closed.
   (Thanks to Iremonger, Bernard)
 - Remove needless cording style fixings.
 - Fix cording style of if-else condition.
   (Thanks to Richardson, Bruce)

Tetsuya Mukawa (6):
  eal: Fix cording style of eal_pci.c and eal_pci_uio.c
  eal: Close file descriptor of uio configuration
  eal: Fix memory leaks and needless incrementation of pci uio
    implementation
  eal/bsdapp: Change names of pci related data structure
  eal: Use map_idx in pci_uio_map_resource() of bsdapp to work same as
    linuxapp
  eal: Fix interface of pci_map_resource()

 lib/librte_eal/bsdapp/eal/eal_pci.c       | 160 +++++++++++++++++-------------
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c |  67 +++++++------
 2 files changed, 130 insertions(+), 97 deletions(-)
  

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index 77bb5ed..901f277 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -350,6 +350,11 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 				loc->domain, loc->bus, loc->devid,
 				loc->function, i);
 
+		/* allocate memory to keep path */
+		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+		if (maps[map_idx].path == NULL)
+			goto fail0;
+
 		/*
 		 * open resource file, to mmap it
 		 */
@@ -357,7 +362,7 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 		if (fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 					devname, strerror(errno));
-			return -1;
+			goto fail0;
 		}
 
 		/* try mapping somewhere close to the end of hugepages */
@@ -372,14 +377,9 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 		pci_map_addr = RTE_PTR_ADD(mapaddr,
 				(size_t)dev->mem_resource[i].len);
 
-		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
-		if (maps[map_idx].path == NULL)
-			fail = 1;
-
 		if (fail) {
-			rte_free(uio_res);
 			close(fd);
-			return -1;
+			goto fail1;
 		}
 		close(fd);
 
@@ -397,6 +397,12 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
 
 	return 0;
+
+fail1:
+	rte_free(maps[map_idx].path);
+fail0:
+	rte_free(uio_res);
+	return -1;
 }
 
 #ifdef RTE_LIBRTE_EAL_HOTPLUG