[PATCH] eal: Fix wrong error checking of rte_eal_parse_devargs_str()

[PATCH] eal: Fix wrong error checking of rte_eal_parse_devargs_str()

[Tetsuya Mukawa]

Currently, a return value of strdup() isn't checked correctly.
The patch fixes it.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_devargs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 2bfe54a..e403717 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -58,7 +58,7 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 		return -1;
 
 	*drvname = strdup(devargs_str);
-	if (drvname == NULL)
+	if (*drvname == NULL)
 		return -1;
 
 	/* set the first ',' to '\0' to split name and arguments */
-- 
2.7.4

Re: [PATCH] eal: Fix wrong error checking of rte_eal_parse_devargs_str()

[David Marchand]

On Wed, Jun 15, 2016 at 5:26 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> Currently, a return value of strdup() isn't checked correctly.
> The patch fixes it.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

Idem, the title does not need the function name to describe what is going wrong.

This might deserve a fixline, I traced this to 0fe11ec592b2 ("eal: add
vdev init and uninit").

> ---
>  lib/librte_eal/common/eal_common_devargs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 2bfe54a..e403717 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -58,7 +58,7 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>                 return -1;
>
>         *drvname = strdup(devargs_str);
> -       if (drvname == NULL)
> +       if (*drvname == NULL)
>                 return -1;
>
>         /* set the first ',' to '\0' to split name and arguments */
> --
> 2.7.4
>

-- 
David Marchand

Re: [PATCH] eal: Fix wrong error checking of rte_eal_parse_devargs_str()

[Tetsuya Mukawa]

On 2016/06/15 23:48, David Marchand wrote:
> On Wed, Jun 15, 2016 at 5:26 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>> Currently, a return value of strdup() isn't checked correctly.
>> The patch fixes it.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> 
> Idem, the title does not need the function name to describe what is going wrong.
> 
> This might deserve a fixline, I traced this to 0fe11ec592b2 ("eal: add
> vdev init and uninit").

Will add it.

Thanks,
Tetsuya

[PATCH v2] eal: Fix wrong error checking while parsing device arguments

[Tetsuya Mukawa]

This patch fixes wrong error checking of rte_eal_parse_devargs_str().
Currently, a return value of strdup() is wrongly checked.

Fixes: 0fe11ec592b2 ("eal: add vdev init and uninit")
Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_devargs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 2bfe54a..e403717 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -58,7 +58,7 @@ rte_eal_parse_devargs_str(const char *devargs_str,
 		return -1;
 
 	*drvname = strdup(devargs_str);
-	if (drvname == NULL)
+	if (*drvname == NULL)
 		return -1;
 
 	/* set the first ',' to '\0' to split name and arguments */
-- 
2.7.4

[PATCH] eal: Fix resource leak of pci_uio_map_secondary()

[Tetsuya Mukawa]

If pci_map_resource() succeeds but mapped address is different from an
address primary process mapped, this should be error.
Then the address secondary process mapped should be freed.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_pci_uio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
index f062e81..e718643 100644
--- a/lib/librte_eal/common/eal_common_pci_uio.c
+++ b/lib/librte_eal/common/eal_common_pci_uio.c
@@ -85,6 +85,9 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 					"Cannot mmap device resource file %s to address: %p\n",
 					uio_res->maps[i].path,
 					uio_res->maps[i].addr);
+				if (mapaddr != MAP_FAILED)
+					pci_unmap_resource(mapaddr,
+						(size_t)uio_res->maps[i].size);
 				return -1;
 			}
 		}
-- 
2.7.4

Re: [PATCH] eal: Fix resource leak of pci_uio_map_secondary()

[David Marchand]

Hello Tetsuya,

On Wed, Jun 15, 2016 at 5:26 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> If pci_map_resource() succeeds but mapped address is different from an
> address primary process mapped, this should be error.
> Then the address secondary process mapped should be freed.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

The title should not contain the function name.


> ---
>  lib/librte_eal/common/eal_common_pci_uio.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
> index f062e81..e718643 100644
> --- a/lib/librte_eal/common/eal_common_pci_uio.c
> +++ b/lib/librte_eal/common/eal_common_pci_uio.c
> @@ -85,6 +85,9 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>                                         "Cannot mmap device resource file %s to address: %p\n",
>                                         uio_res->maps[i].path,
>                                         uio_res->maps[i].addr);
> +                               if (mapaddr != MAP_FAILED)
> +                                       pci_unmap_resource(mapaddr,
> +                                               (size_t)uio_res->maps[i].size);
>                                 return -1;
>                         }
>                 }

What of the previous mappings that might have succeeded earlier in this loop ?


-- 
David Marchand

Re: [PATCH] eal: Fix resource leak of pci_uio_map_secondary()

[Tetsuya Mukawa]

On 2016/06/15 23:45, David Marchand wrote:
> Hello Tetsuya,
> 
> On Wed, Jun 15, 2016 at 5:26 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>> If pci_map_resource() succeeds but mapped address is different from an
>> address primary process mapped, this should be error.
>> Then the address secondary process mapped should be freed.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> 
> The title should not contain the function name.

Hi David,

Thanks, I will change the title of all patches.

> 
> 
>> ---
>>  lib/librte_eal/common/eal_common_pci_uio.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
>> index f062e81..e718643 100644
>> --- a/lib/librte_eal/common/eal_common_pci_uio.c
>> +++ b/lib/librte_eal/common/eal_common_pci_uio.c
>> @@ -85,6 +85,9 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>>                                         "Cannot mmap device resource file %s to address: %p\n",
>>                                         uio_res->maps[i].path,
>>                                         uio_res->maps[i].addr);
>> +                               if (mapaddr != MAP_FAILED)
>> +                                       pci_unmap_resource(mapaddr,
>> +                                               (size_t)uio_res->maps[i].size);
>>                                 return -1;
>>                         }
>>                 }
> 
> What of the previous mappings that might have succeeded earlier in this loop ?

I will unmap all addresses mapped before.

Thanks,
Tetsuya

[PATCH v2] eal: Fix resource leak while secondary process maps pci devices

[Tetsuya Mukawa]

This patch fixes resource leak of pci_uio_map_secondary().
If pci_map_resource() succeeds but mapped address is different from an
address primary process mapped, this should be error.
Then the addresses secondary process mapped should be freed.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_pci_uio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
index 488b6dd..83314c3 100644
--- a/lib/librte_eal/common/eal_common_pci_uio.c
+++ b/lib/librte_eal/common/eal_common_pci_uio.c
@@ -53,7 +53,7 @@ EAL_REGISTER_TAILQ(rte_uio_tailq)
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i;
+	int fd, i, j;
 	struct mapped_pci_resource *uio_res;
 	struct mapped_pci_res_list *uio_res_list =
 			RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
@@ -85,6 +85,17 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
 					"Cannot mmap device resource file %s to address: %p\n",
 					uio_res->maps[i].path,
 					uio_res->maps[i].addr);
+				if (mapaddr != MAP_FAILED) {
+					/* unmap addrs correctly mapped */
+					for (j = 0; j < i; j++)
+						pci_unmap_resource(
+							uio_res->maps[j].addr,
+							(size_t)uio_res->maps[j].size);
+
+					/* unmap addr wrongly mapped */
+					pci_unmap_resource(mapaddr,
+						(size_t)uio_res->maps[i].size);
+				}
 				return -1;
 			}
 		}
-- 
2.7.4

Re: [PATCH v2] eal: Fix resource leak while secondary process maps pci devices

[David Marchand]

On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> This patch fixes resource leak of pci_uio_map_secondary().
> If pci_map_resource() succeeds but mapped address is different from an
> address primary process mapped, this should be error.
> Then the addresses secondary process mapped should be freed.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

scripts/check-git-log.sh is not happy :

Wrong headline uppercase:
    eal: Fix resource leak while secondary process maps pci devices
Wrong headline lowercase:
    eal: Fix resource leak while secondary process maps pci devices
Headline too long:
    eal: Fix resource leak while secondary process maps pci devices
Missing 'Fixes' tag:
    eal: Fix resource leak while secondary process maps pci devices


checkpatch is not happy, but I think we can ignore it.

WARNING:LONG_LINE: line over 80 characters
#48: FILE: lib/librte_eal/common/eal_common_pci_uio.c:93:
+                            (size_t)uio_res->maps[j].size);


Anyways, looks good to me, Thomas, can you fix the commit logs of
those last 3 patches on eal ?
Thanks.

-- 
David Marchand

Re: [PATCH v2] eal: Fix resource leak while secondary process maps pci devices

[Tetsuya Mukawa]

