net/mlx5: fix ETH and VLAN items for E-Switch flows

Message ID 1548429450-27678-1-git-send-email-viacheslavo@mellanox.com
State Changes Requested, archived
Delegated to: Shahaf Shuler
Headers show
Series
  • net/mlx5: fix ETH and VLAN items for E-Switch flows
Related show

Checks

Context Check Description
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Viacheslav Ovsiienko Jan. 25, 2019, 3:17 p.m.
This patch fixes two issues for E-Switch Flows

- RTE_FLOW_ITEM_TYPE_ETH and RTE_FLOW_ITEM_TYPE_VLAN with NULL
in spec field caused the asserts in debug version and segfault
in release, fixed, now empty items are allowed and treated in
correctl way

- empty RTE_FLOW_ITEM_TYPE_VLAN now sets the ethernet type to
ETH_P_802.1Q, in previous version empty VLAN item was ignored

Fixes: 3d14ad9be30e ("net/mlx5: support ethernet type for tunnels on E-Switch")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow_tcf.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Shahaf Shuler Jan. 27, 2019, 11:19 a.m. | #1
Hi Slava,

Friday, January 25, 2019 5:18 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH] net/mlx5: fix ETH and VLAN items for E-Switch
> flows
> 
> This patch fixes two issues for E-Switch Flows
> 
> - RTE_FLOW_ITEM_TYPE_ETH and RTE_FLOW_ITEM_TYPE_VLAN with NULL
> in spec field caused the asserts in debug version and segfault in release,
> fixed, now empty items are allowed and treated in correctl way
> 
> - empty RTE_FLOW_ITEM_TYPE_VLAN now sets the ethernet type to
> ETH_P_802.1Q, in previous version empty VLAN item was ignored

w/ Verbs, for example, having VLAN spec w/o a specific match on the VLAN ID will make packets also w/o VLAN to match the flow.
The reason is the way the kernel driver build the VLAN flow item for the device.

Have you verified we don't hit the same case w/ TC as well? 

