[dpdk-dev] [RFC 0/5] more Coverity related bug fixes

[dpdk-dev] [RFC 0/5] more Coverity related bug fixes

[Stephen Hemminger]

There are some real bugs in here, not just noise.
Labeling this version as RFC since compile tested only.

Stephen Hemminger (5):
  bus/pci: fix allocation of pci device path
  bus/pci: fix TOCTOU issue
  bpf: fix validation of eal_divmod
  eal/memory: avoid double munmap in error path
  pipeline: remove dead code

 drivers/bus/pci/linux/pci_uio.c          | 40 +++++++++++-------------
 lib/librte_bpf/bpf_validate.c            |  2 +-
 lib/librte_eal/linuxapp/eal/eal_memory.c |  1 +
 lib/librte_pipeline/rte_table_action.c   |  2 --
 4 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.17.1

[dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path

[Stephen Hemminger]

The pci_resource_by_index called strlen() on uninitialized
memory which would lead to the wrong size of memory allocated
for the path portion of the resource map. This would either cause
excessively large allocation, or worse memory corruption.

Coverity Issue: 300868
Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index a7c14421aa79..112ac51dddcc 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -295,14 +295,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	loc = &dev->addr;
 	maps = uio_res->maps;
 
-	/* allocate memory to keep path */
-	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
-	if (maps[map_idx].path == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
-				strerror(errno));
-		return -1;
-	}
-
 	/*
 	 * open resource file, to mmap it
 	 */
@@ -335,10 +327,19 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		if (fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				devname, strerror(errno));
-			goto error;
+			return -1;
 		}
 	}
 
+	/* allocate memory to keep path */
+	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+	if (maps[map_idx].path == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
+				strerror(errno));
+		close(fd);
+		return -1;
+	}
+
 	/* try mapping somewhere close to the end of hugepages */
 	if (pci_map_addr == NULL)
 		pci_map_addr = pci_find_max_end_va();
@@ -346,8 +347,10 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	mapaddr = pci_map_resource(pci_map_addr, fd, 0,
 			(size_t)dev->mem_resource[res_idx].len, 0);
 	close(fd);
-	if (mapaddr == MAP_FAILED)
-		goto error;
+	if (mapaddr == MAP_FAILED) {
+		rte_free(maps[map_idx].path);
+		return -1;
+	}
 
 	pci_map_addr = RTE_PTR_ADD(mapaddr,
 			(size_t)dev->mem_resource[res_idx].len);
@@ -360,10 +363,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	dev->mem_resource[res_idx].addr = mapaddr;
 
 	return 0;
-
-error:
-	rte_free(maps[map_idx].path);
-	return -1;
 }
 
 #if defined(RTE_ARCH_X86)
-- 
2.17.1