On 2016/06/17 21:28, David Marchand wrote:
> On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>> This patch fixes resource leak of pci_uio_map_secondary().
>> If pci_map_resource() succeeds but mapped address is different from an
>> address primary process mapped, this should be error.
>> Then the addresses secondary process mapped should be freed.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> 
> scripts/check-git-log.sh is not happy :
> 
> Wrong headline uppercase:
>     eal: Fix resource leak while secondary process maps pci devices
> Wrong headline lowercase:
>     eal: Fix resource leak while secondary process maps pci devices
> Headline too long:
>     eal: Fix resource leak while secondary process maps pci devices
> Missing 'Fixes' tag:
>     eal: Fix resource leak while secondary process maps pci devices
> 
> 
> checkpatch is not happy, but I think we can ignore it.
> 
> WARNING:LONG_LINE: line over 80 characters
> #48: FILE: lib/librte_eal/common/eal_common_pci_uio.c:93:
> +                            (size_t)uio_res->maps[j].size);
> 
> 
> Anyways, looks good to me, Thomas, can you fix the commit logs of
> those last 3 patches on eal ?
> Thanks.
> 

Hi David,

I appreciate your checking.
Next time I will check check-git-log.sh before submitting.


Hi Thomas,

Could you please let me know if you need the fixed patches.

Best,
Tetsuya

Re: [PATCH v2] eal: Fix resource leak while secondary process maps pci devices

[Thomas Monjalon]

2016-06-20 11:19, Tetsuya Mukawa:
> On 2016/06/17 21:28, David Marchand wrote:
> > On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> >> This patch fixes resource leak of pci_uio_map_secondary().
> >> If pci_map_resource() succeeds but mapped address is different from an
> >> address primary process mapped, this should be error.
> >> Then the addresses secondary process mapped should be freed.
> >>
> >> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> > 
> > scripts/check-git-log.sh is not happy :
> > 
> > Wrong headline uppercase:
> >     eal: Fix resource leak while secondary process maps pci devices
> > Wrong headline lowercase:
> >     eal: Fix resource leak while secondary process maps pci devices
> > Headline too long:
> >     eal: Fix resource leak while secondary process maps pci devices
> > Missing 'Fixes' tag:
> >     eal: Fix resource leak while secondary process maps pci devices
> > 
> > 
> > checkpatch is not happy, but I think we can ignore it.
> > 
> > WARNING:LONG_LINE: line over 80 characters
> > #48: FILE: lib/librte_eal/common/eal_common_pci_uio.c:93:
> > +                            (size_t)uio_res->maps[j].size);
> > 
> > 
> > Anyways, looks good to me, Thomas, can you fix the commit logs of
> > those last 3 patches on eal ?
> > Thanks.
> > 
> 
> Hi David,
> 
> I appreciate your checking.
> Next time I will check check-git-log.sh before submitting.
> 
> 
> Hi Thomas,
> 
> Could you please let me know if you need the fixed patches.

Applied, thanks

[PATCH] eal: Fix wrong resource release of pci_uio_unmap()

[Tetsuya Mukawa]

The 'path' member of mapped_pci_resource structure is allocated by
primary process, but currenctly it will be freed by both primary
and secondary process.
The patch fixes to be freed by only primary process.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_pci_uio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
index e718643..b2c68f3 100644
--- a/lib/librte_eal/common/eal_common_pci_uio.c
+++ b/lib/librte_eal/common/eal_common_pci_uio.c
@@ -162,7 +162,9 @@ pci_uio_unmap(struct mapped_pci_resource *uio_res)
 	for (i = 0; i != uio_res->nb_maps; i++) {
 		pci_unmap_resource(uio_res->maps[i].addr,
 				(size_t)uio_res->maps[i].size);
-		rte_free(uio_res->maps[i].path);
+
+		if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+			rte_free(uio_res->maps[i].path);
 	}
 }
 
-- 
2.7.4

Re: [PATCH] eal: Fix wrong resource release of pci_uio_unmap()

[David Marchand]

On Wed, Jun 15, 2016 at 5:27 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> The 'path' member of mapped_pci_resource structure is allocated by
> primary process, but currenctly it will be freed by both primary
> and secondary process.
> The patch fixes to be freed by only primary process.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

Function name in title.

