diff mbox

[dpdk-dev,v2] net/mlx5: fix drop action seg fault

Message ID 09f627ea8059ff7fd490ea1120e5c68137fa4944.1498376886.git.shacharbe@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Shachar Beiser June 25, 2017, 7:53 a.m. UTC 
  Missing room in flow allocation to store the drop specification.
Changing flow without storing the change in rte_flow.
Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")

Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Nélio Laranjeiro June 26, 2017, 12:28 p.m. UTC | #1 
On Sun, Jun 25, 2017 at 07:55:01AM +0000, Shachar Beiser wrote:
> Missing room in flow allocation to store the drop specification.
> Changing flow without storing the change in rte_flow.
> Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")
> 
> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 12893c6..86be929 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -580,6 +580,10 @@ struct mlx5_flow_action {
>  	}
>  	if (action->mark && !flow->ibv_attr && !action->drop)
>  		flow->offset += sizeof(struct ibv_exp_flow_spec_action_tag);
> +#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
> +	if (!flow->ibv_attr && action->drop)
> +		flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
> +#endif
>  	if (!action->queue && !action->drop) {
>  		rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_HANDLE,
>  				   NULL, "no valid action");
> @@ -1011,9 +1015,6 @@ struct mlx5_flow_action {
>  		return NULL;
>  	}
>  	rte_flow->drop = 1;
> -	rte_flow->ibv_attr = flow->ibv_attr;
> -	if (!priv->started)
> -		return rte_flow;
>  #ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
>  	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
>  	*drop = (struct ibv_exp_flow_spec_action_drop){
> @@ -1023,6 +1024,9 @@ struct mlx5_flow_action {
>  	++flow->ibv_attr->num_of_specs;
>  	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
>  #endif
> +	rte_flow->ibv_attr = flow->ibv_attr;
> +	if (!priv->started)
> +		return rte_flow;
>  	rte_flow->qp = priv->flow_drop_queue->qp;
>  	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
>  						 rte_flow->ibv_attr);
> -- 
> 1.8.3.1
 
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Ferruh Yigit June 26, 2017, 12:55 p.m. UTC | #2 
On 6/26/2017 1:28 PM, Nélio Laranjeiro wrote:
> On Sun, Jun 25, 2017 at 07:55:01AM +0000, Shachar Beiser wrote:
>> Missing room in flow allocation to store the drop specification.
>> Changing flow without storing the change in rte_flow.
>> Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")
>>
>> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>

> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Hi Nelio, Shachar,

What do you think squashing this and [1] into 88c77dedfbb0? Both this
and [1] looks like fixing same commit.

[1]
e0e37c1f80a0 ("net/mlx5: fix creation of drop flows")
Nélio Laranjeiro June 26, 2017, 1:41 p.m. UTC | #3 
On Mon, Jun 26, 2017 at 01:55:33PM +0100, Ferruh Yigit wrote:
> On 6/26/2017 1:28 PM, Nélio Laranjeiro wrote:
> > On Sun, Jun 25, 2017 at 07:55:01AM +0000, Shachar Beiser wrote:
> >> Missing room in flow allocation to store the drop specification.
> >> Changing flow without storing the change in rte_flow.
> >> Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")
> >>
> >> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> 
> > Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> 
> Hi Nelio, Shachar,
> 
> What do you think squashing this and [1] into 88c77dedfbb0? Both this
> and [1] looks like fixing same commit.
> 
> [1]
> e0e37c1f80a0 ("net/mlx5: fix creation of drop flows")

Hi Ferruh,

No it is not the same issue, one this new patch is fixing some code for
the hardware drop flow support which will be available with next version
of MLNX_OFED.  

e0e37c1f80a0 ("net/mlx5: fix creation of drop flows") is fixing the
current version of the software drop queue which is possible with the
current MLNX_OFED GA.

Please keep them separate.

Thanks,
Ferruh Yigit June 26, 2017, 1:51 p.m. UTC | #4 
On 6/26/2017 2:41 PM, Nélio Laranjeiro wrote:
> On Mon, Jun 26, 2017 at 01:55:33PM +0100, Ferruh Yigit wrote:
>> On 6/26/2017 1:28 PM, Nélio Laranjeiro wrote:
>>> On Sun, Jun 25, 2017 at 07:55:01AM +0000, Shachar Beiser wrote:
>>>> Missing room in flow allocation to store the drop specification.
>>>> Changing flow without storing the change in rte_flow.
>>>> Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")
>>>>
>>>> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
>>
>>> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>
>> Hi Nelio, Shachar,
>>
>> What do you think squashing this and [1] into 88c77dedfbb0? Both this
>> and [1] looks like fixing same commit.
>>
>> [1]
>> e0e37c1f80a0 ("net/mlx5: fix creation of drop flows")
> 
> Hi Ferruh,
> 
> No it is not the same issue, one this new patch is fixing some code for
> the hardware drop flow support which will be available with next version
> of MLNX_OFED.  
> 
> e0e37c1f80a0 ("net/mlx5: fix creation of drop flows") is fixing the
> current version of the software drop queue which is possible with the
> current MLNX_OFED GA.
> 
> Please keep them separate.

OK, thanks for clarification.

> 
> Thanks,
>
Ferruh Yigit June 26, 2017, 2:12 p.m. UTC | #5 
On 6/26/2017 1:28 PM, Nélio Laranjeiro wrote:
> On Sun, Jun 25, 2017 at 07:55:01AM +0000, Shachar Beiser wrote:
>> Missing room in flow allocation to store the drop specification.
>> Changing flow without storing the change in rte_flow.
>> Fixes: 88c77dedfbb0 ("net/mlx5: implement drop action in hardware classifier")
>>
>> Signed-off-by: Shachar Beiser <shacharbe@mellanox.com>
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Squashed into relevant commit in next-net, thanks.
diff mbox

Patch

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 12893c6..86be929 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -580,6 +580,10 @@  struct mlx5_flow_action {
 	}
 	if (action->mark && !flow->ibv_attr && !action->drop)
 		flow->offset += sizeof(struct ibv_exp_flow_spec_action_tag);
+#ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
+	if (!flow->ibv_attr && action->drop)
+		flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
+#endif
 	if (!action->queue && !action->drop) {
 		rte_flow_error_set(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_HANDLE,
 				   NULL, "no valid action");
@@ -1011,9 +1015,6 @@  struct mlx5_flow_action {
 		return NULL;
 	}
 	rte_flow->drop = 1;
-	rte_flow->ibv_attr = flow->ibv_attr;
-	if (!priv->started)
-		return rte_flow;
 #ifdef HAVE_VERBS_IBV_EXP_FLOW_SPEC_ACTION_DROP
 	drop = (void *)((uintptr_t)flow->ibv_attr + flow->offset);
 	*drop = (struct ibv_exp_flow_spec_action_drop){
@@ -1023,6 +1024,9 @@  struct mlx5_flow_action {
 	++flow->ibv_attr->num_of_specs;
 	flow->offset += sizeof(struct ibv_exp_flow_spec_action_drop);
 #endif
+	rte_flow->ibv_attr = flow->ibv_attr;
+	if (!priv->started)
+		return rte_flow;
 	rte_flow->qp = priv->flow_drop_queue->qp;
 	rte_flow->ibv_flow = ibv_exp_create_flow(rte_flow->qp,
 						 rte_flow->ibv_attr);