Re: [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path

[Thomas Monjalon]

06/11/2018 22:48, Stephen Hemminger:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

This patch is RFC but Ferruh (UIO maintainer) was not Cc'ed.
I feel this bug is critical. Please advise.

Re: [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path

[Ferruh Yigit]

On 11/6/2018 9:48 PM, Stephen Hemminger wrote:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.

Yes this may corrupt memory, I wonder how nobody hit this. I am for including
the fix for the release.

> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421aa79..112ac51dddcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -295,14 +295,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	loc = &dev->addr;
>  	maps = uio_res->maps;
>  
> -	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> -	if (maps[map_idx].path == NULL) {
> -		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
> -				strerror(errno));
> -		return -1;
> -	}

What about simply:

-       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+       maps[map_idx].path = rte_malloc(NULL, sizeof(devname) + 1, 0);

[dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path

[Ferruh Yigit]

The pci_resource_by_index called strlen() on uninitialized
memory which would lead to the wrong size of memory allocated
for the path portion of the resource map. This would either cause
excessively large allocation, or worse memory corruption.

Coverity Issue: 300868
Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index a7c14421a..09ecbb7aa 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	maps = uio_res->maps;
 
 	/* allocate memory to keep path */
-	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
 	if (maps[map_idx].path == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
 				strerror(errno));
-- 
2.17.2

Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path

[Thomas Monjalon]

Please, anyone for a review and a test?

23/11/2018 01:29, Ferruh Yigit:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421a..09ecbb7aa 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	maps = uio_res->maps;
>  
>  	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> +	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
>  	if (maps[map_idx].path == NULL) {
>  		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
>  				strerror(errno));
> 

Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path

[Andrew Rybchenko]

On 11/23/18 1:45 PM, Thomas Monjalon wrote:
> Please, anyone for a review and a test?
>
> 23/11/2018 01:29, Ferruh Yigit:
>> The pci_resource_by_index called strlen() on uninitialized
>> memory which would lead to the wrong size of memory allocated
>> for the path portion of the resource map. This would either cause
>> excessively large allocation, or worse memory corruption.
>>
>> Coverity Issue: 300868
>> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path

[Maxime Coquelin]

On 11/23/18 1:29 AM, Ferruh Yigit wrote:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>   drivers/bus/pci/linux/pci_uio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421a..09ecbb7aa 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>   	maps = uio_res->maps;
>   
>   	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> +	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
>   	if (maps[map_idx].path == NULL) {
>   		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
>   				strerror(errno));
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path

[Thomas Monjalon]

23/11/2018 12:01, Maxime Coquelin:
> On 11/23/18 1:29 AM, Ferruh Yigit wrote:
> > The pci_resource_by_index called strlen() on uninitialized
> > memory which would lead to the wrong size of memory allocated
> > for the path portion of the resource map. This would either cause
> > excessively large allocation, or worse memory corruption.
> > 
> > Coverity Issue: 300868
> > Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied, thanks

[dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue

[Stephen Hemminger]

Using access followed by open causes a static analysis warning
about Time of check versus Time of use. Also, access() and
open() have different UID permission checks.

This is not a serious problem; but easy to fix by using errno instead.

Coverity issue: 300870
Fixes: 4a928ef9f611 ("bus/pci: enable write combining during mapping")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/pci/linux/pci_uio.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 112ac51dddcc..8521fe63b0ae 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -306,12 +306,11 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 			loc->domain, loc->bus, loc->devid,
 			loc->function, res_idx);
 
-		if (access(devname, R_OK|W_OK) != -1) {
-			fd = open(devname, O_RDWR);
-			if (fd < 0)
-				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
-					"Fall-back to non prefetchable mode.\n",
-					devname);
+		fd = open(devname, O_RDWR);
+		if (fd < 0 && errno != ENOENT) {
+			RTE_LOG(INFO, EAL, "%s cannot be mapped. "
+				"Fall-back to non prefetchable mode.\n",
+				devname);
 		}
 	}
 
-- 
2.17.1

Re: [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue

[Thomas Monjalon]

+Cc Ferruh

06/11/2018 22:48, Stephen Hemminger:
> Using access followed by open causes a static analysis warning
> about Time of check versus Time of use. Also, access() and
> open() have different UID permission checks.
> 
> This is not a serious problem; but easy to fix by using errno instead.
> 
> Coverity issue: 300870
> Fixes: 4a928ef9f611 ("bus/pci: enable write combining during mapping")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 112ac51dddcc..8521fe63b0ae 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -306,12 +306,11 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  			loc->domain, loc->bus, loc->devid,
>  			loc->function, res_idx);
>  
> -		if (access(devname, R_OK|W_OK) != -1) {
> -			fd = open(devname, O_RDWR);
> -			if (fd < 0)
> -				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
> -					"Fall-back to non prefetchable mode.\n",
> -					devname);
> +		fd = open(devname, O_RDWR);
> +		if (fd < 0 && errno != ENOENT) {
> +			RTE_LOG(INFO, EAL, "%s cannot be mapped. "
> +				"Fall-back to non prefetchable mode.\n",
> +				devname);
>  		}
>  	}

[dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod

[Stephen Hemminger]

Coverity spotted self assignment in BPF eval_divmod.
This looks like a bug where the incoming source register
should have been used instead.

Coverity issue: 302850
Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_bpf/bpf_validate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
index 83983efc4e5c..b768f72c4c02 100644
--- a/lib/librte_bpf/bpf_validate.c
+++ b/lib/librte_bpf/bpf_validate.c
@@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
 		if (op == BPF_MOD)
 			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
 		else
-			rd->u.max = rd->u.max;
+			rd->u.max = rs->u.max;
 		rd->u.min = 0;
 	}
 
-- 
2.17.1

Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Tuesday, November 6, 2018 9:49 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> 
> Coverity spotted self assignment in BPF eval_divmod.

Yep, there is one.
As I remember I have to add it because one of old versions 
of compiler (clang???) complained about 'variable being used uninitialized'.

> This looks like a bug where the incoming source register
> should have been used instead.

Nope, that's a wrong guess.
We shouldn't do it here.
Konstantin

> 
> Coverity issue: 302850
> Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_bpf/bpf_validate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> index 83983efc4e5c..b768f72c4c02 100644
> --- a/lib/librte_bpf/bpf_validate.c
> +++ b/lib/librte_bpf/bpf_validate.c
> @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
>  		if (op == BPF_MOD)
>  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
>  		else
> -			rd->u.max = rd->u.max;
> +			rd->u.max = rs->u.max;
>  		rd->u.min = 0;
>  	}
> 
> --
> 2.17.1

Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod

[Stephen Hemminger]

On Wed, 7 Nov 2018 12:54:54 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Tuesday, November 6, 2018 9:49 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> > 
> > Coverity spotted self assignment in BPF eval_divmod.  
> 
> Yep, there is one.
> As I remember I have to add it because one of old versions 
> of compiler (clang???) complained about 'variable being used uninitialized'.
> 
> > This looks like a bug where the incoming source register
> > should have been used instead.  
> 
> Nope, that's a wrong guess.
> We shouldn't do it here.
> Konstantin
> 
> > 
> > Coverity issue: 302850
> > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_bpf/bpf_validate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > index 83983efc4e5c..b768f72c4c02 100644
> > --- a/lib/librte_bpf/bpf_validate.c
> > +++ b/lib/librte_bpf/bpf_validate.c
> > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> >  		if (op == BPF_MOD)
> >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> >  		else
> > -			rd->u.max = rd->u.max;
> > +			rd->u.max = rs->u.max;
> >  		rd->u.min = 0;
> >  	}
> > 
> > --
> > 2.17.1  
> 

Well it was being used unintialized, your trick of self assignment fooled clang
but did not fool Coverity.  What does the other BPF validator do?

Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod

[Ananyev, Konstantin]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, November 7, 2018 7:52 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> 
> On Wed, 7 Nov 2018 12:54:54 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Tuesday, November 6, 2018 9:49 PM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> > >
> > > Coverity spotted self assignment in BPF eval_divmod.
> >
> > Yep, there is one.
> > As I remember I have to add it because one of old versions
> > of compiler (clang???) complained about 'variable being used uninitialized'.
> >
> > > This looks like a bug where the incoming source register
> > > should have been used instead.
> >
> > Nope, that's a wrong guess.
> > We shouldn't do it here.
> > Konstantin
> >
> > >
> > > Coverity issue: 302850
> > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  lib/librte_bpf/bpf_validate.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > index 83983efc4e5c..b768f72c4c02 100644
> > > --- a/lib/librte_bpf/bpf_validate.c
> > > +++ b/lib/librte_bpf/bpf_validate.c
> > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > >  		if (op == BPF_MOD)
> > >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > >  		else
> > > -			rd->u.max = rd->u.max;
> > > +			rd->u.max = rs->u.max;
> > >  		rd->u.min = 0;
> > >  	}
> > >
> > > --
> > > 2.17.1
> >
> 
> Well it was being used unintialized, 

I don't think so, but if you can point to me where
exactly it is used uninitialized, we can discuss it further.

> your trick of self assignment fooled clang

It was one particular and pretty old version of clang
(if my memory serves me right).
With latest versions (let say 6.0) it doesn't complain,
if I remove that self-assignment.
gcc also doesn't see any problem here.
That makes me think it was a false-positive with old
version of the compiler.
Konstantin 

