[dpdk-dev] [PATCH] enic: fix seg fault when releasing queues

[dpdk-dev] [PATCH] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, like if
there were more queues requested than available, the queue release
function is called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers. Also, if configuration
fails due to lack of resources, be more specific about which resources
are lacking.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_main.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index bbbe660..a3eb4e3 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -467,14 +467,16 @@ int enic_alloc_intr_resources(struct enic *enic)
 
 void enic_free_rq(void *rxq)
 {
-	struct vnic_rq *rq = (struct vnic_rq *)rxq;
-	struct enic *enic = vnic_dev_priv(rq->vdev);
+	if (rxq != NULL) {
+		struct vnic_rq *rq = (struct vnic_rq *)rxq;
+		struct enic *enic = vnic_dev_priv(rq->vdev);
 
-	enic_rxmbuf_queue_release(enic, rq);
-	rte_free(rq->mbuf_ring);
-	rq->mbuf_ring = NULL;
-	vnic_rq_free(rq);
-	vnic_cq_free(&enic->cq[rq->index]);
+		enic_rxmbuf_queue_release(enic, rq);
+		rte_free(rq->mbuf_ring);
+		rq->mbuf_ring = NULL;
+		vnic_rq_free(rq);
+		vnic_cq_free(&enic->cq[rq->index]);
+	}
 }
 
 void enic_start_wq(struct enic *enic, uint16_t queue_idx)
@@ -841,16 +843,22 @@ int enic_set_vnic_res(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
 
-	if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-		(enic->wq_count < eth_dev->data->nb_tx_queues)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
+	if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+		dev_err(dev, "Not enough Receive queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_rx_queues, enic->rq_count);
+		return -1;
+	}
+	if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+		dev_err(dev, "Not enough Transmit queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_tx_queues, enic->wq_count);
 		return -1;
 	}
 
 	enic->rq_count = eth_dev->data->nb_rx_queues;
 	enic->wq_count = eth_dev->data->nb_tx_queues;
 	if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
+		dev_err(dev, "Not enough Completion queues. Required:%u, Configured:%u\n",
+			enic->rq_count + enic->wq_count, enic->cq_count);
 		return -1;
 	}
 
-- 
2.7.0

[dpdk-dev] [PATCH v2] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, like if
there were more queues requested than available, the queue release
function is called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers. Also, if configuration
fails due to lack of resources, be more specific about which resources
are lacking.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---
v2: Log an error for all resource deficiencies not just the first one
found.

 drivers/net/enic/enic_main.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 996f999..411d23c 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -426,14 +426,16 @@ int enic_alloc_intr_resources(struct enic *enic)
 
 void enic_free_rq(void *rxq)
 {
-	struct vnic_rq *rq = (struct vnic_rq *)rxq;
-	struct enic *enic = vnic_dev_priv(rq->vdev);
+	if (rxq != NULL) {
+		struct vnic_rq *rq = (struct vnic_rq *)rxq;
+		struct enic *enic = vnic_dev_priv(rq->vdev);
 
-	enic_rxmbuf_queue_release(enic, rq);
-	rte_free(rq->mbuf_ring);
-	rq->mbuf_ring = NULL;
-	vnic_rq_free(rq);
-	vnic_cq_free(&enic->cq[rq->index]);
+		enic_rxmbuf_queue_release(enic, rq);
+		rte_free(rq->mbuf_ring);
+		rq->mbuf_ring = NULL;
+		vnic_rq_free(rq);
+		vnic_cq_free(&enic->cq[rq->index]);
+	}
 }
 
 void enic_start_wq(struct enic *enic, uint16_t queue_idx)
@@ -816,22 +818,29 @@ static void enic_dev_deinit(struct enic *enic)
 int enic_set_vnic_res(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	int rc = 0;
 
-	if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-		(enic->wq_count < eth_dev->data->nb_tx_queues)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+	if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+		dev_err(dev, "Not enough Receive queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_rx_queues, enic->rq_count);
+		rc = -1;
+	}
+	if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+		dev_err(dev, "Not enough Transmit queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_tx_queues, enic->wq_count);
+		rc = -1;
 	}
 
 	enic->rq_count = eth_dev->data->nb_rx_queues;
 	enic->wq_count = eth_dev->data->nb_tx_queues;
 	if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+		dev_err(dev, "Not enough Completion queues. Required:%u, Configured:%u\n",
+			enic->rq_count + enic->wq_count, enic->cq_count);
+		rc = -1;
 	}
 
 	enic->cq_count = enic->rq_count + enic->wq_count;
-	return 0;
+	return rc;
 }
 
 static int enic_dev_init(struct enic *enic)
-- 
2.7.0

Re: [dpdk-dev] [PATCH v2] enic: fix seg fault when releasing queues

[Bruce Richardson]

On Wed, May 25, 2016 at 07:45:00PM -0700, John Daley wrote:
> If device configuration failed due to a lack of resources, like if
> there were more queues requested than available, the queue release
> function is called with NULL pointers which were being dereferenced.
> 
> Skip releasing queues if they are NULL pointers. Also, if configuration
> fails due to lack of resources, be more specific about which resources
> are lacking.

