Re: [dpdk-dev] [PATCH] net/mlx5: update VLAN and encap actions validation

[Raslan Darawsheh <rasland@mellanox.com>]

Hi,

> -----Original Message-----
> From: Dekel Peled <dekelp@mellanox.com>
> Sent: Tuesday, March 24, 2020 2:58 PM
> To: Matan Azrad <matan@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/mlx5: update VLAN and encap actions validation
> 
> Flow rule in NIC table on VF representor should not contain VLAN pop
> or push actions, and encap or decap actions. Using these actions in
> NIC table on VF representor is not a valid use case.
> This patch updates the various validation functions to reject such
> rules.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> Acked-by: Jack Min <jackmin@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow_dv.c | 82
> +++++++++++++++++++++++++++++++++--------
>  1 file changed, 67 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index 2090631..7fe61c2 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1698,7 +1698,7 @@ struct field_modify_info modify_tcp[] = {
>  				 const struct rte_flow_attr *attr,
>  				 struct rte_flow_error *error)
>  {
> -	struct mlx5_priv *priv = dev->data->dev_private;
> +	const struct mlx5_priv *priv = dev->data->dev_private;
> 
>  	(void)action;
>  	(void)attr;
> @@ -1729,6 +1729,11 @@ struct field_modify_info modify_tcp[] = {
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> action,
>  					  "wrong action order, port_id should
> "
>  					  "be after pop VLAN action");
> +	if (!attr->transfer && priv->representor)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					  "pop vlan action for VF representor "
> +					  "not supported on NIC table");
>  	return 0;
>  }
> 
> @@ -1792,6 +1797,8 @@ struct field_modify_info modify_tcp[] = {
>  /**
>   * Validate the push VLAN action.
>   *
> + * @param[in] dev
> + *   Pointer to the rte_eth_dev structure.
>   * @param[in] action_flags
>   *   Holds the actions detected until now.
>   * @param[in] item_flags
> @@ -1807,13 +1814,15 @@ struct field_modify_info modify_tcp[] = {
>   *   0 on success, a negative errno value otherwise and rte_errno is set.
>   */
>  static int
> -flow_dv_validate_action_push_vlan(uint64_t action_flags,
> +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_action *action,
>  				  const struct rte_flow_attr *attr,
>  				  struct rte_flow_error *error)
>  {
>  	const struct rte_flow_action_of_push_vlan *push_vlan = action-
> >conf;
> +	const struct mlx5_priv *priv = dev->data->dev_private;
> 
>  	if (!attr->transfer && attr->ingress)
>  		return rte_flow_error_set(error, ENOTSUP,
> @@ -1836,6 +1845,11 @@ struct field_modify_info modify_tcp[] = {
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> action,
>  					  "wrong action order, port_id should
> "
>  					  "be after push VLAN");
> +	if (!attr->transfer && priv->representor)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					  "push vlan action for VF representor
> "
> +					  "not supported on NIC table");
>  	(void)attr;
>  	return 0;
>  }
> @@ -2201,10 +2215,14 @@ struct field_modify_info modify_tcp[] = {
>  /**
>   * Validate the L2 encap action.
>   *
> + * @param[in] dev
> + *   Pointer to the rte_eth_dev structure.
>   * @param[in] action_flags
>   *   Holds the actions detected until now.
>   * @param[in] action
>   *   Pointer to the action structure.
> + * @param[in] attr
> + *   Pointer to flow attributes.
>   * @param[out] error
>   *   Pointer to error structure.
>   *
> @@ -2212,10 +2230,14 @@ struct field_modify_info modify_tcp[] = {
>   *   0 on success, a negative errno value otherwise and rte_errno is set.
>   */
>  static int
> -flow_dv_validate_action_l2_encap(uint64_t action_flags,
> +flow_dv_validate_action_l2_encap(struct rte_eth_dev *dev,
> +				 uint64_t action_flags,
>  				 const struct rte_flow_action *action,
> +				 const struct rte_flow_attr *attr,
>  				 struct rte_flow_error *error)
>  {
> +	const struct mlx5_priv *priv = dev->data->dev_private;
> +
>  	if (!(action->conf))
>  		return rte_flow_error_set(error, EINVAL,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> action,
> @@ -2225,12 +2247,19 @@ struct field_modify_info modify_tcp[] = {
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "can only have a single encap action
> "
>  					  "in a flow");
> +	if (!attr->transfer && priv->representor)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					  "encap action for VF representor "
> +					  "not supported on NIC table");
>  	return 0;
>  }
> 
>  /**
>   * Validate a decap action.
>   *
> + * @param[in] dev
> + *   Pointer to the rte_eth_dev structure.
>   * @param[in] action_flags
>   *   Holds the actions detected until now.
>   * @param[in] attr
> @@ -2242,10 +2271,13 @@ struct field_modify_info modify_tcp[] = {
>   *   0 on success, a negative errno value otherwise and rte_errno is set.
>   */
>  static int
> -flow_dv_validate_action_decap(uint64_t action_flags,
> -				 const struct rte_flow_attr *attr,
> -				 struct rte_flow_error *error)
> +flow_dv_validate_action_decap(struct rte_eth_dev *dev,
> +			      uint64_t action_flags,
> +			      const struct rte_flow_attr *attr,
> +			      struct rte_flow_error *error)
>  {
> +	const struct mlx5_priv *priv = dev->data->dev_private;
> +
>  	if (action_flags & MLX5_FLOW_XCAP_ACTIONS)
>  		return rte_flow_error_set(error, ENOTSUP,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> @@ -2264,6 +2296,11 @@ struct field_modify_info modify_tcp[] = {
>  					  NULL,
>  					  "decap action not supported for "
>  					  "egress");
> +	if (!attr->transfer && priv->representor)
> +		return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					  "decap action for VF representor "
> +					  "not supported on NIC table");
>  	return 0;
>  }
> 
> @@ -2272,6 +2309,8 @@ struct field_modify_info modify_tcp[] = {
>  /**
>   * Validate the raw encap and decap actions.
>   *
> + * @param[in] dev
> + *   Pointer to the rte_eth_dev structure.
>   * @param[in] decap
>   *   Pointer to the decap action.
>   * @param[in] encap
> @@ -2290,11 +2329,13 @@ struct field_modify_info modify_tcp[] = {
>   */
>  static int
>  flow_dv_validate_action_raw_encap_decap
> -	(const struct rte_flow_action_raw_decap *decap,
> +	(struct rte_eth_dev *dev,
> +	 const struct rte_flow_action_raw_decap *decap,
>  	 const struct rte_flow_action_raw_encap *encap,
>  	 const struct rte_flow_attr *attr, uint64_t *action_flags,
>  	 int *actions_n, struct rte_flow_error *error)
>  {
> +	const struct mlx5_priv *priv = dev->data->dev_private;
>  	int ret;
> 
>  	if (encap && (!encap->size || !encap->data))
> @@ -2327,7 +2368,8 @@ struct field_modify_info modify_tcp[] = {
>  				"encap combination");
>  	}
>  	if (decap) {
> -		ret = flow_dv_validate_action_decap(*action_flags, attr,
> error);
> +		ret = flow_dv_validate_action_decap(dev, *action_flags,
> attr,
> +						    error);
>  		if (ret < 0)
>  			return ret;
>  		*action_flags |= MLX5_FLOW_ACTION_DECAP;
> @@ -2344,6 +2386,12 @@ struct field_modify_info modify_tcp[] = {
> 
> RTE_FLOW_ERROR_TYPE_ACTION,
>  						  NULL,
>  						  "more than one encap
> action");
> +		if (!attr->transfer && priv->representor)
> +			return rte_flow_error_set
> +					(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					 "encap action for VF representor "
> +					 "not supported on NIC table");
>  		*action_flags |= MLX5_FLOW_ACTION_ENCAP;
>  		++(*actions_n);
>  	}
> @@ -4888,7 +4936,8 @@ struct field_modify_info modify_tcp[] = {
>  			++actions_n;
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN:
> -			ret =
> flow_dv_validate_action_push_vlan(action_flags,
> +			ret = flow_dv_validate_action_push_vlan(dev,
> +								action_flags,
>  								item_flags,
>  								actions, attr,
>  								error);
> @@ -4916,8 +4965,10 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP:
>  		case RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP:
> -			ret =
> flow_dv_validate_action_l2_encap(action_flags,
> -							       actions, error);
> +			ret = flow_dv_validate_action_l2_encap(dev,
> +							       action_flags,
> +							       actions, attr,
> +							       error);
>  			if (ret < 0)
>  				return ret;
>  			action_flags |= MLX5_FLOW_ACTION_ENCAP;
> @@ -4925,8 +4976,8 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP:
>  		case RTE_FLOW_ACTION_TYPE_NVGRE_DECAP:
> -			ret = flow_dv_validate_action_decap(action_flags,
> attr,
> -							    error);
> +			ret = flow_dv_validate_action_decap(dev,
> action_flags,
> +							    attr, error);
>  			if (ret < 0)
>  				return ret;
>  			action_flags |= MLX5_FLOW_ACTION_DECAP;
> @@ -4934,7 +4985,7 @@ struct field_modify_info modify_tcp[] = {
>  			break;
>  		case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
>  			ret = flow_dv_validate_action_raw_encap_decap
> -				(NULL, actions->conf, attr, &action_flags,
> +				(dev, NULL, actions->conf, attr,
> &action_flags,
>  				 &actions_n, error);
>  			if (ret < 0)
>  				return ret;
> @@ -4950,7 +5001,8 @@ struct field_modify_info modify_tcp[] = {
>  				encap = actions->conf;
>  			}
>  			ret = flow_dv_validate_action_raw_encap_decap
> -					   (decap ? decap : &empty_decap,
> encap,
> +					   (dev,
> +					    decap ? decap : &empty_decap,
> encap,
>  					    attr, &action_flags, &actions_n,
>  					    error);
>  			if (ret < 0)
> --
> 1.8.3.1


Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh