Re: [PATCH] app/testpmd: fix indirect action list parameters parsing

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/9/2023 6:22 PM, Etelson, Gregory wrote:
> Hello Ferruh,
> 
>>> 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 can't see how stack corruption can happen, can you please provide call
stack and flow command?


> I'll change the id definition to uintptr_t to match token in v2.
> 
> Regards,
> Gregory
> 
>>
>> 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?


>>
>>
>>> The patch updated variable size for translation results.
>>>
>>> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
>>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>>> ---
>>>  app/test-pmd/cmdline_flow.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index 0d521159e9..cf1ca33208 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -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;
>>
>>