app/testpmd: fix indirect action list parameters parsing
Checks
Commit Message
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 <getelson@nvidia.com>
---
app/test-pmd/cmdline_flow.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
On 11/8/2023 4:34 PM, Gregory Etelson wrote:
> 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.
> 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;
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'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.
>
>
>
>> 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;
>
>
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;
>>
>>
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?
>
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, ...}
>>> 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?
>
I've missed that.
Need to re-think.
Regards,
Gregory
>
>>>
>>>
>>>> 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;
>>>
>>>
>
>
On 11/9/2023 7:57 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?
>>
>
> 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, ...}
>
Thanks Gregory, I can see the problem now.
if 'handle' is not an address, but an ID, your V3 makes sense to convert
its type to 'uint32_t', and it solves to problem. Same for 'conf'.
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]
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 0d521159e97d..cd123c69265e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -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:
>
>>>> 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?
>>
>
> I've missed that.
> Need to re-think.
>
> Regards,
> Gregory
>
>>
>>>>
>>>>
>>>>> 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;
>>>>
>>>>
>>
>>
Hello Ferruh,
[:snip:]
>> *** 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, ...}
>>
>
> Thanks Gregory, I can see the problem now.
>
[: Thumbs up :]
[:snip:]
> 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]
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0d521159e97d..cd123c69265e 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -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:
>
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:
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index ce71818705..7fb3b61d37 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
+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
On 11/10/2023 5:41 PM, Etelson, Gregory wrote:
> Hello Ferruh,
>
> [:snip:]
>
>>> *** 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, ...}
>>>
>>
>> Thanks Gregory, I can see the problem now.
>>
>
> [: Thumbs up :]
>
> [:snip:]
>
>> 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]
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 0d521159e97d..cd123c69265e 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -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:
>>
>
> 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?
> 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:
>
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.
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index ce71818705..7fb3b61d37 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
>
> +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
Hello Ferruh,
>> 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?
>
>
parse_int() receives target buffer size as an argument and also has access
to original token size through the arg pointer.
In that configuration the token size has priority.
parse_int() must compare token and buffer sizes and return an error if
target buffer size did not fit.
That will not break existing parser_int() functionality.
I'll send a fix in a new patch series.
>> 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:
>>
>
> 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.
>
The function parses indirect ID. That ID is translated into the original
object of any size according to token type. That means testpmd will have
common pool of 32 bits IDs for all possible indirect translations.
>
> 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.
>
>
[: Thumbs up :]
Regards,
Gregory
@@ -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;