The "also", and a a review of the code, indicates that the error message
changes should be a separate patch, as it's not related to the NULL check fix.
:-)


> 
> Fixes: fefed3d1e62c ("enic: new driver")
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
> v2: Log an error for all resource deficiencies not just the first one
> found.
> 
>  drivers/net/enic/enic_main.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 996f999..411d23c 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -426,14 +426,16 @@ int enic_alloc_intr_resources(struct enic *enic)
>  
>  void enic_free_rq(void *rxq)
>  {
> -	struct vnic_rq *rq = (struct vnic_rq *)rxq;
> -	struct enic *enic = vnic_dev_priv(rq->vdev);
> +	if (rxq != NULL) {
> +		struct vnic_rq *rq = (struct vnic_rq *)rxq;
> +		struct enic *enic = vnic_dev_priv(rq->vdev);
>  
> -	enic_rxmbuf_queue_release(enic, rq);
> -	rte_free(rq->mbuf_ring);
> -	rq->mbuf_ring = NULL;
> -	vnic_rq_free(rq);
> -	vnic_cq_free(&enic->cq[rq->index]);
> +		enic_rxmbuf_queue_release(enic, rq);
> +		rte_free(rq->mbuf_ring);
> +		rq->mbuf_ring = NULL;
> +		vnic_rq_free(rq);
> +		vnic_cq_free(&enic->cq[rq->index]);
> +	}
>  }

Is it not just easier to put a check at the top for: 
	if (rq == NULL)
		return;

Rather than putting the whole body of the function inside an if statement? It
would certainly be a smaller diff.

/Bruce

[dpdk-dev] [PATCH v3 0/2] enic: fix seg fault when releasing queues

[John Daley]

Patch broken into separate patches for fixing seg fault and improving
log messages.

John Daley (2):
  enic: fix seg fault when releasing queues
  enic: more specific out of resources error messages

 drivers/net/enic/enic_main.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

-- 
2.7.0

[dpdk-dev] [PATCH v3 1/2] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, such as
if more queues are requested than are available, the queue release
function is called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---

v3: bail out of free rq function if rq is null instead of if
around != NULL.

 drivers/net/enic/enic_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 996f999..5939b9d 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -429,6 +429,9 @@ void enic_free_rq(void *rxq)
 	struct vnic_rq *rq = (struct vnic_rq *)rxq;
 	struct enic *enic = vnic_dev_priv(rq->vdev);
 
+	if (rxq == NULL)
+		return;
+
 	enic_rxmbuf_queue_release(enic, rq);
 	rte_free(rq->mbuf_ring);
 	rq->mbuf_ring = NULL;
-- 
2.7.0

[dpdk-dev] [PATCH v3 2/2] enic: more specific out of resources error messages

[John Daley]

If configuration fails due to lack of resources, be more specific
about which resources are lacking - work queues, read queues or
completion queues.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---
v3: Log messages fix in separate patch. Log errors
for all lacking resources, not just 1st encountered.

 drivers/net/enic/enic_main.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 5939b9d..70776c2 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -819,22 +819,29 @@ static void enic_dev_deinit(struct enic *enic)
 int enic_set_vnic_res(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	int rc = 0;
 
-	if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-		(enic->wq_count < eth_dev->data->nb_tx_queues)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+	if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+		dev_err(dev, "Not enough Receive queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_rx_queues, enic->rq_count);
+		rc = -1;
+	}
+	if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+		dev_err(dev, "Not enough Transmit queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_tx_queues, enic->wq_count);
+		rc = -1;
 	}
 
 	enic->rq_count = eth_dev->data->nb_rx_queues;
 	enic->wq_count = eth_dev->data->nb_tx_queues;
 	if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+		dev_err(dev, "Not enough Completion queues. Required:%u, Configured:%u\n",
+			enic->rq_count + enic->wq_count, enic->cq_count);
+		rc = -1;
 	}
 
 	enic->cq_count = enic->rq_count + enic->wq_count;
-	return 0;
+	return rc;
 }
 
 static int enic_dev_init(struct enic *enic)
-- 
2.7.0

Re: [dpdk-dev] [PATCH v3 1/2] enic: fix seg fault when releasing queues

[Bruce Richardson]

On Thu, Jun 09, 2016 at 11:56:08AM -0700, John Daley wrote:
> If device configuration failed due to a lack of resources, such as
> if more queues are requested than are available, the queue release
> function is called with NULL pointers which were being dereferenced.
> 
> Skip releasing queues if they are NULL pointers.
> 
> Fixes: fefed3d1e62c ("enic: new driver")
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
> 
> v3: bail out of free rq function if rq is null instead of if
> around != NULL.
> 
>  drivers/net/enic/enic_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 996f999..5939b9d 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -429,6 +429,9 @@ void enic_free_rq(void *rxq)
>  	struct vnic_rq *rq = (struct vnic_rq *)rxq;
>  	struct enic *enic = vnic_dev_priv(rq->vdev);

