app/testpmd: fix indirect action list parameters parsing

Indirect actions list arguments parser was configured to place target number into 64bit value, while the code provided 32bits memory.

The patch updated variable size for translation results.

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

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

---
 app/test-pmd/cmdline_flow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

app/test-pmd/cmdline_flow.c +3 -2

index 0d521159e9..cf1ca33208 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	struct rte_flow_action *action = ctx->object;
 	struct rte_flow_action_indirect_list *action_conf;
 	const struct indlst_conf *indlst_conf;
-	uint32_t id;
+	uint64_t id;
 	int ret;
 
 	if (!action)

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	action_conf->handle = (typeof(action_conf->handle))
 				port_action_handle_get_by_id(ctx->port, id);
 		if (!action_conf->handle) {
-			printf("no indirect list handle for id %u\n", id);
+			printf("no indirect list handle for id %"PRIu64"\n",
+			       id);
 			return -1;
 		}
 		break;

Quoted 3 lines

Indirect actions list arguments parser was configured to place target number into 64bit value, while the code provided 32bits memory.

Hi Gregory,

Can you please give more details why 'id' needs to be 64 bits, with callstack or usecase etc?
And please describe what is the observed problem with current code?

Inside 'parse_indlst_id2ptr()',
'parse_int()' can work or 32bits and 64bits variables, so that one is OK. But both 'port_action_handle_get_by_id()' & 'indirect_action_list_conf_get()' gets 'id' as parameter and they get 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and loose data, should those functions needs to be updated as well.

Quoted 31 lines

The patch updated variable size for translation results.

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

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

---
 app/test-pmd/cmdline_flow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

app/test-pmd/cmdline_flow.c +3 -2

index 0d521159e9..cf1ca33208 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
      struct rte_flow_action *action = ctx->object;
      struct rte_flow_action_indirect_list *action_conf;
      const struct indlst_conf *indlst_conf;
-     uint32_t id;
+     uint64_t id;
      int ret;
 
      if (!action)

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
      action_conf->handle = (typeof(action_conf->handle))
                              port_action_handle_get_by_id(ctx->port, id);
              if (!action_conf->handle) {
-                     printf("no indirect list handle for id %u\n", id);
+                     printf("no indirect list handle for id %"PRIu64"\n",
+                            id);
                      return -1;
              }
              break;

Hello Ferruh,

Quoted 10 lines

Quoted 3 lines

Indirect actions list arguments parser was configured to place target number into 64bit value, while the code provided 32bits memory.

Hi Gregory,

Can you please give more details why 'id' needs to be 64 bits, with callstack or usecase etc?
And please describe what is the observed problem with current code?

In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.

Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens define arguments size as uintptr_t.

On 64 bits system, defining the id variable as 32 bits value corrupted parse_indlst_id2ptr stack.

I'll change the id definition to uintptr_t to match token in v2.

Regards,
Gregory

Quoted 8 lines

Inside 'parse_indlst_id2ptr()',
'parse_int()' can work or 32bits and 64bits variables, so that one is OK. But both 'port_action_handle_get_by_id()' & 'indirect_action_list_conf_get()' gets 'id' as parameter and they get 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and loose data, should those functions needs to be updated as well.

Quoted 39 lines

Quoted 37 lines

The patch updated variable size for translation results.

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

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

---
 app/test-pmd/cmdline_flow.c | 5 +++--

 1 file changed, 3 insertions(+), 2 deletions(-)

app/test-pmd/cmdline_flow.c +1 -1

index 0d521159e9..cf1ca33208 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
       struct rte_flow_action *action = ctx->object;
       struct rte_flow_action_indirect_list *action_conf;
       const struct indlst_conf *indlst_conf;
-     uint32_t id;
+     uint64_t id;
       int ret;
 

      if (!action)

app/test-pmd/cmdline_flow.c +2 -1

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
       action_conf->handle = (typeof(action_conf->handle))
                               port_action_handle_get_by_id(ctx->port,
 id);
               if (!action_conf->handle) {
-                     printf("no indirect list handle for id %u\n", id);
+                     printf("no indirect list handle for id
 %"PRIu64"\n",
+                            id);

                      return -1;
              }
              break;

Quoted 16 lines

Quoted 10 lines

Quoted 3 lines

Indirect actions list arguments parser was configured to place target number into 64bit value, while the code provided 32bits memory.

Hi Gregory,

Can you please give more details why 'id' needs to be 64 bits, with callstack or usecase etc?
And please describe what is the observed problem with current code?

In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.

Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens define arguments size as uintptr_t.

Quoted 3 lines

On 64 bits system, defining the id variable as 32 bits value corrupted parse_indlst_id2ptr stack.

