[dpdk-dev] [PATCH] net/mlx5: fix zero value validation for metadata

[dpdk-dev] [PATCH] net/mlx5: fix zero value validation for metadata

[Wisam Jaddo]

MARK and META come and go to data path, so zero
value has special meaning to metadata actions/items.

The special meaning is to say that this packet don't
have META/MARK anymore - clear it.

Thus, metadata actions/items now allowed to set/match
on zero value as long as mask is not zero

Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
Fixes: e554b672aa05 ("net/mlx5: support flow tag")
Fixes: 55deee1715f0 ("net/mlx5: extend flow mark support")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
---
 doc/guides/nics/mlx5.rst        | 10 ++++++++++
 drivers/net/mlx5/mlx5_flow_dv.c | 20 ++++++++++++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index afd11cd..e1e2fa4 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -1267,6 +1267,16 @@ Supported hardware offloads
    |                       | |  ConnectX-5   | | ConnectX-5    |
    +-----------------------+-----------------+-----------------+
 
+Notes for metadata
+------------------
+MARK and META come and go to data path, so zero value has special meaning to
+metadata actions/items. The special meaning is to say that this packet don't
+have META/MARK anymore - clear it.
+
+Summery:
+1. Metadata actions allows setting meta, tag & mark to zero value.
+2. Match on metadata items is allowed, as long as mask is not zero.
+
 Notes for testpmd
 -----------------
 
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 2090631..09865f6 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1406,6 +1406,11 @@ flow_dv_validate_item_mark(struct rte_eth_dev *dev,
 					  "mark id exceeds the limit");
 	if (!mask)
 		mask = &nic_mask;
+	else if (!mask->id)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_mark),
@@ -1451,10 +1456,7 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 					  RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
 					  item->spec,
 					  "data cannot be empty");
-	if (!spec->data)
-		return rte_flow_error_set(error, EINVAL,
-					  RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
-					  "data cannot be zero");
+
 	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
 		if (!mlx5_flow_ext_mreg_supported(dev))
 			return rte_flow_error_set(error, ENOTSUP,
@@ -1474,6 +1476,11 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 	}
 	if (!mask)
 		mask = &rte_flow_item_meta_mask;
+	else if (!mask->data)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_meta),
@@ -1522,6 +1529,11 @@ flow_dv_validate_item_tag(struct rte_eth_dev *dev,
 					  "data cannot be empty");
 	if (!mask)
 		mask = &rte_flow_item_tag_mask;
+	else if (!mask->data)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_tag),
-- 
2.7.4

[dpdk-dev] [PATCH v2] net/mlx5: fix zero value validation for metadata

[Wisam Jaddo]

MARK and META items are interrelated with datapath -
they might move from/to the applications in mbuf.

zero value for these items has the special meaning -
it means "no metadata are provided", not zero values
are treated by applications and PMD as valid ones.

Moreover in the flow engine domain the value zero is
acceptable to match and set, and we should allow to
specify zero values as rte_flow parameters for the
META and MARK items and actions. In the same time
zero mask has no meaning and and should be rejected
on validation stage.

Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
Fixes: e554b672aa05 ("net/mlx5: support flow tag")
Fixes: 55deee1715f0 ("net/mlx5: extend flow mark support")
Cc: viacheslavo@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
---
Changes in v2
- Fix commit message
- Fix documentation
- Remove extra line
- Always check for zero mask
---
---
 doc/guides/nics/mlx5.rst        | 12 ++++++++++++
 drivers/net/mlx5/mlx5_flow_dv.c | 19 +++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index e8f9984df0..e13c07d9ab 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -1275,6 +1275,18 @@ Supported hardware offloads
    |                       | |  ConnectX-5   | | ConnectX-5    |
    +-----------------------+-----------------+-----------------+
 
+Notes for metadata
+------------------
+MARK and META items are interrelated with datapath - they might move from/to
+the applications in mbuf fields. Hence, zero value for these items has the
+special meaning - it means "no metadata are provided", not zero values are
+treated by applications and PMD as valid ones.
+
+Moreover in the flow engine domain the value zero is acceptable to match and
+set, and we should allow to specify zero values as rte_flow parameters for the
+META and MARK items and actions. In the same time zero mask has no meaning and
+should be rejected on validation stage.
+
 Notes for testpmd
 -----------------
 
diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
index 809833b7ee..da4a925404 100644
--- a/drivers/net/mlx5/mlx5_flow_dv.c
+++ b/drivers/net/mlx5/mlx5_flow_dv.c
@@ -1406,6 +1406,11 @@ flow_dv_validate_item_mark(struct rte_eth_dev *dev,
 					  "mark id exceeds the limit");
 	if (!mask)
 		mask = &nic_mask;
+	if (!mask->id)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_mark),
@@ -1451,10 +1456,6 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 					  RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
 					  item->spec,
 					  "data cannot be empty");
-	if (!spec->data)
-		return rte_flow_error_set(error, EINVAL,
-					  RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
-					  "data cannot be zero");
 	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
 		if (!mlx5_flow_ext_mreg_supported(dev))
 			return rte_flow_error_set(error, ENOTSUP,
@@ -1474,6 +1475,11 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev __rte_unused,
 	}
 	if (!mask)
 		mask = &rte_flow_item_meta_mask;
+	if (!mask->data)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_meta),
@@ -1522,6 +1528,11 @@ flow_dv_validate_item_tag(struct rte_eth_dev *dev,
 					  "data cannot be empty");
 	if (!mask)
 		mask = &rte_flow_item_tag_mask;
+	if (!mask->data)
+		return rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
+					"mask cannot be zero");
+
 	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
 					(const uint8_t *)&nic_mask,
 					sizeof(struct rte_flow_item_tag),
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix zero value validation for metadata

[Slava Ovsiienko]

> -----Original Message-----
> From: Wisam Monther <wisamm@mellanox.com>
> Sent: Thursday, March 26, 2020 12:22
> To: Ori Kam <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>;
> dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; stable@dpdk.org
> Subject: [PATCH v2] net/mlx5: fix zero value validation for metadata
> 
> MARK and META items are interrelated with datapath - they might move
> from/to the applications in mbuf.
> 
> zero value for these items has the special meaning - it means "no metadata
> are provided", not zero values are treated by applications and PMD as valid
> ones.
> 
> Moreover in the flow engine domain the value zero is acceptable to match
> and set, and we should allow to specify zero values as rte_flow parameters
> for the META and MARK items and actions. In the same time zero mask has
> no meaning and and should be rejected on validation stage.
> 
> Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
> Fixes: e554b672aa05 ("net/mlx5: support flow tag")
> Fixes: 55deee1715f0 ("net/mlx5: extend flow mark support")
> Cc: viacheslavo@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

