[dpdk-dev] [PATCH 0/4] Coverity issue fixes

[dpdk-dev] [PATCH 0/4] Coverity issue fixes

[Stephen Hemminger]

These are all error path issues and should not matter in
a real application; most of the error are impossible to cause
and applications just fail if setup fails.

Stephen Hemminger (4):
  net/failsafe: avoid rte_memcpy if rte_realloc fails
  bus/vmbus: fix directory handle leak on error
  net/tap: fix file descriptor leak on error
  net/tap: fix warning about comparison of fd

 drivers/bus/vmbus/linux/vmbus_uio.c | 12 +++++++-----
 drivers/net/failsafe/failsafe_ops.c |  2 +-
 drivers/net/tap/rte_eth_tap.c       |  2 +-
 drivers/net/tap/tap_netlink.c       |  3 +++
 4 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/4] net/failsafe: avoid rte_memcpy if rte_realloc fails

[Stephen Hemminger]

There is a potential issue seen by static tools if number of multicast
addresses is zero, and rte_realloc of zero size fails (ie returns NULL).
This won't happen in real world for a couple of reasons: Azure doesn't
support multicast (ie this is dead code); and rte_realloc of zero size
will never fail, but safe to just always return -ENOMEM of realloc fails.

Coverity issue: 323487
Fixes: 901efc0da925 ("net/failsafe: support multicast address list set")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/failsafe/failsafe_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 7f8bcd4c69f4..a20953a662e1 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -1155,7 +1155,7 @@ fs_set_mc_addr_list(struct rte_eth_dev *dev,
 
 	mcast_addrs = rte_realloc(PRIV(dev)->mcast_addrs,
 		nb_mc_addr * sizeof(PRIV(dev)->mcast_addrs[0]), 0);
-	if (mcast_addrs == NULL && nb_mc_addr > 0) {
+	if (mcast_addrs == NULL) {
 		ret = -ENOMEM;
 		goto rollback;
 	}
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/4] net/failsafe: avoid rte_memcpy if rte_realloc fails

[Andrew Rybchenko]

On 11/6/18 10:30 PM, Stephen Hemminger wrote:
> There is a potential issue seen by static tools if number of multicast
> addresses is zero, and rte_realloc of zero size fails (ie returns NULL).
> This won't happen in real world for a couple of reasons: Azure doesn't
> support multicast (ie this is dead code);

Is it guaranteed that Azure is the only user of the code?
Sorry, it does not sound like an argument at all.

> and rte_realloc of zero size
> will never fail, but safe to just always return -ENOMEM of realloc fails.

It is false statement. If ptr is NULL, rte_malloc() is used which explicitly
returns NULL if size is 0.

> Coverity issue: 323487

It is 100% false alarm from Coverity. rte_memcpy() does nothing
if size is 0 and it is zero if number of addresses is zero.
If we really want to cope with it (I'm not sure), just do rte_memcpy()
in else branch. And it should explained in the comment that it is
required to address false issue from static analysis tool only.

Other option is to add check for dummy set (zero number of addresses
when it is already zero, but it is extra lines of code and extra logic which
is not actually required here. So, more harm from my point of view.

> Fixes: 901efc0da925 ("net/failsafe: support multicast address list set")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/failsafe/failsafe_ops.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index 7f8bcd4c69f4..a20953a662e1 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -1155,7 +1155,7 @@ fs_set_mc_addr_list(struct rte_eth_dev *dev,
>   
>   	mcast_addrs = rte_realloc(PRIV(dev)->mcast_addrs,
>   		nb_mc_addr * sizeof(PRIV(dev)->mcast_addrs[0]), 0);
> -	if (mcast_addrs == NULL && nb_mc_addr > 0) {
> +	if (mcast_addrs == NULL) {
>   		ret = -ENOMEM;
>   		goto rollback;
>   	}

Re: [dpdk-dev] [PATCH 1/4] net/failsafe: avoid rte_memcpy if rte_realloc fails

[Stephen Hemminger]

On Wed, 7 Nov 2018 09:30:13 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 11/6/18 10:30 PM, Stephen Hemminger wrote:
> > There is a potential issue seen by static tools if number of multicast
> > addresses is zero, and rte_realloc of zero size fails (ie returns NULL).
> > This won't happen in real world for a couple of reasons: Azure doesn't
> > support multicast (ie this is dead code);  
> 
> Is it guaranteed that Azure is the only user of the code?
> Sorry, it does not sound like an argument at all.

There are no guarantees in life...

Failsafe in DPDK is probably only useful with Azure.
AWS, GCP, and other clouds handle virtual networking differently.
There has been some talk of doing similar things with KVM, but the device
model is different there. Failsafe makes some assumptions about device
layering that are specific to Azure. It also has some generalizations about
cases that will never matter.

Vdev_netvsc is definitely Azure specific. It will be deprecated once
netvsc PMD is more widely supported.


> 
> > and rte_realloc of zero size
> > will never fail, but safe to just always return -ENOMEM of realloc fails.  
> 
> It is false statement. If ptr is NULL, rte_malloc() is used which explicitly
> returns NULL if size is 0.
> 
> > Coverity issue: 323487  
> 
> It is 100% false alarm from Coverity. rte_memcpy() does nothing
> if size is 0 and it is zero if number of addresses is zero.
> If we really want to cope with it (I'm not sure), just do rte_memcpy()
> in else branch. And it should explained in the comment that it is
> required to address false issue from static analysis tool only.
> 
> Other option is to add check for dummy set (zero number of addresses
> when it is already zero, but it is extra lines of code and extra logic which
> is not actually required here. So, more harm from my point of view.

We are in agreement, it is a false alarm because:
	* Coverity assumes that memcpy and rte_memcpy have same semantics.
	* the case can't never happen

Any solution is fine because of that. You could flag it as false in Coverity
or change code to avoid warning. Just want to get it fixed, don't care how.

Re: [dpdk-dev] [PATCH 1/4] net/failsafe: avoid rte_memcpy if rte_realloc fails

[Andrew Rybchenko]

On 11/7/18 9:15 PM, Stephen Hemminger wrote:
> Any solution is fine because of that. You could flag it as false in Coverity
> or change code to avoid warning. Just want to get it fixed, don't care how.

Done, marked it as false positive in Coverity and provided explanation why.

Andrew.

[dpdk-dev] [PATCH 2/4] bus/vmbus: fix directory handle leak on error

[Stephen Hemminger]

If sysfs directory was incorrectly formatted then the vmbus
setup code would leak a directory handle in the error path.

Coverity issue: 302848
Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/vmbus/linux/vmbus_uio.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/vmbus/linux/vmbus_uio.c b/drivers/bus/vmbus/linux/vmbus_uio.c
index 856c6d66785d..12e97e3a420a 100644
--- a/drivers/bus/vmbus/linux/vmbus_uio.c
+++ b/drivers/bus/vmbus/linux/vmbus_uio.c
@@ -329,6 +329,7 @@ int vmbus_uio_get_subchan(struct vmbus_channel *primary,
 	char chan_path[PATH_MAX], subchan_path[PATH_MAX];
 	struct dirent *ent;
 	DIR *chan_dir;
+	int err;
 
 	snprintf(chan_path, sizeof(chan_path),
 		 "%s/%s/channels",
@@ -344,7 +345,6 @@ int vmbus_uio_get_subchan(struct vmbus_channel *primary,
 	while ((ent = readdir(chan_dir))) {
 		unsigned long relid, subid, monid;
 		char *endp;
-		int err;
 
 		if (ent->d_name[0] == '.')
 			continue;
@@ -364,8 +364,7 @@ int vmbus_uio_get_subchan(struct vmbus_channel *primary,
 		if (err) {
 			VMBUS_LOG(NOTICE, "invalid subchannel id %lu",
 				  subid);
-			closedir(chan_dir);
-			return err;
+			goto fail;
 		}
 
 		if (subid == 0)
@@ -382,17 +381,20 @@ int vmbus_uio_get_subchan(struct vmbus_channel *primary,
 		if (err) {
 			VMBUS_LOG(NOTICE, "invalid monitor id %lu",
 				  monid);
-			return err;
+			goto fail;
 		}
 
 		err = vmbus_chan_create(dev, relid, subid, monid, subchan);
 		if (err) {
 			VMBUS_LOG(NOTICE, "subchannel setup failed");
-			return err;
+			goto fail;
 		}
 		break;
 	}
 	closedir(chan_dir);
 
 	return (ent == NULL) ? -ENOENT : 0;
+fail:
+	closedir(chan_dir);
+	return err;
 }
-- 
2.17.1

[dpdk-dev] [PATCH 3/4] net/tap: fix file descriptor leak on error

[Stephen Hemminger]

If netlink socket setup fails the file descriptor was leaked.

Coverity issue: 257040
Fixes: 7c25284e30c2 ("net/tap: add netlink back-end for flow API")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_netlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/tap/tap_netlink.c b/drivers/net/tap/tap_netlink.c
index 6cb510092218..14bbbec754f6 100644
--- a/drivers/net/tap/tap_netlink.c
+++ b/drivers/net/tap/tap_netlink.c
@@ -51,14 +51,17 @@ tap_nl_init(uint32_t nl_groups)
 	}
 	if (setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &sndbuf_size, sizeof(int))) {
 		TAP_LOG(ERR, "Unable to set socket buffer send size");
+		close(fd);
 		return -1;
 	}
 	if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf_size, sizeof(int))) {
 		TAP_LOG(ERR, "Unable to set socket buffer receive size");
+		close(fd);
 		return -1;
 	}
 	if (bind(fd, (struct sockaddr *)&local, sizeof(local)) < 0) {
 		TAP_LOG(ERR, "Unable to bind to the netlink socket");
+		close(fd);
 		return -1;
 	}
 	return fd;
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/4] net/tap: fix file descriptor leak on error

[Wiles, Keith]

> On Nov 6, 2018, at 7:30 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> If netlink socket setup fails the file descriptor was leaked.

Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> Coverity issue: 257040
> Fixes: 7c25284e30c2 ("net/tap: add netlink back-end for flow API")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/tap_netlink.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/tap/tap_netlink.c b/drivers/net/tap/tap_netlink.c
> index 6cb510092218..14bbbec754f6 100644
> --- a/drivers/net/tap/tap_netlink.c
> +++ b/drivers/net/tap/tap_netlink.c
> @@ -51,14 +51,17 @@ tap_nl_init(uint32_t nl_groups)
> 	}
> 	if (setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &sndbuf_size, sizeof(int))) {
> 		TAP_LOG(ERR, "Unable to set socket buffer send size");
> +		close(fd);
> 		return -1;
> 	}
> 	if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf_size, sizeof(int))) {
> 		TAP_LOG(ERR, "Unable to set socket buffer receive size");
> +		close(fd);
> 		return -1;
> 	}
> 	if (bind(fd, (struct sockaddr *)&local, sizeof(local)) < 0) {
> 		TAP_LOG(ERR, "Unable to bind to the netlink socket");
> +		close(fd);
> 		return -1;
> 	}
> 	return fd;
> -- 
> 2.17.1
> 

Regards,
Keith

[dpdk-dev] [PATCH 4/4] net/tap: fix warning about comparison of fd

[Stephen Hemminger]

Static analysis tools don't like the fact that fd could be zero
in the error path. This won't happen in real world because
stdin would have to be closed, then other error occurring.

Coverity issue: 14079
Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 84aaf241019a..f7087222b319 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -231,7 +231,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
 	return fd;
 
 error:
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 	return -1;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH 4/4] net/tap: fix warning about comparison of fd

[Wiles, Keith]

> On Nov 6, 2018, at 7:30 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> Static analysis tools don't like the fact that fd could be zero
> in the error path. This won't happen in real world because
> stdin would have to be closed, then other error occurring.

Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> Coverity issue: 14079
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 84aaf241019a..f7087222b319 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -231,7 +231,7 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive)
> 	return fd;
> 
> error:
> -	if (fd > 0)
> +	if (fd >= 0)
> 		close(fd);
> 	return -1;
> }
> -- 
> 2.17.1
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH 0/4] Coverity issue fixes

[Thomas Monjalon]

06/11/2018 20:30, Stephen Hemminger:
> These are all error path issues and should not matter in
> a real application; most of the error are impossible to cause
> and applications just fail if setup fails.
> 
> Stephen Hemminger (4):
>   net/failsafe: avoid rte_memcpy if rte_realloc fails
>   bus/vmbus: fix directory handle leak on error
>   net/tap: fix file descriptor leak on error
>   net/tap: fix warning about comparison of fd

Series applied without failsafe patch.