diff mbox series

[1/7] net/mlx5: fix the modify field check of tag

Message ID 20230630054303.432238-1-bingz@nvidia.com (mailing list archive)
State Accepted, archived
Delegated to: Raslan Darawsheh
Headers
Series [1/7] net/mlx5: fix the modify field check of tag |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bing Zhao June 30, 2023, 5:43 a.m. UTC 
  The new member "tag_index" was added into the structure
"rte_flow_action_modify_data". It is the index of the header inside
encapsulation level. Both the "RTE_FLOW_FIELD_TAG" and
"MLX5_RTE_FLOW_FIELD_META_REG" types will use this member instead
of the "level", but the "level" will still be supported for the
compatibility.

In the validation stage, not only the "RTE_FLOW_FIELD_TAG" type, but
also the "MLX5_RTE_FLOW_FIELD_META_REG" needs to be checked.

In the meanwhile, lowering down the log priority will help to
prevent the flooding.

Fixes: 68c513e861ca ("net/mlx5: align implementation with modify API")
Cc: michaelba@nvidia.com

Signed-off-by: Bing Zhao <bingz@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 5 +++--
 drivers/net/mlx5/mlx5_flow.h | 7 ++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Stephen Hemminger June 30, 2023, 6:08 a.m. UTC | #1 
On Fri, 30 Jun 2023 08:43:03 +0300
Bing Zhao <bingz@nvidia.com> wrote:

> @@ -1117,9 +1117,10 @@ flow_dv_fetch_field(const uint8_t *data, uint32_t size)
>  static inline bool
>  flow_modify_field_support_tag_array(enum rte_flow_field_id field)
>  {
> -	switch (field) {
> +	switch ((int)field) {
>  	case RTE_FLOW_FIELD_TAG:
>  	case RTE_FLOW_FIELD_MPLS:
> +	case MLX5_RTE_FLOW_FIELD_META_REG:

Mixing internal and API fields seems like something that could get
easily broken by changes to rte_flow.
Bing Zhao July 3, 2023, 1:31 p.m. UTC | #2 
Hi Stephen,
If I understand correctly, do you mean that the internal value and rte_flow API value may have some conflict?
All the MLX5 internal enum values start from INT_MIN. When treating it as a int value, it would not have the same value with rte_flow enums, unless all the 2^^32 are defined.
But yes, this has some risk since there is no limitation of the values in the rte_flow API.

BR. Bing

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, June 30, 2023 2:09 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> dev@dpdk.org; Michael Baum <michaelba@nvidia.com>
> Subject: Re: [PATCH 1/7] net/mlx5: fix the modify field check of tag
> 
> External email: Use caution opening links or attachments
> 
> 
> On Fri, 30 Jun 2023 08:43:03 +0300
> Bing Zhao <bingz@nvidia.com> wrote:
> 
> > @@ -1117,9 +1117,10 @@ flow_dv_fetch_field(const uint8_t *data,
> > uint32_t size)  static inline bool
> > flow_modify_field_support_tag_array(enum rte_flow_field_id field)  {
> > -     switch (field) {
> > +     switch ((int)field) {
> >       case RTE_FLOW_FIELD_TAG:
> >       case RTE_FLOW_FIELD_MPLS:
> > +     case MLX5_RTE_FLOW_FIELD_META_REG:
> 
> Mixing internal and API fields seems like something that could get easily
> broken by changes to rte_flow.
Thomas Monjalon July 6, 2023, 9:37 a.m. UTC | #3 
03/07/2023 15:31, Bing Zhao:
> Hi Stephen,
> If I understand correctly, do you mean that the internal value and rte_flow API value may have some conflict?
> All the MLX5 internal enum values start from INT_MIN. When treating it as a int value, it would not have the same value with rte_flow enums, unless all the 2^^32 are defined.
> But yes, this has some risk since there is no limitation of the values in the rte_flow API.

We can assume it will never happen.
This is good to go.


> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, June 30, 2023 2:09 PM
> > To: Bing Zhao <bingz@nvidia.com>
> > Cc: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> > <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> > <suanmingm@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> > dev@dpdk.org; Michael Baum <michaelba@nvidia.com>
> > Subject: Re: [PATCH 1/7] net/mlx5: fix the modify field check of tag
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, 30 Jun 2023 08:43:03 +0300
> > Bing Zhao <bingz@nvidia.com> wrote:
> > 
> > > @@ -1117,9 +1117,10 @@ flow_dv_fetch_field(const uint8_t *data,
> > > uint32_t size)  static inline bool
> > > flow_modify_field_support_tag_array(enum rte_flow_field_id field)  {
> > > -     switch (field) {
> > > +     switch ((int)field) {
> > >       case RTE_FLOW_FIELD_TAG:
> > >       case RTE_FLOW_FIELD_MPLS:
> > > +     case MLX5_RTE_FLOW_FIELD_META_REG:
> > 
> > Mixing internal and API fields seems like something that could get easily
> > broken by changes to rte_flow.
>
Bing Zhao July 6, 2023, 9:59 a.m. UTC | #4 
Thank you.

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, July 6, 2023 5:37 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Bing Zhao
> <bingz@nvidia.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> dev@dpdk.org; Michael Baum <michaelba@nvidia.com>
> Subject: Re: [PATCH 1/7] net/mlx5: fix the modify field check of tag
> 
> External email: Use caution opening links or attachments
> 
> 
> 03/07/2023 15:31, Bing Zhao:
> > Hi Stephen,
> > If I understand correctly, do you mean that the internal value and rte_flow
> API value may have some conflict?
> > All the MLX5 internal enum values start from INT_MIN. When treating it as a
> int value, it would not have the same value with rte_flow enums, unless all the
> 2^^32 are defined.
> > But yes, this has some risk since there is no limitation of the values in the
> rte_flow API.
> 
> We can assume it will never happen.
> This is good to go.
> 
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Friday, June 30, 2023 2:09 PM
> > > To: Bing Zhao <bingz@nvidia.com>
> > > Cc: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> > > <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> > > <suanmingm@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> > > dev@dpdk.org; Michael Baum <michaelba@nvidia.com>
> > > Subject: Re: [PATCH 1/7] net/mlx5: fix the modify field check of tag
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Fri, 30 Jun 2023 08:43:03 +0300
> > > Bing Zhao <bingz@nvidia.com> wrote:
> > >
> > > > @@ -1117,9 +1117,10 @@ flow_dv_fetch_field(const uint8_t *data,
> > > > uint32_t size)  static inline bool
> > > > flow_modify_field_support_tag_array(enum rte_flow_field_id field)  {
> > > > -     switch (field) {
> > > > +     switch ((int)field) {
> > > >       case RTE_FLOW_FIELD_TAG:
> > > >       case RTE_FLOW_FIELD_MPLS:
> > > > +     case MLX5_RTE_FLOW_FIELD_META_REG:
> > >
> > > Mixing internal and API fields seems like something that could get
> > > easily broken by changes to rte_flow.
> >
> 
> 
> 
>
Raslan Darawsheh July 10, 2023, 10:34 a.m. UTC | #5 
Hi,

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Friday, June 30, 2023 8:43 AM
> To: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>
> Cc: dev@dpdk.org; Michael Baum <michaelba@nvidia.com>
> Subject: [PATCH 1/7] net/mlx5: fix the modify field check of tag
> 
> The new member "tag_index" was added into the structure
> "rte_flow_action_modify_data". It is the index of the header inside
> encapsulation level. Both the "RTE_FLOW_FIELD_TAG" and
> "MLX5_RTE_FLOW_FIELD_META_REG" types will use this member instead of
> the "level", but the "level" will still be supported for the compatibility.
> 
> In the validation stage, not only the "RTE_FLOW_FIELD_TAG" type, but also
> the "MLX5_RTE_FLOW_FIELD_META_REG" needs to be checked.
> 
> In the meanwhile, lowering down the log priority will help to prevent the
> flooding.
> 
> Fixes: 68c513e861ca ("net/mlx5: align implementation with modify API")
> Cc: michaelba@nvidia.com
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>

Series applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh
diff mbox series

Patch

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index cf83db7b60..97211fc209 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2411,7 +2411,8 @@  flow_validate_modify_field_level(const struct rte_flow_action_modify_data *data,
 {
 	if (data->level == 0)
 		return 0;
-	if (data->field != RTE_FLOW_FIELD_TAG)
+	if (data->field != RTE_FLOW_FIELD_TAG &&
+	    data->field != (enum rte_flow_field_id)MLX5_RTE_FLOW_FIELD_META_REG)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ACTION, NULL,
 					  "inner header fields modification is not supported");
@@ -2424,7 +2425,7 @@  flow_validate_modify_field_level(const struct rte_flow_action_modify_data *data,
 	 * 'tag_index' field. In old API, it was provided using 'level' field
 	 * and it is still supported for backwards compatibility.
 	 */
-	DRV_LOG(WARNING, "tag array provided in 'level' field instead of 'tag_index' field.");
+	DRV_LOG(DEBUG, "tag array provided in 'level' field instead of 'tag_index' field.");
 	return 0;
 }
 
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 003e7da3a6..d6e88425a3 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -52,8 +52,8 @@  enum mlx5_rte_flow_action_type {
 
 /* Private (internal) Field IDs for MODIFY_FIELD action. */
 enum mlx5_rte_flow_field_id {
-		MLX5_RTE_FLOW_FIELD_END = INT_MIN,
-			MLX5_RTE_FLOW_FIELD_META_REG,
+	MLX5_RTE_FLOW_FIELD_END = INT_MIN,
+	MLX5_RTE_FLOW_FIELD_META_REG,
 };
 
 #define MLX5_INDIRECT_ACTION_TYPE_OFFSET 29
@@ -1117,9 +1117,10 @@  flow_dv_fetch_field(const uint8_t *data, uint32_t size)
 static inline bool
 flow_modify_field_support_tag_array(enum rte_flow_field_id field)
 {
-	switch (field) {
+	switch ((int)field) {
 	case RTE_FLOW_FIELD_TAG:
 	case RTE_FLOW_FIELD_MPLS:
+	case MLX5_RTE_FLOW_FIELD_META_REG:
 		return true;
 	default:
 		break;