[dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[Gregory Etelson]

RTE flow API defines two flow items and actions types - common
and PMD private. Common RTE flow types are defined in rte_flow.h
while PMD private types restricted to specific PMD only.
RTE flow API allows PMD private types in flow rule,
but it must not try to interpret private item or acton properties.

Current implementation tried to locate PMD private element, item or
action, in common flow elements records.

The patch restricts access to common flow elements records for
non-private PMD types only.

Fixes: 6cf72047332b ("ethdev: support flow elements with variable length")

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 lib/ethdev/rte_flow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index d268784532..a93f68abbc 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
 	/**
 	 * Allow PMD private flow item
 	 */
-	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
+	bool rte_type = type >= 0;
+
+	size_t sz = rte_type ? desc[type].size : sizeof(void *);
 	if (buf == NULL || data == NULL)
 		return 0;
 	rte_memcpy(buf, data, (size > sz ? sz : size));
-	if (desc[type].desc_fn)
+	if (rte_type && desc[type].desc_fn)
 		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
 	return sz;
 }
-- 
2.33.1

Re: [dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[Ferruh Yigit]

On 10/26/2021 10:05 AM, Gregory Etelson wrote:
> RTE flow API defines two flow items and actions types - common
> and PMD private. Common RTE flow types are defined in rte_flow.h
> while PMD private types restricted to specific PMD only.
> RTE flow API allows PMD private types in flow rule,
> but it must not try to interpret private item or acton properties.
> 
> Current implementation tried to locate PMD private element, item or
> action, in common flow elements records.
> 
> The patch restricts access to common flow elements records for
> non-private PMD types only.
> 
> Fixes: 6cf72047332b ("ethdev: support flow elements with variable length")
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>   lib/ethdev/rte_flow.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index d268784532..a93f68abbc 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
>   	/**
>   	 * Allow PMD private flow item
>   	 */
> -	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
> +	bool rte_type = type >= 0;
> +
> +	size_t sz = rte_type ? desc[type].size : sizeof(void *);
>   	if (buf == NULL || data == NULL)
>   		return 0;
>   	rte_memcpy(buf, data, (size > sz ? sz : size));
> -	if (desc[type].desc_fn)

Was this (possible) negative array index intentional, or are you fixing it?

> +	if (rte_type && desc[type].desc_fn)
>   		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
>   	return sz;
>   }
> 

Re: [dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[Gregory Etelson]

Hello Ferruh,

> >   lib/ethdev/rte_flow.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_flow.c
> b/lib/ethdev/rte_flow.c
> > index d268784532..a93f68abbc 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -54,11 +54,13 @@
> rte_flow_conv_copy(void *buf, const void *data,
> const size_t size,
> >       /**
> >        * Allow PMD private flow item
> >        */
> > -     size_t sz = type >= 0 ? desc[type].size :
> sizeof(void *);
> > +     bool rte_type = type >= 0;
> > +
> > +     size_t sz = rte_type ? desc[type].size :
> sizeof(void *);
> >       if (buf == NULL || data == NULL)
> >               return 0;
> >       rte_memcpy(buf, data, (size > sz ? sz :
> size));
> > -     if (desc[type].desc_fn)
> 
> Was this (possible) negative array index
> intentional, or are you fixing it?
> 

Negative type values assigned to PMD private items and actions. 
RTE allows private PMD types in flow rules since 
5d1bff8fe2 ethdev: allow negative values in flow rule types
We construct flow rules with private PMD types
to implement tunnel offload.
However, negative type must not be used as index in
rte_flow_desc_item[] and rte_flow_desc_action[] arrays.

> > +     if (rte_type && desc[type].desc_fn)
> >               sz += desc[type].desc_fn(size > 0 ? buf
> : NULL, data);
> >       return sz;
> >   }
> >

Re: [dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[Ferruh Yigit]

On 11/4/2021 5:33 AM, Gregory Etelson wrote:
> Hello Ferruh,
> 
>>>    lib/ethdev/rte_flow.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_flow.c
>> b/lib/ethdev/rte_flow.c
>>> index d268784532..a93f68abbc 100644
>>> --- a/lib/ethdev/rte_flow.c
>>> +++ b/lib/ethdev/rte_flow.c
>>> @@ -54,11 +54,13 @@
>> rte_flow_conv_copy(void *buf, const void *data,
>> const size_t size,
>>>        /**
>>>         * Allow PMD private flow item
>>>         */
>>> -     size_t sz = type >= 0 ? desc[type].size :
>> sizeof(void *);
>>> +     bool rte_type = type >= 0;
>>> +
>>> +     size_t sz = rte_type ? desc[type].size :
>> sizeof(void *);
>>>        if (buf == NULL || data == NULL)
>>>                return 0;
>>>        rte_memcpy(buf, data, (size > sz ? sz :
>> size));
>>> -     if (desc[type].desc_fn)
>>
>> Was this (possible) negative array index
>> intentional, or are you fixing it?
>>
> 
> Negative type values assigned to PMD private items and actions.
> RTE allows private PMD types in flow rules since
> 5d1bff8fe2 ethdev: allow negative values in flow rule types
> We construct flow rules with private PMD types
> to implement tunnel offload.
> However, negative type must not be used as index in
> rte_flow_desc_item[] and rte_flow_desc_action[] arrays.
> 

That is what I assumed, but wasn't clear from commit log, commit log
reads as it is updating an intended usage.

>>> +     if (rte_type && desc[type].desc_fn)
>>>                sz += desc[type].desc_fn(size > 0 ? buf
>> : NULL, data);
>>>        return sz;
>>>    }
>>>
> 

Re: [dpdk-dev] [PATCH] ethdev: fix variable length flow elements support

[Gregory Etelson]

Hello Ferruh,

..snip..
> >
> > Negative type values assigned to PMD private
> items and actions.
> > RTE allows private PMD types in flow rules
> since
> > 5d1bff8fe2 ethdev: allow negative values in
> flow rule types
> > We construct flow rules with private PMD
> types
> > to implement tunnel offload.
> > However, negative type must not be used as
> index in
> > rte_flow_desc_item[] and
> rte_flow_desc_action[] arrays.
> >
> 
> That is what I assumed, but wasn't clear from
> commit log, commit log
> reads as it is updating an intended usage.
> 

I'll update the commit message in v2.

> >>> +     if (rte_type && desc[type].desc_fn)
> >>>                sz += desc[type].desc_fn(size > 0 ?
> buf
> >> : NULL, data);
> >>>        return sz;
> >>>    }
> >>>
> >

Regards,
Gregory

[dpdk-dev] [PATCH v2] ethdev: fix variable length flow elements support

[Gregory Etelson]

RTE flow API defines two flow elements types - common and PMD private.
Common RTE flow types are defined in rte_flow.h while PMD private
types exists inside specific PMD only. Application can create a flow
rule with PMD private items or actions. RTE flow API restricts
private PMD types to negative values.

Current implementation tried to use negative PMD private item type
value as index in the rte_flow_desc_item[] array.

The patch allows access to rte_flow_desc_item[] and
rte_flow_desc_action[] arrays to non-private PMD types only.

Fixes: 6cf72047332b ("ethdev: support flow elements with variable length")
Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: Update the patch comment.
---
 lib/ethdev/rte_flow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index d268784532..a93f68abbc 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -54,11 +54,13 @@ rte_flow_conv_copy(void *buf, const void *data, const size_t size,
 	/**
 	 * Allow PMD private flow item
 	 */
-	size_t sz = type >= 0 ? desc[type].size : sizeof(void *);
+	bool rte_type = type >= 0;
+
+	size_t sz = rte_type ? desc[type].size : sizeof(void *);
 	if (buf == NULL || data == NULL)
 		return 0;
 	rte_memcpy(buf, data, (size > sz ? sz : size));
-	if (desc[type].desc_fn)
+	if (rte_type && desc[type].desc_fn)
 		sz += desc[type].desc_fn(size > 0 ? buf : NULL, data);
 	return sz;
 }
-- 
2.33.1

Re: [dpdk-dev] [PATCH v2] ethdev: fix variable length flow elements support

[Ferruh Yigit]

On 11/4/2021 11:27 AM, Gregory Etelson wrote:
> RTE flow API defines two flow elements types - common and PMD private.
> Common RTE flow types are defined in rte_flow.h while PMD private
> types exists inside specific PMD only. Application can create a flow
> rule with PMD private items or actions. RTE flow API restricts
> private PMD types to negative values.
> 
> Current implementation tried to use negative PMD private item type
> value as index in the rte_flow_desc_item[] array.
> 
> The patch allows access to rte_flow_desc_item[] and
> rte_flow_desc_action[] arrays to non-private PMD types only.
> 
> Fixes: 6cf72047332b ("ethdev: support flow elements with variable length")
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.