Re: [RFC 0/2] ethdev: update GENEVE option item structure

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

On 4/17/2024 8:23 AM, Michael Baum wrote:
> The "rte_flow_item_geneve_opt" structure describes the GENEVE TLV option
> header according to RFC 8926 [1]:
> 
> struct rte_flow_item_geneve_opt {
> 	rte_be16_t option_class;
> 	uint8_t option_type;
> 	uint8_t option_len;
> 	uint32_t *data;
> };
> 
> The "option_len" field is used for two different purposes:
>  1. item field for matching with value/mask.
>  2. descriptor for data array size.
> 

For the long run solution, we may consider adding geneve option header
to net/rte_geneve.h and make "struct rte_flow_item_geneve_opt" + data size ?

> Those two different purposes might limit each other. For example, when
> matching on length with full mask (0x1f), the data array in the mask
> structure might be taken as size 31 and read invalid memory.
> 
> This problem appears in conversion API. In current implementation, the
> "rte_flow_conv" API copies the "rte_flow_item_geneve_opt" structure
> without taking care about data deep-copy. The attempt to solve this
> revealed the problem in determining the size of the mask data array. To
> resolve this issue, two solutions are suggested.
> 

Are we having this problem only with geneve options because data size is
not fixed / defined for the header?

> Immediate Workaround:
> The data array size in the "mask" structure is determined by
> "option_len" field in the "spec" structure. This workaround can be
> integrated soon to avoid deep-copy missing.
> 

This requires a geneve specific pointer in the item spec, which is not
really nice, although it is temporary solution. Perhaps we can skip
this, can you please check below comment.

> Long Run Solution:
> Add a new field into "rte_flow_item_geneve_opt" structure regardless to
> "option_len" field. This solution should wait to "24.11" version since
> it contains API change.
>

I was expecting the same, but CI seems passed ABI test case [1], it may
be because new field appended end of the struct.
Can you please double check, if ABI is not broken, we can go with this
solution directly?

[1]
https://mails.dpdk.org/archives/test-report/2024-April/643570.html

> When the API is changed, I'll take the opportunity to add documentation
> for this item in "rte_flow.rst" file and update the data type to
> "rte_be32_t".
> 

If we can go with updating struct in this release, adding protocol
option struct in net library can wait v24.11 release.
So "rte_be32_t" type change in this struct won't be a thing.

> [1] https://datatracker.ietf.org/doc/html/rfc8926
> 
> Michael Baum (2):
>   ethdev: fix GENEVE option item conversion
>   ethdev: add data size field to GENEVE option item
> 
> 

@Ori, Can you please help reviewing this patch?
At worst, it can be good to address the fix in this release.