> ---
> Changes in v2
> - Fix commit message
> - Fix documentation
> - Remove extra line
> - Always check for zero mask
> ---
> ---
>  doc/guides/nics/mlx5.rst        | 12 ++++++++++++
>  drivers/net/mlx5/mlx5_flow_dv.c | 19 +++++++++++++++----
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> e8f9984df0..e13c07d9ab 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -1275,6 +1275,18 @@ Supported hardware offloads
>     |                       | |  ConnectX-5   | | ConnectX-5    |
>     +-----------------------+-----------------+-----------------+
> 
> +Notes for metadata
> +------------------
> +MARK and META items are interrelated with datapath - they might move
> +from/to the applications in mbuf fields. Hence, zero value for these
> +items has the special meaning - it means "no metadata are provided",
> +not zero values are treated by applications and PMD as valid ones.
> +
> +Moreover in the flow engine domain the value zero is acceptable to
> +match and set, and we should allow to specify zero values as rte_flow
> +parameters for the META and MARK items and actions. In the same time
> +zero mask has no meaning and should be rejected on validation stage.
> +
>  Notes for testpmd
>  -----------------
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c index 809833b7ee..da4a925404 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1406,6 +1406,11 @@ flow_dv_validate_item_mark(struct rte_eth_dev
> *dev,
>  					  "mark id exceeds the limit");
>  	if (!mask)
>  		mask = &nic_mask;
> +	if (!mask->id)
> +		return rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
> NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_mark),
> @@ -1451,10 +1456,6 @@ flow_dv_validate_item_meta(struct rte_eth_dev
> *dev __rte_unused,
> 
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
>  					  item->spec,
>  					  "data cannot be empty");
> -	if (!spec->data)
> -		return rte_flow_error_set(error, EINVAL,
> -
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
> -					  "data cannot be zero");
>  	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
>  		if (!mlx5_flow_ext_mreg_supported(dev))
>  			return rte_flow_error_set(error, ENOTSUP, @@ -
> 1474,6 +1475,11 @@ flow_dv_validate_item_meta(struct rte_eth_dev *dev
> __rte_unused,
>  	}
>  	if (!mask)
>  		mask = &rte_flow_item_meta_mask;
> +	if (!mask->data)
> +		return rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
> NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_meta),
> @@ -1522,6 +1528,11 @@ flow_dv_validate_item_tag(struct rte_eth_dev
> *dev,
>  					  "data cannot be empty");
>  	if (!mask)
>  		mask = &rte_flow_item_tag_mask;
> +	if (!mask->data)
> +		return rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
> NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_tag),
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix zero value validation for metadata

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Wisam Monther <wisamm@mellanox.com>
> Sent: Thursday, March 26, 2020 12:22 PM
> To: Ori Kam <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>;
> dev@dpdk.org
> Cc: Raslan Darawsheh <rasland@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; stable@dpdk.org
> Subject: [PATCH v2] net/mlx5: fix zero value validation for metadata
> 
> MARK and META items are interrelated with datapath -
> they might move from/to the applications in mbuf.
> 
> zero value for these items has the special meaning -
> it means "no metadata are provided", not zero values
> are treated by applications and PMD as valid ones.
> 
> Moreover in the flow engine domain the value zero is
> acceptable to match and set, and we should allow to
> specify zero values as rte_flow parameters for the
> META and MARK items and actions. In the same time
> zero mask has no meaning and and should be rejected
> on validation stage.
> 
> Fixes: fcc8d2f716fd ("net/mlx5: extend flow metadata support")
> Fixes: e554b672aa05 ("net/mlx5: support flow tag")
> Fixes: 55deee1715f0 ("net/mlx5: extend flow mark support")
> Cc: viacheslavo@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> ---
> Changes in v2
> - Fix commit message
> - Fix documentation
> - Remove extra line
> - Always check for zero mask
> ---
> ---
>  doc/guides/nics/mlx5.rst        | 12 ++++++++++++
>  drivers/net/mlx5/mlx5_flow_dv.c | 19 +++++++++++++++----
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
> index e8f9984df0..e13c07d9ab 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -1275,6 +1275,18 @@ Supported hardware offloads
>     |                       | |  ConnectX-5   | | ConnectX-5    |
>     +-----------------------+-----------------+-----------------+
> 
> +Notes for metadata
> +------------------
> +MARK and META items are interrelated with datapath - they might move
> from/to
> +the applications in mbuf fields. Hence, zero value for these items has the
> +special meaning - it means "no metadata are provided", not zero values are
> +treated by applications and PMD as valid ones.
> +
> +Moreover in the flow engine domain the value zero is acceptable to match
> and
> +set, and we should allow to specify zero values as rte_flow parameters for
> the
> +META and MARK items and actions. In the same time zero mask has no
> meaning and
> +should be rejected on validation stage.
> +
>  Notes for testpmd
>  -----------------
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index 809833b7ee..da4a925404 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -1406,6 +1406,11 @@ flow_dv_validate_item_mark(struct rte_eth_dev
> *dev,
>  					  "mark id exceeds the limit");
>  	if (!mask)
>  		mask = &nic_mask;
> +	if (!mask->id)
> +		return rte_flow_error_set(error, EINVAL,
> +
> 	RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_mark),
> @@ -1451,10 +1456,6 @@ flow_dv_validate_item_meta(struct rte_eth_dev
> *dev __rte_unused,
> 
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
>  					  item->spec,
>  					  "data cannot be empty");
> -	if (!spec->data)
> -		return rte_flow_error_set(error, EINVAL,
> -
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
> -					  "data cannot be zero");
>  	if (config->dv_xmeta_en != MLX5_XMETA_MODE_LEGACY) {
>  		if (!mlx5_flow_ext_mreg_supported(dev))
>  			return rte_flow_error_set(error, ENOTSUP,
> @@ -1474,6 +1475,11 @@ flow_dv_validate_item_meta(struct rte_eth_dev
> *dev __rte_unused,
>  	}
>  	if (!mask)
>  		mask = &rte_flow_item_meta_mask;
> +	if (!mask->data)
> +		return rte_flow_error_set(error, EINVAL,
> +
> 	RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_meta),
> @@ -1522,6 +1528,11 @@ flow_dv_validate_item_tag(struct rte_eth_dev
> *dev,
>  					  "data cannot be empty");
>  	if (!mask)
>  		mask = &rte_flow_item_tag_mask;
> +	if (!mask->data)
> +		return rte_flow_error_set(error, EINVAL,
> +
> 	RTE_FLOW_ERROR_TYPE_ITEM_SPEC, NULL,
> +					"mask cannot be zero");
> +
>  	ret = mlx5_flow_item_acceptable(item, (const uint8_t *)mask,
>  					(const uint8_t *)&nic_mask,
>  					sizeof(struct rte_flow_item_tag),
> --
> 2.17.1


Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh