ethdev: fix variable length flow elements support

PATCH Superseded Gregory Etelson 2 participants 7 messages Oct 26, 2021 – Nov 4, 2021

Share & tools

Gregory Etelson

author

2021-10-26 09:06 UTC #0 lore raw

PATCH 1/1 ethdev: fix variable length flow elements support

RTE flow API defines two flow items and actions types - common and PMD private. Common RTE flow types are defined in rte_flow.h while PMD private types restricted to specific PMD only. RTE flow API allows PMD private types in flow rule,
but it must not try to interpret private item or acton properties.

Current implementation tried to locate PMD private element, item or action, in common flow elements records.

The patch restricts access to common flow elements records for non-private PMD types only.

Fixes 6cf72047332b ("ethdev: support flow elements with variable length")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Viacheslav Ovsiienko <[email protected]>

---
 lib/ethdev/rte_flow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

lib/ethdev/rte_flow.c +4 -2

index d268784532..a93f68abbc 100644
@@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
 	/**
 	 * Allow PMD private flow item
 	 */
-	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
+	bool rte_type = type >= 0;
+
+	size_t sz = rte_type ? desc[type].size : sizeof(void *);
 	if (buf == NULL || data == NULL)
 		return 0;
 	rte_memcpy(buf, data, (size > sz ? sz : size));
-	if (desc[type].desc_fn)
+	if (rte_type && desc[type].desc_fn)
 		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
 	return sz;
 }

Signature · 1 line

2.33.1

Ferruh Yigit

2021-11-03 21:04 UTC #1 lore raw

Quoted 26 lines · Gregory Etelson

RTE flow API defines two flow items and actions types - common and PMD private. Common RTE flow types are defined in rte_flow.h while PMD private types restricted to specific PMD only. RTE flow API allows PMD private types in flow rule,
but it must not try to interpret private item or acton properties.
Current implementation tried to locate PMD private element, item or action, in common flow elements records.
The patch restricts access to common flow elements records for non-private PMD types only.
Fixes 6cf72047332b ("ethdev: support flow elements with variable length")
Signed-off-by Gregory Etelson <[email protected]>
Acked-by Viacheslav Ovsiienko <[email protected]>
---
  lib/ethdev/rte_flow.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
lib/ethdev/rte_flow.c +0 -0
index d268784532..a93f68abbc 100644
@@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
  	/**

lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size, · line 61

Quoted 10 lines

lib/ethdev/rte_flow.c +3 -2

  	 * Allow PMD private flow item
  	 */
-	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
+	bool rte_type = type >= 0;
+
+	size_t sz = rte_type ? desc[type].size : sizeof(void *);
  	if (buf == NULL || data == NULL)
  		return 0;
  	rte_memcpy(buf, data, (size > sz ? sz : size));
-	if (desc[type].desc_fn)

Was this (possible) negative array index intentional, or are you fixing it?

Quoted 5 lines

lib/ethdev/rte_flow.c +1 -0

+	if (rte_type && desc[type].desc_fn)
  		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
  	return sz;
  }

Gregory Etelson

author

2021-11-04 05:33 UTC #2 lore raw

Hello Ferruh,

Quoted 20 lines

Review
Quoted 4 lines
  lib/ethdev/rte_flow.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/lib/ethdev/rte_flow.c
lib/ethdev/rte_flow.c
Quoted 4 lines
index d268784532..a93f68abbc 100644
lib/ethdev/rte_flow.c +0 -0
@@ -54,11 +54,13 @@
rte_flow_conv_copy(void *buf, const void *data,
const size_t size,
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 4 lines
lib/ethdev/rte_flow.c +0 -1
      /**
       * Allow PMD private flow item
       */
-     size_t sz = type >= 0 ? desc[type].size :
sizeof(void *);
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 3 lines
lib/ethdev/rte_flow.c +3 -0
+     bool rte_type = type >= 0;
+
+     size_t sz = rte_type ? desc[type].size :
sizeof(void *);

Quoted 9 lines

lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 3 lines
lib/ethdev/rte_flow.c +0 -0
      if (buf == NULL || data == NULL)
              return 0;
      rte_memcpy(buf, data, (size > sz ? sz :
size));
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size, · line 61
Quoted 1 line
lib/ethdev/rte_flow.c +0 -1
-     if (desc[type].desc_fn)
Was this (possible) negative array index
intentional, or are you fixing it?

Negative type values assigned to PMD private items and actions. RTE allows private PMD types in flow rules since 5d1bff8fe2 ethdev: allow negative values in flow rule types We construct flow rules with private PMD types
to implement tunnel offload.
However, negative type must not be used as index in rte_flow_desc_item[] and rte_flow_desc_action[] arrays.

Quoted 6 lines

lib/ethdev/rte_flow.c line 64
Quoted 2 lines
lib/ethdev/rte_flow.c +1 -0
+     if (rte_type && desc[type].desc_fn)
              sz += desc[type].desc_fn(size > 0 ? buf
: NULL, data);
Quoted 3 lines
lib/ethdev/rte_flow.c +0 -0
      return sz;
  }

Ferruh Yigit

2021-11-04 09:10 UTC #3 lore raw

Quoted 32 lines · Gregory Etelson

Hello Ferruh,
Quoted 20 lines
Review
Quoted 4 lines
   lib/ethdev/rte_flow.c | 6 ++++--
   1 file changed, 4 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/lib/ethdev/rte_flow.c
