[dpdk-dev] [PATCH] net/mlx4: fix drop action setting before start

[dpdk-dev] [PATCH] net/mlx4: fix drop action setting before start

[Matan Azrad]

The corrupted code causes segmentation fault when user creates
flow with drop action before device starting.

For example, failsafe PMD recreates all the flows before calling
dev_start in plug-in sequence and mlx4 allocated its flow drop
queue in dev_start.
Hence, when failsafe created flow with drop action after plug-in
event, mlx4 tried to dereference flow drop queue which was uninitialized.

The fix changed the device private structure to hold the flow drop
queue by value instead of by reference.
Hence, the flow drop queue dynamic allocation and free were removed, and
all the accesses to its internal fields were changed.

The segmentation fault should not occur anymore because the memory
of flow drop queue is always allocated in configuration time.

Fixes: 642fe56a1ba5 ("net/mlx4: use a single drop queue for all drop flows")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx4/mlx4.h      |  7 ++++++-
 drivers/net/mlx4/mlx4_flow.c | 32 +++++++-------------------------
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index a2e0ae7..ecaab35 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -309,6 +309,11 @@ struct txq {
 
 struct rte_flow;
 
+struct rte_flow_drop {
+	struct ibv_qp *qp; /**< Verbs queue pair. */
+	struct ibv_cq *cq; /**< Verbs completion queue. */
+};
+
 struct priv {
 	struct rte_eth_dev *dev; /* Ethernet device. */
 	struct ibv_context *ctx; /* Verbs context. */
@@ -352,7 +357,7 @@ struct priv {
 	struct txq *(*txqs)[]; /* TX queues. */
 	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
 	struct rte_intr_handle intr_handle; /* Interrupt handler. */
-	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
+	struct rte_flow_drop flow_drop_queue; /* Flow drop queue. */
 	LIST_HEAD(mlx4_flows, rte_flow) flows;
 	struct rte_intr_conf intr_conf; /* Active interrupt configuration. */
 	LIST_HEAD(mlx4_parents, rxq) parents;
diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index b998bb9..a398f46 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -103,11 +103,6 @@ struct mlx4_flow_items {
 	const enum rte_flow_item_type *const items;
 };
 
-struct rte_flow_drop {
-	struct ibv_qp *qp; /**< Verbs queue pair. */
-	struct ibv_cq *cq; /**< Verbs completion queue. */
-};
-
 /** Valid action for this PMD. */
 static const enum rte_flow_action_type valid_actions[] = {
 	RTE_FLOW_ACTION_TYPE_DROP,
@@ -795,13 +790,9 @@ struct rte_flow_drop {
 static void
 mlx4_flow_destroy_drop_queue(struct priv *priv)
 {
-	if (priv->flow_drop_queue) {
-		struct rte_flow_drop *fdq = priv->flow_drop_queue;
-
-		priv->flow_drop_queue = NULL;
-		claim_zero(ibv_destroy_qp(fdq->qp));
-		claim_zero(ibv_destroy_cq(fdq->cq));
-		rte_free(fdq);
+	if (priv->flow_drop_queue.cq) {
+		claim_zero(ibv_destroy_qp(priv->flow_drop_queue.qp));
+		claim_zero(ibv_destroy_cq(priv->flow_drop_queue.cq));
 	}
 }
 
@@ -819,20 +810,14 @@ struct rte_flow_drop {
 {
 	struct ibv_qp *qp;
 	struct ibv_cq *cq;
-	struct rte_flow_drop *fdq;
 
-	fdq = rte_calloc(__func__, 1, sizeof(*fdq), 0);
-	if (!fdq) {
-		ERROR("Cannot allocate memory for drop struct");
-		goto err;
-	}
 	cq = ibv_exp_create_cq(priv->ctx, 1, NULL, NULL, 0,
 			      &(struct ibv_exp_cq_init_attr){
 					.comp_mask = 0,
 			      });
 	if (!cq) {
 		ERROR("Cannot create drop CQ");
-		goto err_create_cq;
+		goto err;
 	}
 	qp = ibv_exp_create_qp(priv->ctx,
 			      &(struct ibv_exp_qp_init_attr){
@@ -853,16 +838,13 @@ struct rte_flow_drop {
 		ERROR("Cannot create drop QP");
 		goto err_create_qp;
 	}
-	*fdq = (struct rte_flow_drop){
+	priv->flow_drop_queue = (struct rte_flow_drop){
 		.qp = qp,
 		.cq = cq,
 	};
-	priv->flow_drop_queue = fdq;
 	return 0;
 err_create_qp:
 	claim_zero(ibv_destroy_cq(cq));
-err_create_cq:
-	rte_free(fdq);
 err:
 	return -1;
 }
@@ -977,7 +959,7 @@ struct rte_flow_drop {
 		return NULL;
 	}
 	if (action->drop) {
-		qp = priv->flow_drop_queue->qp;
+		qp = priv->flow_drop_queue.qp;
 	} else {
 		int ret;
 		unsigned int i;
@@ -1307,7 +1289,7 @@ struct rte_flow *
 	for (flow = LIST_FIRST(&priv->flows);
 	     flow;
 	     flow = LIST_NEXT(flow, next)) {
-		qp = flow->qp ? flow->qp : priv->flow_drop_queue->qp;
+		qp = flow->qp ? flow->qp : priv->flow_drop_queue.qp;
 		flow->ibv_flow = ibv_create_flow(qp, flow->ibv_attr);
 		if (!flow->ibv_flow) {
 			DEBUG("Flow %p cannot be applied", (void *)flow);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx4: fix drop action setting before start

[Adrien Mazarguil]

Hi Matan,

On Tue, Jul 25, 2017 at 02:18:28PM +0300, Matan Azrad wrote:
> The corrupted code causes segmentation fault when user creates
> flow with drop action before device starting.

Thanks for debugging this issue. This should address the crash but I'm
concerned about its root cause, as you've described:

> For example, failsafe PMD recreates all the flows before calling
> dev_start in plug-in sequence and mlx4 allocated its flow drop
> queue in dev_start.
> Hence, when failsafe created flow with drop action after plug-in
> event, mlx4 tried to dereference flow drop queue which was uninitialized.

So question is, how come the mlx4 PMD attempts to create underlying flows if
the device is not yet started? In my opinion it should only record the flow
and refrain from applying it (by calling ibv_create_flow()) until the
application requests the device to be started.

> The fix changed the device private structure to hold the flow drop
> queue by value instead of by reference.
> Hence, the flow drop queue dynamic allocation and free were removed, and
> all the accesses to its internal fields were changed.
> 
> The segmentation fault should not occur anymore because the memory
> of flow drop queue is always allocated in configuration time.
> 
> Fixes: 642fe56a1ba5 ("net/mlx4: use a single drop queue for all drop flows")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

In the current code this drop queue is unconditionally created by
mlx4_priv_flow_start() before applying the remaining flows. So it makes
sense to assume no flows are supposed to be applied before the device is
started, right?

Then the problem is actually when a user calls rte_flow_create(),
priv_flow_create_action_queue() does not check the started state of the
device before applying the flow, because if it did, the drop queue would
have been present.

Fixing this behavior is also necessary since it causes the device to receive
packets through configured flows even though the application hasn't started
it yet. This is a major issue present since the beginning in commit:

 46d5736a7049 ("net/mlx4: support basic flow items and actions")

Therefore this patch does not address the root cause, I think it should just
add a check on the started flag where appropriate. Making the drop queue
allocation more static shouldn't be necessary afterward.

> ---
>  drivers/net/mlx4/mlx4.h      |  7 ++++++-
>  drivers/net/mlx4/mlx4_flow.c | 32 +++++++-------------------------
>  2 files changed, 13 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
> index a2e0ae7..ecaab35 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -309,6 +309,11 @@ struct txq {
>  
>  struct rte_flow;
>  
> +struct rte_flow_drop {
> +	struct ibv_qp *qp; /**< Verbs queue pair. */
> +	struct ibv_cq *cq; /**< Verbs completion queue. */
> +};
> +
>  struct priv {
>  	struct rte_eth_dev *dev; /* Ethernet device. */
>  	struct ibv_context *ctx; /* Verbs context. */
> @@ -352,7 +357,7 @@ struct priv {
>  	struct txq *(*txqs)[]; /* TX queues. */
>  	struct rte_intr_handle intr_handle_dev; /* Device interrupt handler. */
>  	struct rte_intr_handle intr_handle; /* Interrupt handler. */
> -	struct rte_flow_drop *flow_drop_queue; /* Flow drop queue. */
> +	struct rte_flow_drop flow_drop_queue; /* Flow drop queue. */
>  	LIST_HEAD(mlx4_flows, rte_flow) flows;
>  	struct rte_intr_conf intr_conf; /* Active interrupt configuration. */
>  	LIST_HEAD(mlx4_parents, rxq) parents;
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index b998bb9..a398f46 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -103,11 +103,6 @@ struct mlx4_flow_items {
>  	const enum rte_flow_item_type *const items;
>  };
>  
> -struct rte_flow_drop {
> -	struct ibv_qp *qp; /**< Verbs queue pair. */
> -	struct ibv_cq *cq; /**< Verbs completion queue. */
> -};
> -
>  /** Valid action for this PMD. */
>  static const enum rte_flow_action_type valid_actions[] = {
>  	RTE_FLOW_ACTION_TYPE_DROP,
> @@ -795,13 +790,9 @@ struct rte_flow_drop {
>  static void
>  mlx4_flow_destroy_drop_queue(struct priv *priv)
>  {
> -	if (priv->flow_drop_queue) {
> -		struct rte_flow_drop *fdq = priv->flow_drop_queue;
> -
> -		priv->flow_drop_queue = NULL;
> -		claim_zero(ibv_destroy_qp(fdq->qp));
> -		claim_zero(ibv_destroy_cq(fdq->cq));
> -		rte_free(fdq);
> +	if (priv->flow_drop_queue.cq) {
> +		claim_zero(ibv_destroy_qp(priv->flow_drop_queue.qp));
> +		claim_zero(ibv_destroy_cq(priv->flow_drop_queue.cq));
>  	}
>  }
>  
> @@ -819,20 +810,14 @@ struct rte_flow_drop {
>  {
>  	struct ibv_qp *qp;
>  	struct ibv_cq *cq;
> -	struct rte_flow_drop *fdq;
>  
> -	fdq = rte_calloc(__func__, 1, sizeof(*fdq), 0);
> -	if (!fdq) {
> -		ERROR("Cannot allocate memory for drop struct");
> -		goto err;
> -	}
>  	cq = ibv_exp_create_cq(priv->ctx, 1, NULL, NULL, 0,
>  			      &(struct ibv_exp_cq_init_attr){
>  					.comp_mask = 0,
>  			      });
>  	if (!cq) {
>  		ERROR("Cannot create drop CQ");
> -		goto err_create_cq;
> +		goto err;
>  	}
>  	qp = ibv_exp_create_qp(priv->ctx,
>  			      &(struct ibv_exp_qp_init_attr){
> @@ -853,16 +838,13 @@ struct rte_flow_drop {
>  		ERROR("Cannot create drop QP");
>  		goto err_create_qp;
>  	}
> -	*fdq = (struct rte_flow_drop){
> +	priv->flow_drop_queue = (struct rte_flow_drop){
>  		.qp = qp,
>  		.cq = cq,
>  	};
> -	priv->flow_drop_queue = fdq;
>  	return 0;
>  err_create_qp:
>  	claim_zero(ibv_destroy_cq(cq));
> -err_create_cq:
> -	rte_free(fdq);
>  err:
>  	return -1;
>  }
> @@ -977,7 +959,7 @@ struct rte_flow_drop {
>  		return NULL;
>  	}
>  	if (action->drop) {
> -		qp = priv->flow_drop_queue->qp;
> +		qp = priv->flow_drop_queue.qp;
>  	} else {
>  		int ret;
>  		unsigned int i;
> @@ -1307,7 +1289,7 @@ struct rte_flow *
>  	for (flow = LIST_FIRST(&priv->flows);
>  	     flow;
>  	     flow = LIST_NEXT(flow, next)) {
> -		qp = flow->qp ? flow->qp : priv->flow_drop_queue->qp;
> +		qp = flow->qp ? flow->qp : priv->flow_drop_queue.qp;
>  		flow->ibv_flow = ibv_create_flow(qp, flow->ibv_attr);
>  		if (!flow->ibv_flow) {
>  			DEBUG("Flow %p cannot be applied", (void *)flow);
> -- 
> 1.8.3.1
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v2] net/mlx4: fix flow creation before start

[Matan Azrad]

The corrupted code causes segmentation fault when user creates
flow with drop action before device starting.

For example, failsafe PMD recreates all the flows before calling
dev_start in plug-in sequence and mlx4 allocated its flow drop
queue in dev_start.
Hence, when failsafe created flow with drop action after plug-in
event, mlx4 tried to dereference flow drop queue which was uninitialized.

The fix added check to the drop qp accesibale and conditioned the
ibv_create_flow calling on device starting.

Fixes: 642fe56a1ba5 ("net/mlx4: use a single drop queue for all drop flows")
Fixes: 46d5736a7049 ("net/mlx4: support basic flow items and actions")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4_flow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

v2 to fix the qp accesible by other way and fix the ibv_create_flow calling
before dev_start.  

diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index 8ade106..925c89c 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -977,7 +977,7 @@ struct rte_flow_drop {
 		return NULL;
 	}
 	if (action->drop) {
-		qp = priv->flow_drop_queue->qp;
+		qp = priv->flow_drop_queue ? priv->flow_drop_queue->qp : NULL;
 	} else {
 		int ret;
 		unsigned int i;
@@ -1015,6 +1015,8 @@ struct rte_flow_drop {
 		rte_flow->qp = qp;
 	}
 	rte_flow->ibv_attr = ibv_attr;
+	if (!priv->started)
+		return rte_flow;
 	rte_flow->ibv_flow = ibv_create_flow(qp, rte_flow->ibv_attr);
 	if (!rte_flow->ibv_flow) {
 		rte_flow_error_set(error, ENOMEM, RTE_FLOW_ERROR_TYPE_HANDLE,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx4: fix flow creation before start

[Ferruh Yigit]

On 7/27/2017 11:27 AM, Matan Azrad wrote:
> The corrupted code causes segmentation fault when user creates
> flow with drop action before device starting.
> 
> For example, failsafe PMD recreates all the flows before calling
> dev_start in plug-in sequence and mlx4 allocated its flow drop
> queue in dev_start.
> Hence, when failsafe created flow with drop action after plug-in
> event, mlx4 tried to dereference flow drop queue which was uninitialized.
> 
> The fix added check to the drop qp accesibale and conditioned the
> ibv_create_flow calling on device starting.
> 
> Fixes: 642fe56a1ba5 ("net/mlx4: use a single drop queue for all drop flows")
> Fixes: 46d5736a7049 ("net/mlx4: support basic flow items and actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied to dpdk-next-net/master, thanks.