[dpdk-dev] [PATCH 0/5] some small fixes

[dpdk-dev] [PATCH 0/5] some small fixes

[Bruce Richardson]

A few coverity fixes, along with a fix for one of the examples meson.build
files which I caught along the way.

Bruce Richardson (5):
  examples/vhost_scsi: fix header check for meson build
  examples/vhost_scsi: fix missing NULL-check for parameter
  net/i40e: fix dereference before NULL check in mbuf release
  app/testpmd: fix variable use before NULL check
  net/i40e: fix dereference before check when getting EEPROM

 app/test-pmd/cmdline.c           | 4 ++--
 drivers/net/i40e/i40e_ethdev.c   | 5 +++--
 drivers/net/i40e/i40e_rxtx.c     | 4 ++--
 examples/vhost_scsi/meson.build  | 2 +-
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 5 files changed, 14 insertions(+), 7 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/5] examples/vhost_scsi: fix header check for meson build

[Bruce Richardson]

The header check for the example app was looking for virtio_scsi.h without
the "linux/" prefix, which meant it was never getting found when it should
have been.

Fixes: 8d47a753b7cb ("examples/vhost_scsi: disable build if missing dependency")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vhost_scsi/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/vhost_scsi/meson.build b/examples/vhost_scsi/meson.build
index ca1248023..e00a7dab1 100644
--- a/examples/vhost_scsi/meson.build
+++ b/examples/vhost_scsi/meson.build
@@ -10,7 +10,7 @@ if host_machine.system() != 'linux'
 	build = false
 endif
 
-if not cc.has_header('virtio_scsi.h')
+if not cc.has_header('linux/virtio_scsi.h')
 	build = false
 endif
 
-- 
2.20.1

[dpdk-dev] [PATCH 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Bruce Richardson]

Coverity points out that there is a check in the main thread loop for the
ctrlr->bdev being NULL, but by that stage the pointer has already been
dereferenced. Therefore, for safety, before we enter the loop do an
initial check on the parameter structure.

Coverity issue: 158657
Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index 2908ff68b..a6465d089 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -285,6 +285,12 @@ ctrlr_worker(void *arg)
 	cpu_set_t cpuset;
 	pthread_t thread;
 
+	if (ctrlr == NULL || ctrlr->bdev == NULL) {
+		fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n",
+				__func__);
+		return NULL;
+	}
+
 	thread = pthread_self();
 	CPU_ZERO(&cpuset);
 	CPU_SET(0, &cpuset);
-- 
2.20.1

[dpdk-dev] [PATCH 3/5] net/i40e: fix dereference before NULL check in mbuf release

[Bruce Richardson]

Coverity flags that the txq variable is used before it's checked for NULL.

Coverity issue: 195023

Fixes: 24853544c84c ("net/i40e: fix mbuf free in vector Tx")
Cc: qi.z.zhang@intel.com
CC: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 8f727fae6..5ce85eb52 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2423,13 +2423,13 @@ i40e_tx_queue_release_mbufs(struct i40e_tx_queue *txq)
 	struct rte_eth_dev *dev;
 	uint16_t i;
 
-	dev = &rte_eth_devices[txq->port_id];
-
 	if (!txq || !txq->sw_ring) {
 		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
 		return;
 	}
 
+	dev = &rte_eth_devices[txq->port_id];
+
 	/**
 	 *  vPMD tx will not set sw_ring's mbuf to NULL after free,
 	 *  so need to free remains more carefully.
-- 
2.20.1

Re: [dpdk-dev] [PATCH 3/5] net/i40e: fix dereference before NULL check in mbuf release

[Rami Rosen]

Acked-By: Rami Rosen <ramirose@gmail.com>

In this occasion I noticed that there is a typo in the code that appears in
the patch;
it should be "Pointer to txq" instead of "Pointer to rxq":
I would consider resending and fixing it on this occasion:

>  if (!txq || !txq->sw_ring) {
>               PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
>                return;
>        }






‏<bruce.richardson@intel.com>:

> Coverity flags that the txq variable is used before it's checked for NULL.
>
> Coverity issue: 195023
>
> Fixes: 24853544c84c ("net/i40e: fix mbuf free in vector Tx")
> Cc: qi.z.zhang@intel.com
> CC: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 8f727fae6..5ce85eb52 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -2423,13 +2423,13 @@ i40e_tx_queue_release_mbufs(struct i40e_tx_queue
> *txq)
>         struct rte_eth_dev *dev;
>         uint16_t i;
>
> -       dev = &rte_eth_devices[txq->port_id];
> -
>         if (!txq || !txq->sw_ring) {
>                 PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
>                 return;
>         }
>
> +       dev = &rte_eth_devices[txq->port_id];
> +
>         /**
>          *  vPMD tx will not set sw_ring's mbuf to NULL after free,
>          *  so need to free remains more carefully.
> --
> 2.20.1
>
>

[dpdk-dev] [PATCH 4/5] app/testpmd: fix variable use before NULL check

[Bruce Richardson]

The value returned from rte_eth_dev_tx_offload_name() function is used
for string comparison before being checked for NULL. Move the NULL check
up to be done first.

Coverity issue: 279438
Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
Cc: wei.dai@intel.com
Cc: stable@dpdk.org
Cc: Bernard Iremonger <bernard.iremonger@intel.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test-pmd/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index ee50e4566..2155ff241 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18306,13 +18306,13 @@ search_tx_offload(const char *name)
 	single_offload = 1;
 	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
 		single_name = rte_eth_dev_tx_offload_name(single_offload);
+		if (single_name == NULL)
+			break;
 		if (!strcasecmp(single_name, name)) {
 			found = 1;
 			break;
 		} else if (!strcasecmp(single_name, "UNKNOWN"))
 			break;
-		else if (single_name == NULL)
-			break;
 		single_offload <<= 1;
 	}
 
-- 
2.20.1

Re: [dpdk-dev] [PATCH 4/5] app/testpmd: fix variable use before NULL check

[Rami Rosen]

Acked-by: Rami Rosen <ramirose@gmail.com>

[dpdk-dev] [PATCH 5/5] net/i40e: fix dereference before check when getting EEPROM

[Bruce Richardson]

As flagged by coverity, the "info" structure is being explicitly
dereferenced before being checked later for a NULL value.

Coverity issue: 277241
Fixes: 98e60c0d43f1 ("net/i40e: add module EEPROM callbacks for i40e")
CC: stable@dpdk.org
Cc: zijie.pan@6wind.com
CC: Beilei Xing <beilei.xing@intel.com>
CC: Qi Zhang <qi.z.zhang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index b031bf4c6..6450af016 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -11893,16 +11893,17 @@ static int i40e_get_module_eeprom(struct rte_eth_dev *dev,
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	bool is_sfp = false;
 	i40e_status status;
-	uint8_t *data = info->data;
+	uint8_t *data;
 	uint32_t value = 0;
 	uint32_t i;
 
-	if (!info || !info->length || !data)
+	if (!info || !info->length || !info->data)
 		return -EINVAL;
 
 	if (hw->phy.link_info.module_type[0] == I40E_MODULE_TYPE_SFP)
 		is_sfp = true;
 
+	data = info->data;
 	for (i = 0; i < info->length; i++) {
 		u32 offset = i + info->offset;
 		u32 addr = is_sfp ? I40E_I2C_EEPROM_DEV_ADDR : 0;
-- 
2.20.1

Re: [dpdk-dev] [PATCH 5/5] net/i40e: fix dereference before check when getting EEPROM

[Rami Rosen]

Acked-By: Rami Rosen <ramirose@gmail.com>

[dpdk-dev] [PATCH v2 0/5] some small fixes

[Bruce Richardson]

A few coverity fixes, along with a fix for one of the examples meson.build
files which I caught along the way.

v2: include a typo fix identified by Rami Rosen.

Bruce Richardson (5):
  examples/vhost_scsi: fix header check for meson build
  examples/vhost_scsi: fix missing NULL-check for parameter
  net/i40e: fix dereference before NULL check in mbuf release
  app/testpmd: fix variable use before NULL check
  net/i40e: fix dereference before check when getting EEPROM

 app/test-pmd/cmdline.c           | 4 ++--
 drivers/net/i40e/i40e_ethdev.c   | 5 +++--
 drivers/net/i40e/i40e_rxtx.c     | 6 +++---
 examples/vhost_scsi/meson.build  | 2 +-
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 5 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/5] examples/vhost_scsi: fix header check for meson build

[Bruce Richardson]

The header check for the example app was looking for virtio_scsi.h without
the "linux/" prefix, which meant it was never getting found when it should
have been.

Fixes: 8d47a753b7cb ("examples/vhost_scsi: disable build if missing dependency")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vhost_scsi/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/vhost_scsi/meson.build b/examples/vhost_scsi/meson.build
index ca1248023..e00a7dab1 100644
--- a/examples/vhost_scsi/meson.build
+++ b/examples/vhost_scsi/meson.build
@@ -10,7 +10,7 @@ if host_machine.system() != 'linux'
 	build = false
 endif
 
-if not cc.has_header('virtio_scsi.h')
+if not cc.has_header('linux/virtio_scsi.h')
 	build = false
 endif
 
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 1/5] examples/vhost_scsi: fix header check for meson build

[Tiwei Bie]

On Fri, Apr 05, 2019 at 03:37:05PM +0100, Bruce Richardson wrote:
> The header check for the example app was looking for virtio_scsi.h without
> the "linux/" prefix, which meant it was never getting found when it should
> have been.
> 
> Fixes: 8d47a753b7cb ("examples/vhost_scsi: disable build if missing dependency")
> CC: stable@dpdk.org
> CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> CC: Tiwei Bie <tiwei.bie@intel.com>
> CC: Zhihong Wang <zhihong.wang@intel.com>
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  examples/vhost_scsi/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/vhost_scsi/meson.build b/examples/vhost_scsi/meson.build
> index ca1248023..e00a7dab1 100644
> --- a/examples/vhost_scsi/meson.build
> +++ b/examples/vhost_scsi/meson.build
> @@ -10,7 +10,7 @@ if host_machine.system() != 'linux'
>  	build = false
>  endif
>  
> -if not cc.has_header('virtio_scsi.h')
> +if not cc.has_header('linux/virtio_scsi.h')
>  	build = false
>  endif
>  
> -- 
> 2.20.1

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

[dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Bruce Richardson]

Coverity points out that there is a check in the main thread loop for the
ctrlr->bdev being NULL, but by that stage the pointer has already been
dereferenced. Therefore, for safety, before we enter the loop do an
initial check on the parameter structure.

Coverity issue: 158657
Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index 2908ff68b..a6465d089 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -285,6 +285,12 @@ ctrlr_worker(void *arg)
 	cpu_set_t cpuset;
 	pthread_t thread;
 
+	if (ctrlr == NULL || ctrlr->bdev == NULL) {
+		fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n",
+				__func__);
+		return NULL;
+	}
+
 	thread = pthread_self();
 	CPU_ZERO(&cpuset);
 	CPU_SET(0, &cpuset);
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Tiwei Bie]

On Fri, Apr 05, 2019 at 03:37:06PM +0100, Bruce Richardson wrote:
> Coverity points out that there is a check in the main thread loop for the
> ctrlr->bdev being NULL, but by that stage the pointer has already been
> dereferenced. Therefore, for safety, before we enter the loop do an
> initial check on the parameter structure.
> 
> Coverity issue: 158657
> Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
> CC: stable@dpdk.org
> CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> CC: Tiwei Bie <tiwei.bie@intel.com>
> CC: Zhihong Wang <zhihong.wang@intel.com>
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  examples/vhost_scsi/vhost_scsi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
> index 2908ff68b..a6465d089 100644
> --- a/examples/vhost_scsi/vhost_scsi.c
> +++ b/examples/vhost_scsi/vhost_scsi.c
> @@ -285,6 +285,12 @@ ctrlr_worker(void *arg)
>  	cpu_set_t cpuset;
>  	pthread_t thread;
>  
> +	if (ctrlr == NULL || ctrlr->bdev == NULL) {
> +		fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n",

Might be better to print to stderr.

> +				__func__);
> +		return NULL;

Might be better to exit() directly like what the other
error handling in this thread does:

https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L296-L299

Otherwise destroy_device() will hang on sem_wait(&exit_sem):

https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L390

Thanks,
Tiwei

> +	}
> +
>  	thread = pthread_self();
>  	CPU_ZERO(&cpuset);
>  	CPU_SET(0, &cpuset);
> -- 
> 2.20.1
> 

Re: [dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Bruce Richardson]

On Mon, Apr 08, 2019 at 11:31:11AM +0800, Tiwei Bie wrote:
> On Fri, Apr 05, 2019 at 03:37:06PM +0100, Bruce Richardson wrote:
> > Coverity points out that there is a check in the main thread loop for the
> > ctrlr->bdev being NULL, but by that stage the pointer has already been
> > dereferenced. Therefore, for safety, before we enter the loop do an
> > initial check on the parameter structure.
> > 
> > Coverity issue: 158657
> > Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
> > CC: stable@dpdk.org
> > CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> > CC: Tiwei Bie <tiwei.bie@intel.com>
> > CC: Zhihong Wang <zhihong.wang@intel.com>
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  examples/vhost_scsi/vhost_scsi.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
> > index 2908ff68b..a6465d089 100644
> > --- a/examples/vhost_scsi/vhost_scsi.c
> > +++ b/examples/vhost_scsi/vhost_scsi.c
> > @@ -285,6 +285,12 @@ ctrlr_worker(void *arg)
> >  	cpu_set_t cpuset;
> >  	pthread_t thread;
> >  
> > +	if (ctrlr == NULL || ctrlr->bdev == NULL) {
> > +		fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n",
> 
> Might be better to print to stderr.
> 
Yes, this looks a typo on my part.

> > +				__func__);
> > +		return NULL;
> 
> Might be better to exit() directly like what the other
> error handling in this thread does:
> 
> https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L296-L299
> 
> Otherwise destroy_device() will hang on sem_wait(&exit_sem):
> 
> https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L390
> 
Ok, will change in next revision

[dpdk-dev] [PATCH v2 3/5] net/i40e: fix dereference before NULL check in mbuf release

[Bruce Richardson]

Coverity flags that the txq variable is used before it's checked for NULL.
Also fix typo in error message.

Coverity issue: 195023
Fixes: 24853544c84c ("net/i40e: fix mbuf free in vector Tx")
Cc: qi.z.zhang@intel.com
CC: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-By: Rami Rosen <ramirose@gmail.com>

---
V2: Fix typo or copy/paste error while fixing main issue
---
 drivers/net/i40e/i40e_rxtx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 8f727fae6..282e18bc1 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2423,13 +2423,13 @@ i40e_tx_queue_release_mbufs(struct i40e_tx_queue *txq)
 	struct rte_eth_dev *dev;
 	uint16_t i;
 
-	dev = &rte_eth_devices[txq->port_id];
-
 	if (!txq || !txq->sw_ring) {
-		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
+		PMD_DRV_LOG(DEBUG, "Pointer to txq or sw_ring is NULL");
 		return;
 	}
 
+	dev = &rte_eth_devices[txq->port_id];
+
 	/**
 	 *  vPMD tx will not set sw_ring's mbuf to NULL after free,
 	 *  so need to free remains more carefully.
-- 
2.20.1

[dpdk-dev] [PATCH v2 4/5] app/testpmd: fix variable use before NULL check

[Bruce Richardson]

The value returned from rte_eth_dev_tx_offload_name() function is used
for string comparison before being checked for NULL. Move the NULL check
up to be done first.

Coverity issue: 279438
Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
Cc: wei.dai@intel.com
Cc: stable@dpdk.org
Cc: Bernard Iremonger <bernard.iremonger@intel.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Rami Rosen <ramirose@gmail.com>
---
 app/test-pmd/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index ee50e4566..2155ff241 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18306,13 +18306,13 @@ search_tx_offload(const char *name)
 	single_offload = 1;
 	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
 		single_name = rte_eth_dev_tx_offload_name(single_offload);
+		if (single_name == NULL)
+			break;
 		if (!strcasecmp(single_name, name)) {
 			found = 1;
 			break;
 		} else if (!strcasecmp(single_name, "UNKNOWN"))
 			break;
-		else if (single_name == NULL)
-			break;
 		single_offload <<= 1;
 	}
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 5/5] net/i40e: fix dereference before check when getting EEPROM

[Bruce Richardson]

As flagged by coverity, the "info" structure is being explicitly
dereferenced before being checked later for a NULL value.

Coverity issue: 277241
Fixes: 98e60c0d43f1 ("net/i40e: add module EEPROM callbacks for i40e")
CC: stable@dpdk.org
Cc: zijie.pan@6wind.com
CC: Beilei Xing <beilei.xing@intel.com>
CC: Qi Zhang <qi.z.zhang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-By: Rami Rosen <ramirose@gmail.com>
---
 drivers/net/i40e/i40e_ethdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index b031bf4c6..6450af016 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -11893,16 +11893,17 @@ static int i40e_get_module_eeprom(struct rte_eth_dev *dev,
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	bool is_sfp = false;
 	i40e_status status;
-	uint8_t *data = info->data;
+	uint8_t *data;
 	uint32_t value = 0;
 	uint32_t i;
 
-	if (!info || !info->length || !data)
+	if (!info || !info->length || !info->data)
 		return -EINVAL;
 
 	if (hw->phy.link_info.module_type[0] == I40E_MODULE_TYPE_SFP)
 		is_sfp = true;
 
+	data = info->data;
 	for (i = 0; i < info->length; i++) {
 		u32 offset = i + info->offset;
 		u32 addr = is_sfp ? I40E_I2C_EEPROM_DEV_ADDR : 0;
-- 
2.20.1

[dpdk-dev] [PATCH v3 0/5] some small fixes

[Bruce Richardson]

A few coverity fixes, along with a fix for one of the examples meson.build
files which I caught along the way.

v3: updated patch 2, to include fixes from Tiwei.
v2: include a typo fix identified by Rami Rosen.

Bruce Richardson (5):
  examples/vhost_scsi: fix header check for meson build
  examples/vhost_scsi: fix missing NULL-check for parameter
  net/i40e: fix dereference before NULL check in mbuf release
  app/testpmd: fix variable use before NULL check
  net/i40e: fix dereference before check when getting EEPROM

 app/test-pmd/cmdline.c           | 4 ++--
 drivers/net/i40e/i40e_ethdev.c   | 5 +++--
 drivers/net/i40e/i40e_rxtx.c     | 6 +++---
 examples/vhost_scsi/meson.build  | 2 +-
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 5 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/5] examples/vhost_scsi: fix header check for meson build

[Bruce Richardson]

The header check for the example app was looking for virtio_scsi.h without
the "linux/" prefix, which meant it was never getting found when it should
have been.

Fixes: 8d47a753b7cb ("examples/vhost_scsi: disable build if missing dependency")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>
---
 examples/vhost_scsi/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/vhost_scsi/meson.build b/examples/vhost_scsi/meson.build
index ca1248023..e00a7dab1 100644
--- a/examples/vhost_scsi/meson.build
+++ b/examples/vhost_scsi/meson.build
@@ -10,7 +10,7 @@ if host_machine.system() != 'linux'
 	build = false
 endif
 
-if not cc.has_header('virtio_scsi.h')
+if not cc.has_header('linux/virtio_scsi.h')
 	build = false
 endif
 
-- 
2.20.1

[dpdk-dev] [PATCH v3 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Bruce Richardson]

Coverity points out that there is a check in the main thread loop for the
ctrlr->bdev being NULL, but by that stage the pointer has already been
dereferenced. Therefore, for safety, before we enter the loop do an
initial check on the parameter structure.

Coverity issue: 158657
Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
CC: stable@dpdk.org
CC: Maxime Coquelin <maxime.coquelin@redhat.com>
CC: Tiwei Bie <tiwei.bie@intel.com>
CC: Zhihong Wang <zhihong.wang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 examples/vhost_scsi/vhost_scsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index 2908ff68b..513af0cca 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -285,6 +285,12 @@ ctrlr_worker(void *arg)
 	cpu_set_t cpuset;
 	pthread_t thread;
 
+	if (ctrlr == NULL || ctrlr->bdev == NULL) {
+		fprintf(stderr, "%s: Error, invalid argument passed to worker thread\n",
+				__func__);
+		exit(0);
+	}
+
 	thread = pthread_self();
 	CPU_ZERO(&cpuset);
 	CPU_SET(0, &cpuset);
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Tiwei Bie]

On Mon, Apr 08, 2019 at 10:46:37AM +0100, Bruce Richardson wrote:
> Coverity points out that there is a check in the main thread loop for the
> ctrlr->bdev being NULL, but by that stage the pointer has already been
> dereferenced. Therefore, for safety, before we enter the loop do an
> initial check on the parameter structure.
> 
> Coverity issue: 158657
> Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app")
> CC: stable@dpdk.org
> CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> CC: Tiwei Bie <tiwei.bie@intel.com>
> CC: Zhihong Wang <zhihong.wang@intel.com>
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  examples/vhost_scsi/vhost_scsi.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

[dpdk-dev] [PATCH v3 3/5] net/i40e: fix dereference before NULL check in mbuf release

[Bruce Richardson]

Coverity flags that the txq variable is used before it's checked for NULL.
Also fix typo in error message.

Coverity issue: 195023
Fixes: 24853544c84c ("net/i40e: fix mbuf free in vector Tx")
Cc: qi.z.zhang@intel.com
CC: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Rami Rosen <ramirose@gmail.com>
---
 drivers/net/i40e/i40e_rxtx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 8f727fae6..282e18bc1 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2423,13 +2423,13 @@ i40e_tx_queue_release_mbufs(struct i40e_tx_queue *txq)
 	struct rte_eth_dev *dev;
 	uint16_t i;
 
-	dev = &rte_eth_devices[txq->port_id];
-
 	if (!txq || !txq->sw_ring) {
-		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
+		PMD_DRV_LOG(DEBUG, "Pointer to txq or sw_ring is NULL");
 		return;
 	}
 
+	dev = &rte_eth_devices[txq->port_id];
+
 	/**
 	 *  vPMD tx will not set sw_ring's mbuf to NULL after free,
 	 *  so need to free remains more carefully.
-- 
2.20.1

[dpdk-dev] [PATCH v3 4/5] app/testpmd: fix variable use before NULL check

[Bruce Richardson]

The value returned from rte_eth_dev_tx_offload_name() function is used
for string comparison before being checked for NULL. Move the NULL check
up to be done first.

Coverity issue: 279438
Fixes: c73a9071877a ("app/testpmd: add commands to test new offload API")
Cc: wei.dai@intel.com
Cc: stable@dpdk.org
Cc: Bernard Iremonger <bernard.iremonger@intel.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Rami Rosen <ramirose@gmail.com>
---
 app/test-pmd/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 2ab03c111..391bc8097 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -18311,13 +18311,13 @@ search_tx_offload(const char *name)
 	single_offload = 1;
 	for (bit = 0; bit < sizeof(single_offload) * CHAR_BIT; bit++) {
 		single_name = rte_eth_dev_tx_offload_name(single_offload);
+		if (single_name == NULL)
+			break;
 		if (!strcasecmp(single_name, name)) {
 			found = 1;
 			break;
 		} else if (!strcasecmp(single_name, "UNKNOWN"))
 			break;
-		else if (single_name == NULL)
-			break;
 		single_offload <<= 1;
 	}
 
-- 
2.20.1

[dpdk-dev] [PATCH v3 5/5] net/i40e: fix dereference before check when getting EEPROM

[Bruce Richardson]

As flagged by coverity, the "info" structure is being explicitly
dereferenced before being checked later for a NULL value.

Coverity issue: 277241
Fixes: 98e60c0d43f1 ("net/i40e: add module EEPROM callbacks for i40e")
CC: stable@dpdk.org
Cc: zijie.pan@6wind.com
CC: Beilei Xing <beilei.xing@intel.com>
CC: Qi Zhang <qi.z.zhang@intel.com>

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Rami Rosen <ramirose@gmail.com>
---
 drivers/net/i40e/i40e_ethdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 5b01dc1f0..9305b7ac1 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -11901,16 +11901,17 @@ static int i40e_get_module_eeprom(struct rte_eth_dev *dev,
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	bool is_sfp = false;
 	i40e_status status;
-	uint8_t *data = info->data;
+	uint8_t *data;
 	uint32_t value = 0;
 	uint32_t i;
 
-	if (!info || !info->length || !data)
+	if (!info || !info->length || !info->data)
 		return -EINVAL;
 
 	if (hw->phy.link_info.module_type[0] == I40E_MODULE_TYPE_SFP)
 		is_sfp = true;
 
+	data = info->data;
 	for (i = 0; i < info->length; i++) {
 		u32 offset = i + info->offset;
 		u32 addr = is_sfp ? I40E_I2C_EEPROM_DEV_ADDR : 0;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 0/5] some small fixes

[Thomas Monjalon]

> Bruce Richardson (5):
>   examples/vhost_scsi: fix header check for meson build
>   examples/vhost_scsi: fix missing NULL-check for parameter
>   net/i40e: fix dereference before NULL check in mbuf release
>   app/testpmd: fix variable use before NULL check
>   net/i40e: fix dereference before check when getting EEPROM

Applied, thanks