Re: [dpdk-dev] [PATCH v2] net/mlx5: fix validation of push VLAN without full mask

[Raslan Darawsheh <rasland@mellanox.com>]

Hi,
> -----Original Message-----
> From: Xiaoyu Min <jackmin@mellanox.com>
> Sent: Monday, April 13, 2020 6:33 AM
> To: Raslan Darawsheh <rasland@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Slava
> Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Dekel Peled <dekelp@mellanox.com>
> Subject: [PATCH v2] net/mlx5: fix validation of push VLAN without full mask
> 
> Due the limitation of HW, when PMD create push VLAN action it needs to
> know what exactly the value of VID/PCP.
> 
> PMD try to figure out them via:
>   - of_set_vlan_vid/pcp actions
>   - VLAN item in pattern
> If none of above is provided, default value - zero is used.
> 
> However user will write rule like [1] which match on a range of VID and
> without of_set_vlan_vid action and expect the VID will inherit from
> original packet. This is not supported by HW currently. PMD will set VID
> to default value - zero because it cannot figure out the exact value of VID
> from VLAN item.
> 
> This is sort of misleading for some users.
> 
> In order to avoid this, PMD will spit out error for rule like [1] to
> force user to provide explicit VID/PCP for new pushed VLAN headers.
> 
> Fixes: 9aee7a8418d4 ("net/mlx5: support push flow action on VLAN header")
> Cc: stable@dpdk.org
> 
> [1]: testpmd> flow create 2 ingress transfer group 0 priority 3 pattern
>                eth / vlan vid spec 2859 vid prefix 4 / ipv4 / end
> 	       actcions  of_push_vlan ethertype 0x88A8 /
> 	       of_set_vlan_pcp vlan_pcp 6 / port_id id 0 / end
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> Reviewed-by: Dekel Peled <dekelp@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
> v2:
>   - rebased
>   - added Acked-by tag
> 
>  drivers/net/mlx5/mlx5_flow_dv.c | 34
> +++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index 18ea577f8c..b1d6bf6e6c 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1829,7 +1829,7 @@ flow_dev_get_vlan_info_from_items(const struct
> rte_flow_item *items,
>  static int
>  flow_dv_validate_action_push_vlan(struct rte_eth_dev *dev,
>  				  uint64_t action_flags,
> -				  uint64_t item_flags __rte_unused,
> +				  const struct rte_flow_item_vlan *vlan_m,
>  				  const struct rte_flow_action *action,
>  				  const struct rte_flow_attr *attr,
>  				  struct rte_flow_error *error)
> @@ -1863,6 +1863,32 @@ flow_dv_validate_action_push_vlan(struct
> rte_eth_dev *dev,
> 
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>  					  "push vlan action for VF representor
> "
>  					  "not supported on NIC table");
> +	if (vlan_m &&
> +	    (vlan_m->tci & MLX5DV_FLOW_VLAN_PCP_MASK_BE) &&
> +	    (vlan_m->tci & MLX5DV_FLOW_VLAN_PCP_MASK_BE) !=
> +		MLX5DV_FLOW_VLAN_PCP_MASK_BE &&
> +	    !(action_flags & MLX5_FLOW_ACTION_OF_SET_VLAN_PCP) &&
> +	    !(mlx5_flow_find_action
> +		(action + 1, RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP)))
> +		return rte_flow_error_set(error, EINVAL,
> +					  RTE_FLOW_ERROR_TYPE_ACTION,
> action,
> +					  "not full match mask on VLAN PCP
> and "
> +					  "there is no of_set_vlan_pcp action,
> "
> +					  "push VLAN action cannot figure out
> "
> +					  "PCP value");
> +	if (vlan_m &&
> +	    (vlan_m->tci & MLX5DV_FLOW_VLAN_VID_MASK_BE) &&
> +	    (vlan_m->tci & MLX5DV_FLOW_VLAN_VID_MASK_BE) !=
> +		MLX5DV_FLOW_VLAN_VID_MASK_BE &&
> +	    !(action_flags & MLX5_FLOW_ACTION_OF_SET_VLAN_VID) &&
> +	    !(mlx5_flow_find_action
> +		(action + 1, RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_VID)))
> +		return rte_flow_error_set(error, EINVAL,
> +					  RTE_FLOW_ERROR_TYPE_ACTION,
> action,
> +					  "not full match mask on VLAN VID
> and "
> +					  "there is no of_set_vlan_vid action,
> "
> +					  "push VLAN action cannot figure out
> "
> +					  "VID value");
>  	(void)attr;
>  	return 0;
>  }
> @@ -4563,6 +4589,7 @@ flow_dv_validate(struct rte_eth_dev *dev, const
> struct rte_flow_attr *attr,
>  	struct mlx5_priv *priv = dev->data->dev_private;
>  	struct mlx5_dev_config *dev_conf = &priv->config;
>  	uint16_t queue_index = 0xFFFF;
> +	const struct rte_flow_item_vlan *vlan_m = NULL;
> 
>  	if (items == NULL)
>  		return -1;
> @@ -4620,6 +4647,9 @@ flow_dv_validate(struct rte_eth_dev *dev, const
> struct rte_flow_attr *attr,
>  			} else {
>  				ether_type = 0;
>  			}
> +			/* Store outer VLAN mask for of_push_vlan action.
> */
> +			if (!tunnel)
> +				vlan_m = items->mask;
>  			break;
>  		case RTE_FLOW_ITEM_TYPE_IPV4:
>  			mlx5_flow_tunnel_ip_check(items, next_protocol,
> @@ -4935,7 +4965,7 @@ flow_dv_validate(struct rte_eth_dev *dev, const
> struct rte_flow_attr *attr,
>  		case RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN:
>  			ret = flow_dv_validate_action_push_vlan(dev,
>  								action_flags,
> -								item_flags,
> +								vlan_m,
>  								actions, attr,
>  								error);
>  			if (ret < 0)
> --
> 2.26.0

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh