diff mbox

[dpdk-dev,3/6] eal: Fix memory leaks and needless incrementation of pci uio implementation

Message ID 1426584645-28828-4-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Tetsuya Mukawa March 17, 2015, 9:30 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 memory leaks.
When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
In this case, pci_map_addr should not be incremented.

Also, the patch fixes belows.
 - To shrink code, move close().
 - Remove fail variable.

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

Comments

David Marchand March 18, 2015, 3:39 p.m. UTC | #1 
On Tue, Mar 17, 2015 at 10:30 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:

> 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 memory leaks.
> When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
> In this case, pci_map_addr should not be incremented.
>
> Also, the patch fixes belows.
>  - To shrink code, move close().
>  - Remove fail variable.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 28
> ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index b971ec9..5044884 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -333,7 +333,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>         maps = uio_res->maps;
>         for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>                 int fd;
> -               int fail = 0;
>
>                 /* skip empty BAR */
>                 phaddr = dev->mem_resource[i].phys_addr;
> @@ -347,6 +346,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;
> +
>

 [snip]

Neither fail0 nor fail1 labels seem to free previous allocations.
Did I miss something ?

[snip]

+
> +fail1:
> +       rte_free(maps[map_idx].path);
> +fail0:
> +       rte_free(uio_res);
> +       return -1;
>  }
>
Iremonger, Bernard March 19, 2015, 4:20 p.m. UTC | #2 
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, March 17, 2015 9:31 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa
> Subject: [PATCH 3/6] eal: Fix memory leaks and needless incrementation of pci uio implementation

Hi Tetsuya,

It would be better to reword "needless incrementation of pci uio implementation" to "needless increment of pci_map_addr" in commit line.

> 
> 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 memory leaks.
> When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
> In this case, pci_map_addr should not be incremented.
> 
> Also, the patch fixes belows.

Typo "belows" should be "below".

Regards,

Bernard.

>  - To shrink code, move close().
>  - Remove fail variable.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index b971ec9..5044884 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -333,7 +333,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  	maps = uio_res->maps;
>  	for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>  		int fd;
> -		int fail = 0;
> 
>  		/* skip empty BAR */
>  		phaddr = dev->mem_resource[i].phys_addr; @@ -347,6 +346,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
>  		 */
> @@ -354,7 +358,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 fail1;
>  		}
> 
>  		/* try mapping somewhere close to the end of hugepages */ @@ -363,23 +367,13 @@
> pci_uio_map_resource(struct rte_pci_device *dev)
> 
>  		mapaddr = pci_map_resource(pci_map_addr, fd, 0,
>  				(size_t)dev->mem_resource[i].len, 0);
> +		close(fd);
>  		if (mapaddr == MAP_FAILED)
> -			fail = 1;
> +			goto fail1;
> 
>  		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;
> -		}
> -		close(fd);
> -
>  		maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
>  		maps[map_idx].size = dev->mem_resource[i].len;
>  		maps[map_idx].addr = mapaddr;
> @@ -394,6 +388,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 20, 2015, 1:53 a.m. UTC | #3 
On 2015/03/19 0:39, David Marchand wrote:
> On Tue, Mar 17, 2015 at 10:30 AM, Tetsuya Mukawa <mukawa@igel.co.jp
> <mailto:mukawa@igel.co.jp>> wrote:
>
>     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 memory leaks.
>     When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
>     In this case, pci_map_addr should not be incremented.
>
>     Also, the patch fixes belows.
>      - To shrink code, move close().
>      - Remove fail variable.
>
>     Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp
>     <mailto:mukawa@igel.co.jp>>
>     ---
>      lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 28
>     ++++++++++++++--------------
>      1 file changed, 14 insertions(+), 14 deletions(-)
>
>     diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     index b971ec9..5044884 100644
>     --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     @@ -333,7 +333,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>             maps = uio_res->maps;
>             for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>                     int fd;
>     -               int fail = 0;
>
>                     /* skip empty BAR */
>                     phaddr = dev->mem_resource[i].phys_addr;
>     @@ -347,6 +346,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;
>     +
>
>
>  [snip]
>
> Neither fail0 nor fail1 labels seem to free previous allocations.
> Did I miss something ?

Hi David,


Ah, you are right. I will free all 'maps[i].path' allocated in this
function.

Thanks,
Tetsuya


>
> [snip]
>
>     +
>     +fail1:
>     +       rte_free(maps[map_idx].path);
>     +fail0:
>     +       rte_free(uio_res);
>     +       return -1;
>      }
>
>
>  
>
> -- 
> David Marchand
Tetsuya Mukawa March 20, 2015, 1:54 a.m. UTC | #4 
On 2015/03/20 1:20, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Tuesday, March 17, 2015 9:31 AM
>> To: dev@dpdk.org
>> Cc: Iremonger, Bernard; Richardson, Bruce; Tetsuya Mukawa
>> Subject: [PATCH 3/6] eal: Fix memory leaks and needless incrementation of pci uio implementation
> Hi Tetsuya,
>
> It would be better to reword "needless incrementation of pci uio implementation" to "needless increment of pci_map_addr" in commit line.
>
>> 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 memory leaks.
>> When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
>> In this case, pci_map_addr should not be incremented.
>>
>> Also, the patch fixes belows.
> Typo "belows" should be "below".

Hi Bernard,

I appreciate for your checking. I will fix it.

Regards,
Tetsuya

> Regards,
>
> Bernard.
>
>>  - To shrink code, move close().
>>  - Remove fail variable.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> index b971ec9..5044884 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> @@ -333,7 +333,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  	maps = uio_res->maps;
>>  	for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>>  		int fd;
>> -		int fail = 0;
>>
>>  		/* skip empty BAR */
>>  		phaddr = dev->mem_resource[i].phys_addr; @@ -347,6 +346,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
>>  		 */
>> @@ -354,7 +358,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 fail1;
>>  		}
>>
>>  		/* try mapping somewhere close to the end of hugepages */ @@ -363,23 +367,13 @@
>> pci_uio_map_resource(struct rte_pci_device *dev)
>>
>>  		mapaddr = pci_map_resource(pci_map_addr, fd, 0,
>>  				(size_t)dev->mem_resource[i].len, 0);
>> +		close(fd);
>>  		if (mapaddr == MAP_FAILED)
>> -			fail = 1;
>> +			goto fail1;
>>
>>  		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;
>> -		}
>> -		close(fd);
>> -
>>  		maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
>>  		maps[map_idx].size = dev->mem_resource[i].len;
>>  		maps[map_idx].addr = mapaddr;
>> @@ -394,6 +388,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
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index b971ec9..5044884 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -333,7 +333,6 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	maps = uio_res->maps;
 	for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
 		int fd;
-		int fail = 0;
 
 		/* skip empty BAR */
 		phaddr = dev->mem_resource[i].phys_addr;
@@ -347,6 +346,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
 		 */
@@ -354,7 +358,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 fail1;
 		}
 
 		/* try mapping somewhere close to the end of hugepages */
@@ -363,23 +367,13 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 
 		mapaddr = pci_map_resource(pci_map_addr, fd, 0,
 				(size_t)dev->mem_resource[i].len, 0);
+		close(fd);
 		if (mapaddr == MAP_FAILED)
-			fail = 1;
+			goto fail1;
 
 		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;
-		}
-		close(fd);
-
 		maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
 		maps[map_idx].size = dev->mem_resource[i].len;
 		maps[map_idx].addr = mapaddr;
@@ -394,6 +388,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