> ---
>  lib/librte_eal/common/eal_common_pci_uio.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
> index e718643..b2c68f3 100644
> --- a/lib/librte_eal/common/eal_common_pci_uio.c
> +++ b/lib/librte_eal/common/eal_common_pci_uio.c
> @@ -162,7 +162,9 @@ pci_uio_unmap(struct mapped_pci_resource *uio_res)
>         for (i = 0; i != uio_res->nb_maps; i++) {
>                 pci_unmap_resource(uio_res->maps[i].addr,
>                                 (size_t)uio_res->maps[i].size);
> -               rte_free(uio_res->maps[i].path);
> +
> +               if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +                       rte_free(uio_res->maps[i].path);
>         }
>  }
>

The rest looks good to me.

-- 
David Marchand

[PATCH v2] eal: Fix wrong resource release while unmapping pci devices

[Tetsuya Mukawa]

This patch fixes wrong resource release of pci_uio_unmap().
The 'path' member of mapped_pci_resource structure is allocated by
primary process, but currently it will be freed by both primary
and secondary process.
The patch fixes to be freed by only primary process.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_eal/common/eal_common_pci_uio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci_uio.c b/lib/librte_eal/common/eal_common_pci_uio.c
index f062e81..488b6dd 100644
--- a/lib/librte_eal/common/eal_common_pci_uio.c
+++ b/lib/librte_eal/common/eal_common_pci_uio.c
@@ -159,7 +159,9 @@ pci_uio_unmap(struct mapped_pci_resource *uio_res)
 	for (i = 0; i != uio_res->nb_maps; i++) {
 		pci_unmap_resource(uio_res->maps[i].addr,
 				(size_t)uio_res->maps[i].size);
-		rte_free(uio_res->maps[i].path);
+
+		if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+			rte_free(uio_res->maps[i].path);
 	}
 }
 
-- 
2.7.4

Re: [PATCH v2] eal: Fix wrong resource release while unmapping pci devices

[David Marchand]

On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> This patch fixes wrong resource release of pci_uio_unmap().
> The 'path' member of mapped_pci_resource structure is allocated by
> primary process, but currently it will be freed by both primary
> and secondary process.
> The patch fixes to be freed by only primary process.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

Wrong headline uppercase:
    eal: Fix wrong resource release while unmapping pci devices
Wrong headline lowercase:
    eal: Fix wrong resource release while unmapping pci devices
Missing 'Fixes' tag:
    eal: Fix wrong resource release while unmapping pci devices

Then you can add my ack.


-- 
David Marchand

Re: [PATCH v2] eal: Fix wrong resource release while unmapping pci devices

[Thomas Monjalon]

2016-06-17 14:12, David Marchand:
> On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> > This patch fixes wrong resource release of pci_uio_unmap().
> > The 'path' member of mapped_pci_resource structure is allocated by
> > primary process, but currently it will be freed by both primary
> > and secondary process.
> > The patch fixes to be freed by only primary process.
> >
> > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> 
> Wrong headline uppercase:
>     eal: Fix wrong resource release while unmapping pci devices
> Wrong headline lowercase:
>     eal: Fix wrong resource release while unmapping pci devices
> Missing 'Fixes' tag:
>     eal: Fix wrong resource release while unmapping pci devices
> 
> Then you can add my ack.

Applied, thanks

Re: [PATCH v2] eal: Fix wrong error checking while parsing device arguments

[David Marchand]

On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> This patch fixes wrong error checking of rte_eal_parse_devargs_str().
> Currently, a return value of strdup() is wrongly checked.
>
> Fixes: 0fe11ec592b2 ("eal: add vdev init and uninit")
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>

Forgot to pass this patch through scripts/check-git-log.sh script :

Wrong headline uppercase:
    eal: Fix wrong error checking while parsing device arguments


With this fixed, you can add my ack.

-- 
David Marchand

Re: [PATCH v2] eal: Fix wrong error checking while parsing device arguments

[Thomas Monjalon]

2016-06-17 14:10, David Marchand:
> On Thu, Jun 16, 2016 at 4:33 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
> > This patch fixes wrong error checking of rte_eal_parse_devargs_str().
> > Currently, a return value of strdup() is wrongly checked.
> >
> > Fixes: 0fe11ec592b2 ("eal: add vdev init and uninit")
> > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> 
> Forgot to pass this patch through scripts/check-git-log.sh script :
> 
> Wrong headline uppercase:
>     eal: Fix wrong error checking while parsing device arguments
> 
> 
> With this fixed, you can add my ack.

Applied, thanks