[dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Helin Zhang]

It fixes several problematic dereferences in i40e driver,
reported by Coverity.

Helin Zhang (6):
  i40e: fix problematic dereference
  i40e: fix problematic dereference
  i40e: fix problematic dereference
  i40e: fix problematic dereference
  i40e: fix problematic dereference
  i40e: fix problematic dereference

 drivers/net/i40e/i40e_pf.c   |  7 +++----
 drivers/net/i40e/i40e_rxtx.c | 18 +++++++++++-------
 2 files changed, 14 insertions(+), 11 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH 1/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 119267: Dereference before null check.

Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 4d35d83..9c126a3 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2592,14 +2592,14 @@ i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq)
 {
 	uint16_t i;
 
-	/* SSE Vector driver has a different way of releasing mbufs. */
-	if (rxq->rx_using_sse) {
-		i40e_rx_queue_release_mbufs_vec(rxq);
+	if (!rxq || !rxq->sw_ring) {
+		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
 		return;
 	}
 
-	if (!rxq || !rxq->sw_ring) {
-		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
+	/* SSE Vector driver has a different way of releasing mbufs. */
+	if (rxq->rx_using_sse) {
+		i40e_rx_queue_release_mbufs_vec(rxq);
 		return;
 	}
 
-- 
2.5.0

Re: [dpdk-dev] [PATCH 1/6] i40e: fix problematic dereference

[Stephen Hemminger]

On Thu, 21 Apr 2016 11:42:52 +0800
Helin Zhang <helin.zhang@intel.com> wrote:

> Fix issue reported by Coverity.
> 
> Coverity ID 119267: Dereference before null check.
> 
> Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 4d35d83..9c126a3 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -2592,14 +2592,14 @@ i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq)
>  {
>  	uint16_t i;
>  
> -	/* SSE Vector driver has a different way of releasing mbufs. */
> -	if (rxq->rx_using_sse) {
> -		i40e_rx_queue_release_mbufs_vec(rxq);
> +	if (!rxq || !rxq->sw_ring) {
> +		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
>  		return;
>  	}
>  

I can't see how you could ever trigger this.
 i40e_rx_queue_release_mbufs() is called only called from places where rxq
is guraanteed non NULL.

Are you sure Coverity isn't tell you that?

Re: [dpdk-dev] [PATCH 1/6] i40e: fix problematic dereference

[Zhang, Helin]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, April 22, 2016 12:10 AM
> To: Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/6] i40e: fix problematic dereference
> 
> On Thu, 21 Apr 2016 11:42:52 +0800
> Helin Zhang <helin.zhang@intel.com> wrote:
> 
> > Fix issue reported by Coverity.
> >
> > Coverity ID 119267: Dereference before null check.
> >
> > Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index 4d35d83..9c126a3 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -2592,14 +2592,14 @@ i40e_rx_queue_release_mbufs(struct
> > i40e_rx_queue *rxq)  {
> >  	uint16_t i;
> >
> > -	/* SSE Vector driver has a different way of releasing mbufs. */
> > -	if (rxq->rx_using_sse) {
> > -		i40e_rx_queue_release_mbufs_vec(rxq);
> > +	if (!rxq || !rxq->sw_ring) {
> > +		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
> >  		return;
> >  	}
> >
> 
> I can't see how you could ever trigger this.
>  i40e_rx_queue_release_mbufs() is called only called from places where rxq is
> guraanteed non NULL.
> 
> Are you sure Coverity isn't tell you that?
Coverity just reported " Dereference before null check ", nothing else there.
The function was called several times in different places. It could be safer, in case
somebody wrongly give the queue id.
But you are right, it shouldn't happen if nothing wrong.

Are you suggesting to remove rxq check? Thanks!

Regards,
Helin

[dpdk-dev] [PATCH 2/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 13301: Dereference before null check.

Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 9c126a3..31bfc44 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -3035,13 +3035,15 @@ i40e_fdir_setup_rx_resources(struct i40e_pf *pf)
 	struct i40e_rx_queue *rxq;
 	const struct rte_memzone *rz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the RX queue data structure. */
 	rxq = rte_zmalloc_socket("i40e fdir rx queue",
 				  sizeof(struct i40e_rx_queue),
-- 
2.5.0

[dpdk-dev] [PATCH 3/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 13294: Dereference before null check.

Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 31bfc44..4fd96fe 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2981,13 +2981,15 @@ i40e_fdir_setup_tx_resources(struct i40e_pf *pf)
 	struct i40e_tx_queue *txq;
 	const struct rte_memzone *tz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the TX queue data structure. */
 	txq = rte_zmalloc_socket("i40e fdir tx queue",
 				  sizeof(struct i40e_tx_queue),
-- 
2.5.0

[dpdk-dev] [PATCH 4/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 13299: Dereference before null check.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 5afd61a..3ec8f7c 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -913,7 +913,7 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	/* AdminQ will pass absolute VF id, transfer to internal vf id */
 	uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id;
 
-	if (!dev || vf_id > pf->vf_num - 1 || !pf->vfs) {
+	if (vf_id > pf->vf_num - 1 || !pf->vfs) {
 		PMD_DRV_LOG(ERR, "invalid argument");
 		return;
 	}
-- 
2.5.0

[dpdk-dev] [PATCH 5/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 13298: Dereference before null check.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 3ec8f7c..1bd599b 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -123,7 +123,7 @@ int
 i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 {
 	uint32_t val, i;
-	struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
+	struct i40e_hw *hw;
 	uint16_t vf_id, abs_vf_id, vf_msix_num;
 	int ret;
 	struct i40e_virtchnl_queue_select qsel;
@@ -131,6 +131,7 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 	if (vf == NULL)
 		return -EINVAL;
 
+	hw = I40E_PF_TO_HW(vf->pf);
 	vf_id = vf->vf_idx;
 	abs_vf_id = vf_id + hw->func_caps.vf_base_id;
 
-- 
2.5.0

[dpdk-dev] [PATCH 6/6] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 13265: Missing break in switch.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 1bd599b..1ea7b0a 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -1000,8 +1000,6 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	 /* Don't add command supported below, which will
 	 *  return an error code.
 	 */
-	case I40E_VIRTCHNL_OP_FCOE:
-		PMD_DRV_LOG(ERR, "OP_FCOE received, not supported");
 	default:
 		PMD_DRV_LOG(ERR, "%u received, not supported", opcode);
 		i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM,
-- 
2.5.0

Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Thomas Monjalon]

2016-04-21 11:42, Helin Zhang:
> It fixes several problematic dereferences in i40e driver,
> reported by Coverity.
> 
> Helin Zhang (6):
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference

One patch is enough to fix all the occurences of the same issue.

Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, April 21, 2016 6:07 PM
> To: Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference
> 
> 2016-04-21 11:42, Helin Zhang:
> > It fixes several problematic dereferences in i40e driver, reported by
> > Coverity.
> >
> > Helin Zhang (6):
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> 
> One patch is enough to fix all the occurences of the same issue.

But their Fixes line are different. How to handle that?

Thanks,
Helin

Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Thomas Monjalon]

> > 2016-04-21 11:42, Helin Zhang:
> > > It fixes several problematic dereferences in i40e driver, reported by
> > > Coverity.
> > >
> > > Helin Zhang (6):
> > >   i40e: fix problematic dereference
> > >   i40e: fix problematic dereference
> > >   i40e: fix problematic dereference
> > >   i40e: fix problematic dereference
> > >   i40e: fix problematic dereference
> > >   i40e: fix problematic dereference
> > 
> > One patch is enough to fix all the occurences of the same issue.
> 
> But their Fixes line are different. How to handle that?

You can have several Fixes lines.
Thanks

Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Bruce Richardson]

On Thu, Apr 21, 2016 at 11:42:51AM +0800, Helin Zhang wrote:
> It fixes several problematic dereferences in i40e driver,
> reported by Coverity.
> 
> Helin Zhang (6):
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
>   i40e: fix problematic dereference
> 
Since this is the same issue fixed multiple times, I think it might be best to
just merge these patches into just one patch to fix the dereferences. You can
include all the fixes lines and coverity defect references in the one commit
message.

/Bruce

Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference

[Zhang, Helin]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, April 21, 2016 7:02 PM
> To: Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/6] fix i40e problematic dereference
> 
> On Thu, Apr 21, 2016 at 11:42:51AM +0800, Helin Zhang wrote:
> > It fixes several problematic dereferences in i40e driver, reported by
> > Coverity.
> >
> > Helin Zhang (6):
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >   i40e: fix problematic dereference
> >
> Since this is the same issue fixed multiple times, I think it might be best to just
> merge these patches into just one patch to fix the dereferences. You can include
> all the fixes lines and coverity defect references in the one commit message.
Yes, that's good idea!
I will send v2 soon later.

Thanks,
Helin

> 
> /Bruce

[dpdk-dev] [PATCH v2] i40e: fix problematic dereference

[Helin Zhang]

Fix issue reported by Coverity.

Coverity ID 119267: Dereference before null check.
Coverity ID 13301: Dereference before null check.
Coverity ID 13294: Dereference before null check.
Coverity ID 13299: Dereference before null check.
Coverity ID 13298: Dereference before null check.
Coverity ID 13265: Missing break in switch.

Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c   |  7 +++----
 drivers/net/i40e/i40e_rxtx.c | 10 +++++++---
 2 files changed, 10 insertions(+), 7 deletions(-)

v2:
 - Combined all the patches into a single one, as suggested.
 - Remove checking rxq, as it shouldn't be NULL at any time.

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 5afd61a..1ea7b0a 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -123,7 +123,7 @@ int
 i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 {
 	uint32_t val, i;
-	struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
+	struct i40e_hw *hw;
 	uint16_t vf_id, abs_vf_id, vf_msix_num;
 	int ret;
 	struct i40e_virtchnl_queue_select qsel;
@@ -131,6 +131,7 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 	if (vf == NULL)
 		return -EINVAL;
 
+	hw = I40E_PF_TO_HW(vf->pf);
 	vf_id = vf->vf_idx;
 	abs_vf_id = vf_id + hw->func_caps.vf_base_id;
 
@@ -913,7 +914,7 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	/* AdminQ will pass absolute VF id, transfer to internal vf id */
 	uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id;
 
-	if (!dev || vf_id > pf->vf_num - 1 || !pf->vfs) {
+	if (vf_id > pf->vf_num - 1 || !pf->vfs) {
 		PMD_DRV_LOG(ERR, "invalid argument");
 		return;
 	}
@@ -999,8 +1000,6 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	 /* Don't add command supported below, which will
 	 *  return an error code.
 	 */
-	case I40E_VIRTCHNL_OP_FCOE:
-		PMD_DRV_LOG(ERR, "OP_FCOE received, not supported");
 	default:
 		PMD_DRV_LOG(ERR, "%u received, not supported", opcode);
 		i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM,
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 4d35d83..f9e17fd 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2598,7 +2598,7 @@ i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq)
 		return;
 	}
 
-	if (!rxq || !rxq->sw_ring) {
+	if (!rxq->sw_ring) {
 		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
 		return;
 	}
@@ -2981,13 +2981,15 @@ i40e_fdir_setup_tx_resources(struct i40e_pf *pf)
 	struct i40e_tx_queue *txq;
 	const struct rte_memzone *tz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the TX queue data structure. */
 	txq = rte_zmalloc_socket("i40e fdir tx queue",
 				  sizeof(struct i40e_tx_queue),
@@ -3035,13 +3037,15 @@ i40e_fdir_setup_rx_resources(struct i40e_pf *pf)
 	struct i40e_rx_queue *rxq;
 	const struct rte_memzone *rz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the RX queue data structure. */
 	rxq = rte_zmalloc_socket("i40e fdir rx queue",
 				  sizeof(struct i40e_rx_queue),
-- 
2.5.0

Re: [dpdk-dev] [PATCH v2] i40e: fix problematic dereference

[Bruce Richardson]

On Mon, Apr 25, 2016 at 01:44:02PM +0800, Helin Zhang wrote:
> Fix issue reported by Coverity.
> 
> Coverity ID 119267: Dereference before null check.
> Coverity ID 13301: Dereference before null check.
> Coverity ID 13294: Dereference before null check.
> Coverity ID 13299: Dereference before null check.
> Coverity ID 13298: Dereference before null check.

+1 for merging all patches into a single one

> Coverity ID 13265: Missing break in switch.

-1 for sticking in a different fix type into your single patch.

> 
> Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
> Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_pf.c   |  7 +++----
>  drivers/net/i40e/i40e_rxtx.c | 10 +++++++---
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> v2:
>  - Combined all the patches into a single one, as suggested.
>  - Remove checking rxq, as it shouldn't be NULL at any time.

Thanks for the V2, but please try and keep the patches logically consistent.
If you have multiple patches fixing the same thing in the one driver, please
keep it in one patch. A fix for a different issue type needs to be in a different
patch. If the fix for one type of issue also causes a second issue of a different
type to be fixed also, please call that out separately in the commit message.
Please also explain how the null checks issues are being fixed, and what the
original issue was.

Regards,
/Bruce

[dpdk-dev] [PATCH v3 0/2] fix Coverity reported issues

[Helin Zhang]

It fixes the issue reported by Coverity of 'Missing break in
switch', by deleting 'case I40E_VIRTCHNL_OP_FCOE', as it is
not necessary at all.
It also fixes the issue reported by Coverity of 'Dereference
before null check', by deleting null checks as they are not
necessary at all, or move null checks before where uses it.

v3:
 - Split the code changes into different patches, according
   to the issue type.
 - Reworded the commit logs.

v2:
 - Combined all the patches into a single one, as suggested.
 - Remove checking rxq, as it shouldn't be NULL at any time.

Helin Zhang (2):
  i40e: fix problematic dereference
  i40e: fix missing break in switch

 drivers/net/i40e/i40e_pf.c   | 11 +++++------
 drivers/net/i40e/i40e_rxtx.c | 10 +++++++---
 2 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH v3 1/2] i40e: fix problematic dereference

[Helin Zhang]

It fixes the issue reported by Coverity of 'Dereference before
null check', by deleting null checks as they are not necessary
at all, or move null checks before where uses it.

Coverity ID 119267: Dereference before null check.
Coverity ID 13301: Dereference before null check.
Coverity ID 13294: Dereference before null check.
Coverity ID 13299: Dereference before null check.
Coverity ID 13298: Dereference before null check.

Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c   |  5 +++--
 drivers/net/i40e/i40e_rxtx.c | 10 +++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

v3:
 - Split the code changes into different patches, according
   to the issue type.
 - Reworded the commit logs.

v2:
 - Combined all the patches into a single one, as suggested.
 - Remove checking rxq, as it shouldn't be NULL at any time.

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 5afd61a..1bd599b 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -123,7 +123,7 @@ int
 i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 {
 	uint32_t val, i;
-	struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
+	struct i40e_hw *hw;
 	uint16_t vf_id, abs_vf_id, vf_msix_num;
 	int ret;
 	struct i40e_virtchnl_queue_select qsel;
@@ -131,6 +131,7 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 	if (vf == NULL)
 		return -EINVAL;
 
+	hw = I40E_PF_TO_HW(vf->pf);
 	vf_id = vf->vf_idx;
 	abs_vf_id = vf_id + hw->func_caps.vf_base_id;
 
@@ -913,7 +914,7 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	/* AdminQ will pass absolute VF id, transfer to internal vf id */
 	uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id;
 
-	if (!dev || vf_id > pf->vf_num - 1 || !pf->vfs) {
+	if (vf_id > pf->vf_num - 1 || !pf->vfs) {
 		PMD_DRV_LOG(ERR, "invalid argument");
 		return;
 	}
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 4d35d83..f9e17fd 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2598,7 +2598,7 @@ i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq)
 		return;
 	}
 
-	if (!rxq || !rxq->sw_ring) {
+	if (!rxq->sw_ring) {
 		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
 		return;
 	}
@@ -2981,13 +2981,15 @@ i40e_fdir_setup_tx_resources(struct i40e_pf *pf)
 	struct i40e_tx_queue *txq;
 	const struct rte_memzone *tz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the TX queue data structure. */
 	txq = rte_zmalloc_socket("i40e fdir tx queue",
 				  sizeof(struct i40e_tx_queue),
@@ -3035,13 +3037,15 @@ i40e_fdir_setup_rx_resources(struct i40e_pf *pf)
 	struct i40e_rx_queue *rxq;
 	const struct rte_memzone *rz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the RX queue data structure. */
 	rxq = rte_zmalloc_socket("i40e fdir rx queue",
 				  sizeof(struct i40e_rx_queue),
-- 
2.5.0

[dpdk-dev] [PATCH v3 2/2] i40e: fix missing break in switch

[Helin Zhang]

It fixes the issue reported by Coverity of 'Missing break in
switch', by deleting 'case I40E_VIRTCHNL_OP_FCOE', as it is
not necessary at all.

Coverity ID 13265: Missing break in switch.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 1bd599b..b47a374 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -997,11 +997,9 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(INFO, "OP_CFG_VLAN_PVID received");
 		i40e_pf_host_process_cmd_cfg_pvid(vf, msg, msglen);
 		break;
-	 /* Don't add command supported below, which will
-	 *  return an error code.
+	/* Don't add command supported below, which will
+	 * return an error code.
 	 */
-	case I40E_VIRTCHNL_OP_FCOE:
-		PMD_DRV_LOG(ERR, "OP_FCOE received, not supported");
 	default:
 		PMD_DRV_LOG(ERR, "%u received, not supported", opcode);
 		i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM,
-- 
2.5.0

[dpdk-dev] [PATCH v2 0/2] fix coverity issues

[Helin Zhang]

It fixes several problematic dereference issues and missing
break issue reported by Coverity.

Helin Zhang (2):
  i40e: fix problematic dereference
  i40e: fix missing break in switch

 drivers/net/i40e/i40e_pf.c   | 11 +++++------
 drivers/net/i40e/i40e_rxtx.c | 12 ++++++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH v2 1/2] i40e: fix problematic dereference

[Helin Zhang]

It fixes the issue reported by Coverity of 'Dereference before
null check', by deleting null checks as they are not necessary
at all, or move null checks before where uses it.

Coverity ID 119267: Dereference before null check.
Coverity ID 13301: Dereference before null check.
Coverity ID 13294: Dereference before null check.
Coverity ID 13299: Dereference before null check.
Coverity ID 13298: Dereference before null check.

Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c   |  5 +++--
 drivers/net/i40e/i40e_rxtx.c | 12 ++++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 5afd61a..1bd599b 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -123,7 +123,7 @@ int
 i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 {
 	uint32_t val, i;
-	struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf);
+	struct i40e_hw *hw;
 	uint16_t vf_id, abs_vf_id, vf_msix_num;
 	int ret;
 	struct i40e_virtchnl_queue_select qsel;
@@ -131,6 +131,7 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool do_hw_reset)
 	if (vf == NULL)
 		return -EINVAL;
 
+	hw = I40E_PF_TO_HW(vf->pf);
 	vf_id = vf->vf_idx;
 	abs_vf_id = vf_id + hw->func_caps.vf_base_id;
 
@@ -913,7 +914,7 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 	/* AdminQ will pass absolute VF id, transfer to internal vf id */
 	uint16_t vf_id = abs_vf_id - hw->func_caps.vf_base_id;
 
-	if (!dev || vf_id > pf->vf_num - 1 || !pf->vfs) {
+	if (vf_id > pf->vf_num - 1 || !pf->vfs) {
 		PMD_DRV_LOG(ERR, "invalid argument");
 		return;
 	}
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 049a813..7bb0aa9 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2614,8 +2614,8 @@ i40e_rx_queue_release_mbufs(struct i40e_rx_queue *rxq)
 		return;
 	}
 
-	if (!rxq || !rxq->sw_ring) {
-		PMD_DRV_LOG(DEBUG, "Pointer to rxq or sw_ring is NULL");
+	if (!rxq->sw_ring) {
+		PMD_DRV_LOG(DEBUG, "Pointer to sw_ring is NULL");
 		return;
 	}
 
@@ -3002,13 +3002,15 @@ i40e_fdir_setup_tx_resources(struct i40e_pf *pf)
 	struct i40e_tx_queue *txq;
 	const struct rte_memzone *tz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the TX queue data structure. */
 	txq = rte_zmalloc_socket("i40e fdir tx queue",
 				  sizeof(struct i40e_tx_queue),
@@ -3056,13 +3058,15 @@ i40e_fdir_setup_rx_resources(struct i40e_pf *pf)
 	struct i40e_rx_queue *rxq;
 	const struct rte_memzone *rz = NULL;
 	uint32_t ring_size;
-	struct rte_eth_dev *dev = pf->adapter->eth_dev;
+	struct rte_eth_dev *dev;
 
 	if (!pf) {
 		PMD_DRV_LOG(ERR, "PF is not available");
 		return I40E_ERR_BAD_PTR;
 	}
 
+	dev = pf->adapter->eth_dev;
+
 	/* Allocate the RX queue data structure. */
 	rxq = rte_zmalloc_socket("i40e fdir rx queue",
 				  sizeof(struct i40e_rx_queue),
-- 
2.5.0

Re: [dpdk-dev] [PATCH v2 1/2] i40e: fix problematic dereference

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> Sent: Sunday, June 26, 2016 4:46 PM
> To: dev@dpdk.org
> Cc: Zhang, Helin <helin.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2 1/2] i40e: fix problematic dereference
> 
> It fixes the issue reported by Coverity of 'Dereference before null
> check', by deleting null checks as they are not necessary at all, or move
> null checks before where uses it.
> 
> Coverity ID 119267: Dereference before null check.
> Coverity ID 13301: Dereference before null check.
> Coverity ID 13294: Dereference before null check.
> Coverity ID 13299: Dereference before null check.
> Coverity ID 13298: Dereference before null check.
> 
> Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")
> Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
> Fixes: a778a1fa2e4e ("i40e: set up and initialize flow director")
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

[dpdk-dev] [PATCH v2 2/2] i40e: fix missing break in switch

[Helin Zhang]

It fixes the issue reported by Coverity of 'Missing break in
switch', by deleting 'case I40E_VIRTCHNL_OP_FCOE', as it is
not necessary at all.

Coverity ID 13265: Missing break in switch.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 drivers/net/i40e/i40e_pf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c
index 1bd599b..b47a374 100644
--- a/drivers/net/i40e/i40e_pf.c
+++ b/drivers/net/i40e/i40e_pf.c
@@ -997,11 +997,9 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(INFO, "OP_CFG_VLAN_PVID received");
 		i40e_pf_host_process_cmd_cfg_pvid(vf, msg, msglen);
 		break;
-	 /* Don't add command supported below, which will
-	 *  return an error code.
+	/* Don't add command supported below, which will
+	 * return an error code.
 	 */
-	case I40E_VIRTCHNL_OP_FCOE:
-		PMD_DRV_LOG(ERR, "OP_FCOE received, not supported");
 	default:
 		PMD_DRV_LOG(ERR, "%u received, not supported", opcode);
 		i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM,
-- 
2.5.0

Re: [dpdk-dev] [PATCH v2 2/2] i40e: fix missing break in switch

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> Sent: Sunday, June 26, 2016 4:46 PM
> To: dev@dpdk.org
> Cc: Zhang, Helin <helin.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2 2/2] i40e: fix missing break in switch
> 
> It fixes the issue reported by Coverity of 'Missing break in switch', by
> deleting 'case I40E_VIRTCHNL_OP_FCOE', as it is not necessary at all.
> 
> Coverity ID 13265: Missing break in switch.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH v2 0/2] fix coverity issues

[Bruce Richardson]

On Sun, Jun 26, 2016 at 11:46:16PM +0800, Helin Zhang wrote:
> It fixes several problematic dereference issues and missing
> break issue reported by Coverity.
> 
> Helin Zhang (2):
>   i40e: fix problematic dereference
>   i40e: fix missing break in switch
> 
>  drivers/net/i40e/i40e_pf.c   | 11 +++++------
>  drivers/net/i40e/i40e_rxtx.c | 12 ++++++++----
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> -- 
Applied to dpdk-next-net/rel_16_07

/Bruce

[dpdk-dev] [PATCH v1] app: fix Coverity issues

[Remy Horton]

Fixes memory leaks detected by Coverity. These are due to ephemeral
memory allocations not being freed when errors occur.

Coverity issue 127348: Resource leak
Coverity issue 127349: Resource leak

Fixes: e2aae1c1ced9 ("ethdev: remove name from extended statistic fetch")

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/test-pmd/config.c          | 3 +++
 examples/l2fwd-keepalive/shm.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 10f0a36..cb71c09 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -281,6 +281,7 @@ nic_xstats_display(portid_t port_id)
 	if (cnt_xstats != rte_eth_xstats_get_names(
 			port_id, xstats_names, cnt_xstats)) {
 		printf("Error: Cannot get xstats lookup\n");
+		free(xstats_names);
 		return;
 	}
 
@@ -293,6 +294,8 @@ nic_xstats_display(portid_t port_id)
 	}
 	if (cnt_xstats != rte_eth_xstats_get(port_id, xstats, cnt_xstats)) {
 		printf("Error: Unable to get xstats\n");
+		free(xstats_names);
+		free(xstats);
 		return;
 	}
 
diff --git a/examples/l2fwd-keepalive/shm.c b/examples/l2fwd-keepalive/shm.c
index 66fc433..177aa5b 100644
--- a/examples/l2fwd-keepalive/shm.c
+++ b/examples/l2fwd-keepalive/shm.c
@@ -80,6 +80,8 @@ struct rte_keepalive_shm *rte_keepalive_shm_create(void)
 				RTE_LOG(INFO, EAL,
 					"Failed to setup SHM semaphore (%s)\n",
 					strerror(errno));
+				munmap(ka_shm,
+					sizeof(struct rte_keepalive_shm));
 				return NULL;
 			}
 
-- 
2.5.5

[dpdk-dev] [PATCH v2 0/2] fix Coverity issues

[Remy Horton]

Fixes memory leaks detected by Coverity. These are due to ephemeral
memory allocations not being freed when errors occur.

--
v2 changes:
* Split patch into two

Remy Horton (2):
  app/test-pmd: fix Coverity issues
  examples/l2fwd-keepalive: fix Coverity issues

 app/test-pmd/config.c          | 3 +++
 examples/l2fwd-keepalive/shm.c | 2 ++
 2 files changed, 5 insertions(+)

-- 
2.5.5

Re: [dpdk-dev] [PATCH v2 0/2] fix Coverity issues

[Thomas Monjalon]

> Remy Horton (2):
>   app/test-pmd: fix Coverity issues
>   examples/l2fwd-keepalive: fix Coverity issues

Applied with titles reworded as suggested by Pablo, thanks