> but did not fool Coverity.  What does the other BPF validator do?

Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod

[Ananyev, Konstantin]

> > > >
> > > > Coverity spotted self assignment in BPF eval_divmod.
> > >
> > > Yep, there is one.
> > > As I remember I have to add it because one of old versions
> > > of compiler (clang???) complained about 'variable being used uninitialized'.
> > >
> > > > This looks like a bug where the incoming source register
> > > > should have been used instead.
> > >
> > > Nope, that's a wrong guess.
> > > We shouldn't do it here.
> > > Konstantin
> > >
> > > >
> > > > Coverity issue: 302850
> > > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > >  lib/librte_bpf/bpf_validate.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > > index 83983efc4e5c..b768f72c4c02 100644
> > > > --- a/lib/librte_bpf/bpf_validate.c
> > > > +++ b/lib/librte_bpf/bpf_validate.c
> > > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > > >  		if (op == BPF_MOD)
> > > >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > > >  		else
> > > > -			rd->u.max = rd->u.max;
> > > > +			rd->u.max = rs->u.max;
> > > >  		rd->u.min = 0;
> > > >  	}
> > > >
> > > > --
> > > > 2.17.1
> > >
> >
> > Well it was being used unintialized,
> 
> I don't think so, but if you can point to me where
> exactly it is used uninitialized, we can discuss it further.
> 
> > your trick of self assignment fooled clang
> 
> It was one particular and pretty old version of clang
> (if my memory serves me right).
> With latest versions (let say 6.0) it doesn't complain,
> if I remove that self-assignment.
> gcc also doesn't see any problem here.
> That makes me think it was a false-positive with old
> version of the compiler.
> Konstantin

As a another thought - it wouldn't take much effort to 
send a patch with NOP self-assignment removed.
If it will pass our build-harness test, then it is probably
ok to integrate.
Konstantin

> 
> > but did not fool Coverity.  What does the other BPF validator do?

[dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path

[Stephen Hemminger]

Don't call munmap of hugepage memory twice in the error path.

Coverity issue: 325730
Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index c1b5e079117a..48b23ce19ad3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1617,6 +1617,7 @@ eal_legacy_hugepage_init(void)
 	tmp_hp = NULL;
 
 	munmap(hugepage, nr_hugefiles * sizeof(struct hugepage_file));
+	hugepage = NULL;
 
 	/* we're not going to allocate more pages, so release VA space for
 	 * unused memseg lists
-- 
2.17.1

Re: [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path

[Thomas Monjalon]

06/11/2018 22:49, Stephen Hemminger:
> Don't call munmap of hugepage memory twice in the error path.
> 
> Coverity issue: 325730
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Thank you
Anatoly submitted the same fix:
	https://patches.dpdk.org/patch/47898/

[dpdk-dev] [RFC 5/5] pipeline: remove dead code

[Stephen Hemminger]

Coverity detected dead code since pointer into a global
array can never be NULL.

Coverity issue: 323523
Fixes: 96303217a606 ("pipeline: add symmetric crypto table action")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_pipeline/rte_table_action.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_pipeline/rte_table_action.c b/lib/librte_pipeline/rte_table_action.c
index 537e6593e4a0..69ab41a7ab2d 100644
--- a/lib/librte_pipeline/rte_table_action.c
+++ b/lib/librte_pipeline/rte_table_action.c
@@ -1696,8 +1696,6 @@ get_block_size(const struct rte_crypto_sym_xform *xform, uint8_t cdev_id)
 
 	for (i = 0;; i++) {
 		cap = &dev_info.capabilities[i];
-		if (!cap)
-			break;
 
 		if (cap->sym.xform_type != xform->type)
 			continue;
-- 
2.17.1

Re: [dpdk-dev] [RFC 5/5] pipeline: remove dead code

[Thomas Monjalon]

+Cc Cristian

06/11/2018 22:49, Stephen Hemminger:
> Coverity detected dead code since pointer into a global
> array can never be NULL.
> 
> Coverity issue: 323523
> Fixes: 96303217a606 ("pipeline: add symmetric crypto table action")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>