This doesn't look safe to me. You are referencing the pointer before checking
it for NULL.

>  
> +	if (rxq == NULL)
> +		return;
> +

/Bruce

>  	enic_rxmbuf_queue_release(enic, rq);
>  	rte_free(rq->mbuf_ring);
>  	rq->mbuf_ring = NULL;
> -- 
> 2.7.0
> 

[dpdk-dev] [PATCH v4 1/2] enic: fix seg fault when releasing queues

[John Daley]

If device configuration failed due to a lack of resources, such as
if more queues are requested than are available, the queue release
functions are called with NULL pointers which were being dereferenced.

Skip releasing queues if they are NULL pointers.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---

v4: fix check for NULL before pointer is set

 drivers/net/enic/enic_main.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 996f999..738792e 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -426,9 +426,14 @@ int enic_alloc_intr_resources(struct enic *enic)
 
 void enic_free_rq(void *rxq)
 {
-	struct vnic_rq *rq = (struct vnic_rq *)rxq;
-	struct enic *enic = vnic_dev_priv(rq->vdev);
+	struct vnic_rq *rq;
+	struct enic *enic;
 
+	if (rxq == NULL)
+		return;
+
+	rq = (struct vnic_rq *)rxq;
+	enic = vnic_dev_priv(rq->vdev);
 	enic_rxmbuf_queue_release(enic, rq);
 	rte_free(rq->mbuf_ring);
 	rq->mbuf_ring = NULL;
@@ -514,9 +519,14 @@ err_exit:
 
 void enic_free_wq(void *txq)
 {
-	struct vnic_wq *wq = (struct vnic_wq *)txq;
-	struct enic *enic = vnic_dev_priv(wq->vdev);
+	struct vnic_wq *wq;
+	struct enic *enic;
+
+	if (txq == NULL)
+		return;
 
+	wq = (struct vnic_wq *)txq;
+	enic = vnic_dev_priv(wq->vdev);
 	rte_memzone_free(wq->cqmsg_rz);
 	vnic_wq_free(wq);
 	vnic_cq_free(&enic->cq[enic->rq_count + wq->index]);
-- 
2.7.0

[dpdk-dev] [PATCH v4 2/2] enic: improve out of resources error handling

[John Daley]

If configuration fails due to lack of resources, be more specific
about which resources are lacking - work queues, read queues or
completion queues. Return -EINVAL instead of -1 if more queeues
are requested than are available.

Fixes: fefed3d1e62c ("enic: new driver")
Signed-off-by: John Daley <johndale@cisco.com>
---

v4: only modify wq/rq/cq counts on success, return -EINVAL on error.

 drivers/net/enic/enic_main.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 738792e..32ecdae 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -826,22 +826,32 @@ static void enic_dev_deinit(struct enic *enic)
 int enic_set_vnic_res(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	int rc = 0;
 
-	if ((enic->rq_count < eth_dev->data->nb_rx_queues) ||
-		(enic->wq_count < eth_dev->data->nb_tx_queues)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+	if (enic->rq_count < eth_dev->data->nb_rx_queues) {
+		dev_err(dev, "Not enough Receive queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_rx_queues, enic->rq_count);
+		rc = -EINVAL;
+	}
+	if (enic->wq_count < eth_dev->data->nb_tx_queues) {
+		dev_err(dev, "Not enough Transmit queues. Requested:%u, Configured:%u\n",
+			eth_dev->data->nb_tx_queues, enic->wq_count);
+		rc = -EINVAL;
 	}
 
-	enic->rq_count = eth_dev->data->nb_rx_queues;
-	enic->wq_count = eth_dev->data->nb_tx_queues;
 	if (enic->cq_count < (enic->rq_count + enic->wq_count)) {
-		dev_err(dev, "Not enough resources configured, aborting\n");
-		return -1;
+		dev_err(dev, "Not enough Completion queues. Required:%u, Configured:%u\n",
+			enic->rq_count + enic->wq_count, enic->cq_count);
+		rc = -EINVAL;
 	}
 
-	enic->cq_count = enic->rq_count + enic->wq_count;
-	return 0;
+	if (rc == 0) {
+		enic->rq_count = eth_dev->data->nb_rx_queues;
+		enic->wq_count = eth_dev->data->nb_tx_queues;
+		enic->cq_count = enic->rq_count + enic->wq_count;
+	}
+
+	return rc;
 }
 
 static int enic_dev_init(struct enic *enic)
-- 
2.7.0

Re: [dpdk-dev] [PATCH v4 1/2] enic: fix seg fault when releasing queues

[Bruce Richardson]

On Sat, Jun 11, 2016 at 10:27:04AM -0700, John Daley wrote:
> If device configuration failed due to a lack of resources, such as
> if more queues are requested than are available, the queue release
> functions are called with NULL pointers which were being dereferenced.
> 
> Skip releasing queues if they are NULL pointers.
> 
> Fixes: fefed3d1e62c ("enic: new driver")
> Signed-off-by: John Daley <johndale@cisco.com>
Patchset applied to dpdk-next-net/rel_16_07

/Bruce