I can't see how stack corruption can happen, can you please provide call stack and flow command?

Quoted 7 lines

Quoted 7 lines

Inside 'parse_indlst_id2ptr()',
'parse_int()' can work or 32bits and 64bits variables, so that one is OK. But both 'port_action_handle_get_by_id()' & 'indirect_action_list_conf_get()' gets 'id' as parameter and they get 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and loose data, should those functions needs to be updated as well.

Can you please reply to above question, about changing 'id' type impact to other functions using it?

Quoted 40 lines

Quoted 40 lines

Quoted 38 lines

The patch updated variable size for translation results.

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

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

---
 app/test-pmd/cmdline_flow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

app/test-pmd/cmdline_flow.c +1 -1

index 0d521159e9..cf1ca33208 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
      struct rte_flow_action *action = ctx->object;
      struct rte_flow_action_indirect_list *action_conf;
      const struct indlst_conf *indlst_conf;
-     uint32_t id;
+     uint64_t id;
      int ret;
 

if (!action)

app/test-pmd/cmdline_flow.c +2 -1

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
      action_conf->handle = (typeof(action_conf->handle))
                              port_action_handle_get_by_id(ctx->port,
 id);
              if (!action_conf->handle) {
-                     printf("no indirect list handle for id %u\n", id);
+                     printf("no indirect list handle for id
 %"PRIu64"\n",
+                            id);

                      return -1;
              }
              break;

Hello Ferruh,

Quoted 17 lines

Quoted 17 lines

Quoted 10 lines

Quoted 3 lines

Indirect actions list arguments parser was configured to place target number into 64bit value, while the code provided 32bits memory.

Hi Gregory,

Can you please give more details why 'id' needs to be 64 bits, with callstack or usecase etc?
And please describe what is the observed problem with current code?

In rte_flow.h, struct rte_flow_action_indirect_list::handle is a pointer.

Testpmd ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens define arguments size as uintptr_t.

Quoted 7 lines

Quoted 3 lines

On 64 bits system, defining the id variable as 32 bits value corrupted parse_indlst_id2ptr stack.

I can't see how stack corruption can happen, can you please provide call stack and flow command?

To reproduce the crash buildtype must be release or debugoptimized. The crash will not reproduce with the debug builds.

Testpmd commands I use:

dpdk-testpmd -a ${PCI_ADDR},dv_flow_en=2,representor=vf0-1 -- -i

port stop all
flow configure 0 queues_number 12 queues_size 256
flow configure 1 queues_number 12 queues_size 256
flow configure 2 queues_number 12 queues_size 256
port start all
start

set raw_encap 0 eth dst is 00:16:3e:52:bd:37 src is 00:16:3e:6e:16:e0 type is 2048 has_vlan is 0 / ipv4 src is 110.240.52.255 dst is 189.68.183.147 proto is 17 fragment_offset is 0 packet_id is 1 tos is 102 ttl is 189 version_ihl is 69 / udp src is 56800 dst is 4789 / vxlan vni is 3 / end_set
set sample_actions 0 represented_port ethdev_port_id 0 / end

flow indirect_action 0 create action_id 5 transfer list actions sample ratio 1 index 0 / represented_port ethdev_port_id 2 / end flow actions_template 0 create transfer actions_template_id 6 template indirect_list handle 5 / end mask  indirect_list handle 5 / end

Result:
*** stack smashing detected ***: terminated

The corruption occurred in parse_int() called from parse_indlst_id2ptr().

Inside parse_int() the arg parameter referenced 8 bytes of memory while the target buffer was 4 bytes allocated on caller optimized stack:

(gdb) p *arg
$1 = { ... size = 8, ...}

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

Inside 'parse_indlst_id2ptr()',
'parse_int()' can work or 32bits and 64bits variables, so that one is OK.
But both 'port_action_handle_get_by_id()' & 'indirect_action_list_conf_get()' gets 'id' as parameter and they get 32bits argument, when 'id' is 64bit won't it will be cast to 32bits and loose data, should those functions needs to be updated as well.

Quoted 3 lines

Can you please reply to above question, about changing 'id' type impact to other functions using it?

I've missed that.
Need to re-think.

Regards,
Gregory

Quoted 43 lines

Quoted 41 lines

Quoted 41 lines

Quoted 39 lines

The patch updated variable size for translation results.

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

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

---
 app/test-pmd/cmdline_flow.c | 5 +++--

 1 file changed, 3 insertions(+), 2 deletions(-)

app/test-pmd/cmdline_flow.c +1 -1

index 0d521159e9..cf1ca33208 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
       struct rte_flow_action *action = ctx->object;
       struct rte_flow_action_indirect_list *action_conf;
       const struct indlst_conf *indlst_conf;