lib/ethdev/rte_flow.c
Quoted 4 lines
index d268784532..a93f68abbc 100644
lib/ethdev/rte_flow.c +0 -0
@@ -54,11 +54,13 @@
rte_flow_conv_copy(void *buf, const void *data,
const size_t size,
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 4 lines
lib/ethdev/rte_flow.c +0 -1
       /**
        * Allow PMD private flow item
        */
-     size_t sz = type >= 0 ? desc[type].size :
sizeof(void *);
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 3 lines
lib/ethdev/rte_flow.c +3 -0
+     bool rte_type = type >= 0;
+
+     size_t sz = rte_type ? desc[type].size :
sizeof(void *);
Quoted 9 lines
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size,
Quoted 3 lines
lib/ethdev/rte_flow.c +0 -0
       if (buf == NULL || data == NULL)
               return 0;
       rte_memcpy(buf, data, (size > sz ? sz :
size));
lib/ethdev/rte_flow.c rte_flow_conv_copy(void *buf, const void *data, const size_t size, · line 61
Quoted 1 line
lib/ethdev/rte_flow.c +0 -1
-     if (desc[type].desc_fn)
Was this (possible) negative array index
intentional, or are you fixing it?

Quoted 8 lines

Negative type values assigned to PMD private items and actions. RTE allows private PMD types in flow rules since 5d1bff8fe2 ethdev: allow negative values in flow rule types We construct flow rules with private PMD types
to implement tunnel offload.
However, negative type must not be used as index in rte_flow_desc_item[] and rte_flow_desc_action[] arrays.

That is what I assumed, but wasn't clear from commit log, commit log reads as it is updating an intended usage.

Quoted 7 lines

Quoted 6 lines
lib/ethdev/rte_flow.c line 64
Quoted 2 lines
lib/ethdev/rte_flow.c +1 -0
+     if (rte_type && desc[type].desc_fn)
               sz += desc[type].desc_fn(size > 0 ? buf
: NULL, data);
Quoted 3 lines
lib/ethdev/rte_flow.c +0 -0
       return sz;
   }

Gregory Etelson

author

2021-11-04 09:20 UTC #4 lore raw

Hello Ferruh,

..snip..

Quoted 11 lines

Quoted 2 lines
Negative type values assigned to PMD private
items and actions.
Quoted 1 line
RTE allows private PMD types in flow rules
since
Quoted 1 line
5d1bff8fe2 ethdev: allow negative values in
flow rule types
Quoted 1 line
We construct flow rules with private PMD
types
Quoted 2 lines
to implement tunnel offload.
However, negative type must not be used as

Quoted 9 lines

index in
Quoted 1 line
rte_flow_desc_item[] and
rte_flow_desc_action[] arrays.
Quoted 1 line
That is what I assumed, but wasn't clear from
commit log, commit log
reads as it is updating an intended usage.

I'll update the commit message in v2.

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ if (rte_type && desc[type].desc_fn)
               sz += desc[type].desc_fn(size > 0 ?
buf
Quoted 5 lines
Quoted 4 lines
: NULL, data);
Quoted 3 lines
       return sz;
}

Regards,
Gregory

Gregory Etelson

author

2021-11-04 11:27 UTC #5 lore raw

PATCH v2 1/1 ethdev: fix variable length flow elements support

In reply to Gregory Etelson

RTE flow API defines two flow elements types - common and PMD private. Common RTE flow types are defined in rte_flow.h while PMD private types exists inside specific PMD only. Application can create a flow rule with PMD private items or actions. RTE flow API restricts private PMD types to negative values.

Current implementation tried to use negative PMD private item type value as index in the rte_flow_desc_item[] array.

The patch allows access to rte_flow_desc_item[] and rte_flow_desc_action[] arrays to non-private PMD types only.

Fixes 6cf72047332b ("ethdev: support flow elements with variable length")

Signed-off-by Gregory Etelson <[email protected]>

---
v2: Update the patch comment.

---
 lib/ethdev/rte_flow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

lib/ethdev/rte_flow.c +4 -2

index d268784532..a93f68abbc 100644
@@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
 	/**
 	 * Allow PMD private flow item
 	 */
-	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
+	bool rte_type = type >= 0;
+
+	size_t sz = rte_type ? desc[type].size : sizeof(void *);
 	if (buf == NULL || data == NULL)
 		return 0;
 	rte_memcpy(buf, data, (size > sz ? sz : size));
-	if (desc[type].desc_fn)
+	if (rte_type && desc[type].desc_fn)
 		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
 	return sz;
 }

Signature · 1 line

2.33.1

Ferruh Yigit

Reviewed-by on v2 1/1 Applied

2021-11-04 12:46 UTC #6 lore raw

Quoted 6 lines · Gregory Etelson

RTE flow API defines two flow elements types - common and PMD private. Common RTE flow types are defined in rte_flow.h while PMD private types exists inside specific PMD only. Application can create a flow rule with PMD private items or actions. RTE flow API restricts private PMD types to negative values.

Quoted 8 lines

Current implementation tried to use negative PMD private item type value as index in the rte_flow_desc_item[] array.
The patch allows access to rte_flow_desc_item[] and rte_flow_desc_action[] arrays to non-private PMD types only.
Fixes 6cf72047332b ("ethdev: support flow elements with variable length")
Signed-off-by Gregory Etelson <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

Applied to dpdk-next-net/main, thanks.