Re: [PATCH] app/testpmd: add size parameter to raw_encap action

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

On 2/9/2024 1:43 PM, Dariusz Sosnowski wrote:
>> -----Original Message-----
>> From: Dariusz Sosnowski
>> Sent: Friday, February 9, 2024 12:04
>> To: Gregory Etelson <getelson@nvidia.com>; dev@dpdk.org
>> Cc: Gregory Etelson <getelson@nvidia.com>; Maayan Kashani
>> <mkashani@nvidia.com>; Ori Kam <orika@nvidia.com>; Aman Singh
>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>> Subject: RE: [PATCH] app/testpmd: add size parameter to raw_encap action
>>
>> Hi Gregory,
>>
>>> -----Original Message-----
>>> From: Gregory Etelson <getelson@nvidia.com>
>>> Sent: Thursday, October 26, 2023 09:31
>>> To: dev@dpdk.org
>>> Cc: Gregory Etelson <getelson@nvidia.com>; Maayan Kashani
>>> <mkashani@nvidia.com>; Ori Kam <orika@nvidia.com>; Aman Singh
>>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>>> Subject: [PATCH] app/testpmd: add size parameter to raw_encap action
>>>
>>> Testpmd always provides RAW_ENCAP flow action configuration with encap
>>> buffer and the buffer size.
>>> That implementation does not allow to create non-masked raw_encap
>>> action in the template API actions template.
>>>
>>> The patch adds the `size` parameter to testpmd `raw_encap` action
>>> configuration.
>>> Testpmd can create non-masked raw-encap action template and specify
>>> encap buffer during flow creation.
>>>
>>> Example:
>>>
>>> # total data size is 50
>>> testpmd> set raw_encap 0 \
>>>          eth src is 11:22:33:44:55:66 dst is aa:bb:cc:dd:01:aa / \
>>>          ipv4 src is 31.31.31.31 dst is 63.63.63.1 / udp src is 1 / \
>>>          vxlan vni is 1 / end_set
>>>
>>> testpmd> flow actions_template 0 create ingress \
>>>          actions_template_id 50 \
>>>          template raw_encap size 50 / jump / end \
>>>          mask raw_encap size 50 / jump / end \
>>>
>>> tstpmd> flow queue 0 create 0 template_table 0 \
>>>         pattern_template 0 actions_template 0 postpone no \
>>>         pattern ... end \
>>>         actions raw_encap index 0 / jump group 1 / end
>>>
>>> The new `size` parameter is mutually exclusive with the existing
>>> `index` parameter.
>>>
>>> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
>>
>> The following sequence of commands results in "Bad arguments" error, but I
>> think it should be accepted.
>>
>> testpmd> port stop all
>> Stopping ports...
>> Checking link statuses...
>> Done
>> testpmd> flow configure 0 queues_number 4 queues_size 64
>> Configure flows on port 0: number of queues 4 with 64 elements
>> testpmd> port start all
>> Port 0: B8:CE:F6:7B:D8:E0
>> Checking link statuses...
>> Done
>> testpmd> set raw_encap 0 eth src is 11:22:33:44:55:66 dst is
>> testpmd> aa:bb:cc:dd:01:aa / ipv4 src is 31.31.31.31 dst is 63.63.63.1 /
>> testpmd> udp src is 1 / vxlan vni is 1 / end_set flow actions_template 0
>> testpmd> create ingress actions_template_id 100 template raw_encap index
>> testpmd> 0 / jump / end mask raw_encap index 0 / jump / end
>> Bad arguments
> 
> I bisected the tree, and it appears that this issue is not caused by this commit, but it's an existing problem in testpmd.
> The root cause of "Bad arguments" is the fact that when parsing function for raw_encap index is called,
> parse_int() is called with size == 0, but raw_encap index size is sizeof(size_t).
> This causes a failure in validation introduced in commit 913b919906da ("app/testpmd: add size validation to token parsers").
> 
> The same issue appears for other cases where parse_int() is called with size == 0 e.g., parsing RSS queues in RSS flow action.
> I'll provide a patch for testpmd which addresses that.
> 

Hi Dariusz,

I merged that patch recently, and it is still not merged to main repo,
so we can drop it from next-net if required, instead of having a fix on
top of it.

That patch is because of a defect that overwrites some memory, and side
impact you mentioned was not expected.
Can you please sync with Gregory about it first?