> 
> Fixes: 3d14ad9be30e ("net/mlx5: support ethernet type for tunnels on E-
> Switch")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow_tcf.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c
> b/drivers/net/mlx5/mlx5_flow_tcf.c
> index b8204df..916d3a0 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -2052,7 +2052,8 @@ struct pedit_parser {
>  					 mask.eth,
>  					 "no support for partial mask on"
>  					 " \"type\" field");
> -			assert(items->spec);
> +			if (!items->spec)
> +				break;
>  			spec.eth = items->spec;
>  			if (mask.eth->type &&
>  			    (item_flags & MLX5_FLOW_LAYER_TUNNEL) &&
> @@ -2123,7 +2124,8 @@ struct pedit_parser {
>  					 "outer eth_type conflict,"
>  					 " must be 802.1Q");
>  			outer_etype = RTE_BE16(ETH_P_8021Q);
> -			assert(items->spec);
> +			if (!items->spec)
> +				break;
>  			spec.vlan = items->spec;
>  			if (mask.vlan->inner_type &&
>  			    vlan_etype != RTE_BE16(ETH_P_ALL) && @@ -
> 2520,10 +2522,14 @@ struct pedit_parser {
>  		case RTE_FLOW_ITEM_TYPE_PORT_ID:
>  			break;
>  		case RTE_FLOW_ITEM_TYPE_ETH:
> +			if (!items->spec)
> +				break;
>  			size += SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) *
> 4;
>  				/* dst/src MAC addr and mask. */
>  			break;
>  		case RTE_FLOW_ITEM_TYPE_VLAN:
> +			if (!items->spec)
> +				break;
>  			size +=	SZ_NLATTR_TYPE_OF(uint16_t) +
>  				/* VLAN Ether type. */
>  				SZ_NLATTR_TYPE_OF(uint8_t) + /* VLAN
> prio. */ @@ -3319,6 +3325,8 @@ struct pedit_parser {
>  			assert(mask.eth);
>  			if (mask.eth == &flow_tcf_mask_empty.eth)
>  				break;
> +			if (!items->spec)
> +				break;
>  			spec.eth = items->spec;
>  			if (mask.eth->type) {
>  				if (item_flags &
> MLX5_FLOW_LAYER_TUNNEL) @@ -3363,12 +3371,13 @@ struct
> pedit_parser {
>  				 sizeof(flow_tcf_mask_supported.vlan),
>  				 error);
>  			assert(mask.vlan);
> -			if (mask.vlan == &flow_tcf_mask_empty.vlan)
> -				break;
> -			spec.vlan = items->spec;
>  			assert(outer_etype == RTE_BE16(ETH_P_ALL) ||
>  			       outer_etype == RTE_BE16(ETH_P_8021Q));
>  			outer_etype = RTE_BE16(ETH_P_8021Q);
> +			if (mask.vlan == &flow_tcf_mask_empty.vlan)
> +				break;
> +			assert(items->spec);
> +			spec.vlan = items->spec;
>  			if (mask.vlan->inner_type)
>  				vlan_etype = spec.vlan->inner_type;
>  			if (mask.vlan->tci & RTE_BE16(0xe000))
> --
> 1.8.3.1

Patch

diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
index b8204df..916d3a0 100644
--- a/drivers/net/mlx5/mlx5_flow_tcf.c
+++ b/drivers/net/mlx5/mlx5_flow_tcf.c
@@ -2052,7 +2052,8 @@  struct pedit_parser {
 					 mask.eth,
 					 "no support for partial mask on"
 					 " \"type\" field");
-			assert(items->spec);
+			if (!items->spec)
+				break;
 			spec.eth = items->spec;
 			if (mask.eth->type &&
 			    (item_flags & MLX5_FLOW_LAYER_TUNNEL) &&
@@ -2123,7 +2124,8 @@  struct pedit_parser {
 					 "outer eth_type conflict,"
 					 " must be 802.1Q");
 			outer_etype = RTE_BE16(ETH_P_8021Q);
-			assert(items->spec);
+			if (!items->spec)
+				break;
 			spec.vlan = items->spec;
 			if (mask.vlan->inner_type &&
 			    vlan_etype != RTE_BE16(ETH_P_ALL) &&
@@ -2520,10 +2522,14 @@  struct pedit_parser {
 		case RTE_FLOW_ITEM_TYPE_PORT_ID:
 			break;
 		case RTE_FLOW_ITEM_TYPE_ETH:
+			if (!items->spec)
+				break;
 			size += SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) * 4;
 				/* dst/src MAC addr and mask. */
 			break;
 		case RTE_FLOW_ITEM_TYPE_VLAN:
+			if (!items->spec)
+				break;
 			size +=	SZ_NLATTR_TYPE_OF(uint16_t) +
 				/* VLAN Ether type. */
 				SZ_NLATTR_TYPE_OF(uint8_t) + /* VLAN prio. */
@@ -3319,6 +3325,8 @@  struct pedit_parser {
 			assert(mask.eth);
 			if (mask.eth == &flow_tcf_mask_empty.eth)
 				break;
+			if (!items->spec)
+				break;
 			spec.eth = items->spec;
 			if (mask.eth->type) {
 				if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
@@ -3363,12 +3371,13 @@  struct pedit_parser {
 				 sizeof(flow_tcf_mask_supported.vlan),
 				 error);
 			assert(mask.vlan);
-			if (mask.vlan == &flow_tcf_mask_empty.vlan)
-				break;
-			spec.vlan = items->spec;
 			assert(outer_etype == RTE_BE16(ETH_P_ALL) ||
 			       outer_etype == RTE_BE16(ETH_P_8021Q));
 			outer_etype = RTE_BE16(ETH_P_8021Q);
+			if (mask.vlan == &flow_tcf_mask_empty.vlan)
+				break;
+			assert(items->spec);
+			spec.vlan = items->spec;
 			if (mask.vlan->inner_type)
 				vlan_etype = spec.vlan->inner_type;
 			if (mask.vlan->tci & RTE_BE16(0xe000))