-     uint32_t id;
+     uint64_t id;
       int ret;
 

      if (!action)

app/test-pmd/cmdline_flow.c +1 -1

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx,
 const struct token *token,
       action_conf->handle = (typeof(action_conf->handle))
                               port_action_handle_get_by_id(ctx->port,
 id);
               if (!action_conf->handle) {
-                     printf("no indirect list handle for id %u\n",
 id);
+                     printf("no indirect list handle for id

%"PRIu64"\n",
+                            id);
                      return -1;
              }
              break;

Quoted 5 lines

*** stack smashing detected ***: terminated

The corruption occurred in parse_int() called from parse_indlst_id2ptr().

Quoted 6 lines

Inside parse_int() the arg parameter referenced 8 bytes of memory while the target buffer was 4 bytes allocated on caller optimized stack:

(gdb) p *arg
$1 = { ... size = 8, ...}

Thanks Gregory, I can see the problem now.

BUT back to the root cause of the problem, parse_int() tries to be generic and it support different size of variables [1], but it fails on this.

parse_int() gets 'size' as argument, but it doesn't use parameter value, instead overwrites it with 'size = arg->size;' and uses this value, in this case when context provides larger variable size than what parse_int() gets as parameter, the problem you observed occurs.

What do you think to use 'size' from parameter list, as it is intended, instead of using 'arg->size'?
Or perhaps use 'buf' and 'size' from parameter if they are valid, else get the from context/arg [2]?
I think this solves your problem, can you please verify it?

btw, 'buf' usage is a little more complex, since parse_int() checks for "ctx->object != NULL" to continue, I can't really be sure about intention there, but please check usage in parse_port(), it looks like when 'buf' provided expectation is to get parsed value in the 'buf'.

[1]
switch (size) {
case sizeof(uint8_t):

   ...
 case sizeof(uint16_t):
   ...
 ...
 case sizeof(uint64_t):
   ...

[2]

app/test-pmd/cmdline_flow.c +4 -2

index 0d521159e97d..cd123c69265e 100644
@@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
 token *token,
                        goto error;
                return len;
        }
-       buf = (uint8_t *)ctx->object + arg->offset;
-       size = arg->size;
+       if (buf == NULL || size == 0) {
+               buf = (uint8_t *)ctx->object + arg->offset;
+               size = arg->size;
+       }
        if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
                return -1;

objmask:

Hello Ferruh,

[:snip:]

Quoted 5 lines

Quoted 5 lines

*** stack smashing detected ***: terminated

The corruption occurred in parse_int() called from parse_indlst_id2ptr().

Quoted 9 lines

Quoted 6 lines

Inside parse_int() the arg parameter referenced 8 bytes of memory while the target buffer was 4 bytes allocated on caller optimized stack:

(gdb) p *arg
$1 = { ... size = 8, ...}

Thanks Gregory, I can see the problem now.

[: Thumbs up :]

[:snip:]

Quoted 52 lines

BUT back to the root cause of the problem, parse_int() tries to be generic and it support different size of variables [1], but it fails on this.

parse_int() gets 'size' as argument, but it doesn't use parameter value, instead overwrites it with 'size = arg->size;' and uses this value, in this case when context provides larger variable size than what parse_int() gets as parameter, the problem you observed occurs.

What do you think to use 'size' from parameter list, as it is intended, instead of using 'arg->size'?
Or perhaps use 'buf' and 'size' from parameter if they are valid, else get the from context/arg [2]?
I think this solves your problem, can you please verify it?

btw, 'buf' usage is a little more complex, since parse_int() checks for "ctx->object != NULL" to continue, I can't really be sure about intention there, but please check usage in parse_port(), it looks like when 'buf' provided expectation is to get parsed value in the 'buf'.

[1]
 switch (size) {
 case sizeof(uint8_t):
   ...
 case sizeof(uint16_t):
   ...
 ...
 case sizeof(uint64_t):
   ...

[2]

app/test-pmd/cmdline_flow.c +4 -2

index 0d521159e97d..cd123c69265e 100644
@@ -10805,8 +10805,10 @@ parse_int(struct context *ctx, const struct
 token *token,
                         goto error;
                 return len;
         }
-       buf = (uint8_t *)ctx->object + arg->offset;
-       size = arg->size;
+       if (buf == NULL || size == 0) {
+               buf = (uint8_t *)ctx->object + arg->offset;
+               size = arg->size;
+       }
         if (u > RTE_LEN2MASK(size * CHAR_BIT, uint64_t))
                 return -1;

Quoted 2 lines

 objmask:

About the fault root cause.
There were 2 uncoupled resources in that case: static token size and variable size passed to parse_int().
parse_int() caller must provide a buffer large enough for token size. Otherwise parse_int() will corrupt memory outside the input buffer.

In the generic solution parse_int() caller allocates target buffer using existing knowledge about input token size.

Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates target buffer size from RTE structure member.

Current testpmd cannot use that approach directly because indirect action references internal testpmd ID.

Testpmd indirect ID has no defined type or token that leads to indirect ID parser.

As a solution, testpmd can provide centralized parser function for all indirect IDs. The function will parse ID value and use the token as the key to indirect database search:

app/test-pmd/cmdline_flow.c +29 -0

index ce71818705..7fb3b61d37 100644
 
+typedef uint32_t indirect_id_t;
+
+static int
+parse_indirect_index(struct context *ctx, const struct token *token,
+               const char *str, unsigned int len,
+               void *buf, unsigned int size)
+{
+       indirect_id_t id;
+
+       ctx->object = &id;
+       parse_int(ctx, token, str, len, ctx->object, sizeof(id));
+
+       switch (ctx->curr) {
+       case INDIRECT_ACTION_ID2PTR:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
+               /**/
+               break;
+       case INDIRECT_LIST_ACTION_ID2PTR_CONF:
+               /**/
+               break;
+       default:
+               /**/
+       }
+
+
+}
+

Regards,
Gregory

Quoted 6 lines

About the fault root cause.
There were 2 uncoupled resources in that case: static token size and variable size passed to parse_int().
parse_int() caller must provide a buffer large enough for token size. Otherwise parse_int() will corrupt memory outside the input buffer.

As you said 'parse_int()' has two sizes, 'token->size' and 'size' function argument. Why function ignores 'size' argument and only uses 'token->size', I think this is a mistake.

If 'parse_int()' doesn't use 'buf' and 'size' arguments at all, why it has them?

Quoted 9 lines

In the generic solution parse_int() caller allocates target buffer using existing knowledge about input token size.

Testpmd add_port() imitates the ARGS_ENTRY() macro that extrapolates target buffer size from RTE structure member.

Current testpmd cannot use that approach directly because indirect action references internal testpmd ID.

Quoted 7 lines

Testpmd indirect ID has no defined type or token that leads to indirect ID parser.

As a solution, testpmd can provide centralized parser function for all indirect IDs. The function will parse ID value and use the token as the key to indirect database search:

Although it sounds reasonable to have indirect id parser, won't it have exact same problem?

If token size if 64 bits as it is now, as far as I can see below code will have same stack corruption problem.

I think we should update parse_int function, to use either function parameters or context values, but changes has potential side effect and timing is not good for it, let's continue with your v3 for now.

app/test-pmd/cmdline_flow.c +1 -1

index 0d521159e9..397f9bc3eb 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	struct rte_flow_action *action = ctx->object;
 	struct rte_flow_action_indirect_list *action_conf;
 	const struct indlst_conf *indlst_conf;
-	uint32_t id;
+	uintptr_t id;
 	int ret;
 
 	if (!action)

Quoted 12 lines · On Thu, 9 Nov 2023 20:36:37 +0200 Gregory Etelson <[email protected]> wrote:

app/test-pmd/cmdline_flow.c +1 -1

index 0d521159e9..397f9bc3eb 100644
@@ -11331,7 +11331,7 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	struct rte_flow_action *action = ctx->object;
 	struct rte_flow_action_indirect_list *action_conf;
 	const struct indlst_conf *indlst_conf;
-	uint32_t id;
+	uintptr_t id;
 	int ret;
 

app/test-pmd/cmdline_flow.c parse_indlst_id2ptr(struct context *ctx, const struct token *token, · line 11354

Quoted 8 lines

app/test-pmd/cmdline_flow.c +2 -1

 	if (!action)

@@ -11350,7 +11350,8 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
 	action_conf->handle = (typeof(action_conf->handle))
 				port_action_handle_get_by_id(ctx->port, id);
 		if (!action_conf->handle) {
-			printf("no indirect list handle for id %u\n", id);
+			printf("no indirect list handle for id %"PRIu64"\n",
+			       id);

On 32 bit platforms uintptr_t is 32 bits.
Uintptr_t is always a typedef for unsigned long so use %lu here instead.

Testpmd maps indirect action lists handlers into application IDs. Testpmd API defines indirect IDs as 32 bits values.

The patch fixes indirect IDs size in ACTION_INDIRECT_LIST_HANDLE and ACTION_INDIRECT_LIST_CONF tokens

Fixes 72a3dec7126f ("ethdev: add indirect flow list action")

Cc [email protected]

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