diff mbox

[dpdk-dev,2/2] ethdev: fix shallow copy of flow API RAW item

Message ID 20180516154052.16836-2-adrien.mazarguil@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Adrien Mazarguil May 16, 2018, 3:41 p.m. UTC 
  Like original commit mentioned below, this fix synchronizes flow rule copy
function with testpmd's own implementation following "app/testpmd: fix copy
of raw flow item (revisited)".

Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
Cc: stable@dpdk.org
Cc: Qi Zhang <qi.z.zhang@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_ethdev/rte_flow.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

Comments

Ferruh Yigit May 18, 2018, 5:06 p.m. UTC | #1 
On 5/16/2018 4:41 PM, Adrien Mazarguil wrote:
> Like original commit mentioned below, this fix synchronizes flow rule copy
> function with testpmd's own implementation following "app/testpmd: fix copy
> of raw flow item (revisited)".
> 
> Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
> Cc: stable@dpdk.org
> Cc: Qi Zhang <qi.z.zhang@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Hi Thomas,

What do you suggest about this one?
Scope is limited to rte_flow but still many features are now relies on rte_flow,
what is your comment on getting this in rc5?


> ---
>  lib/librte_ethdev/rte_flow.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 7947529da..b2afba089 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -300,17 +300,26 @@ flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
>  		    enum item_spec_type type)
>  {
>  	size_t size = 0;
> -	const void *item_spec =
> +	const void *data =
>  		type == ITEM_SPEC ? item->spec :
>  		type == ITEM_LAST ? item->last :
>  		type == ITEM_MASK ? item->mask :
>  		NULL;
>  
> -	if (!item_spec)
> +	if (!item->spec || !data)
>  		goto empty;
>  	switch (item->type) {
>  		union {
>  			const struct rte_flow_item_raw *raw;
> +		} spec;
> +		union {
> +			const struct rte_flow_item_raw *raw;
> +		} last;
> +		union {
> +			const struct rte_flow_item_raw *raw;
> +		} mask;
> +		union {
> +			const struct rte_flow_item_raw *raw;
>  		} src;
>  		union {
>  			struct rte_flow_item_raw *raw;
> @@ -318,11 +327,21 @@ flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
>  		size_t off;
>  
>  	case RTE_FLOW_ITEM_TYPE_RAW:
> -		src.raw = item_spec;
> +		spec.raw = item->spec;
> +		last.raw = item->last ? item->last : item->spec;
> +		mask.raw = item->mask ? item->mask : &rte_flow_item_raw_mask;
> +		src.raw = data;
>  		dst.raw = buf;
>  		off = RTE_ALIGN_CEIL(sizeof(struct rte_flow_item_raw),
>  				     sizeof(*src.raw->pattern));
> -		size = off + src.raw->length * sizeof(*src.raw->pattern);
> +		if (type == ITEM_SPEC ||
> +		    (type == ITEM_MASK &&
> +		     ((spec.raw->length & mask.raw->length) >=
> +		      (last.raw->length & mask.raw->length))))
> +			size = spec.raw->length & mask.raw->length;
> +		else
> +			size = last.raw->length & mask.raw->length;
> +		size = off + size * sizeof(*src.raw->pattern);
>  		if (dst.raw) {
>  			memcpy(dst.raw, src.raw, sizeof(*src.raw));
>  			dst.raw->pattern = memcpy((uint8_t *)dst.raw + off,
> @@ -333,7 +352,7 @@ flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
>  	default:
>  		size = rte_flow_desc_item[item->type].size;
>  		if (buf)
> -			memcpy(buf, item_spec, size);
> +			memcpy(buf, data, size);
>  		break;
>  	}
>  empty:
>
Thomas Monjalon May 19, 2018, 2:25 p.m. UTC | #2 
18/05/2018 19:06, Ferruh Yigit:
> On 5/16/2018 4:41 PM, Adrien Mazarguil wrote:
> > Like original commit mentioned below, this fix synchronizes flow rule copy
> > function with testpmd's own implementation following "app/testpmd: fix copy
> > of raw flow item (revisited)".
> > 
> > Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
> > Cc: stable@dpdk.org
> > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Hi Thomas,
> 
> What do you suggest about this one?
> Scope is limited to rte_flow but still many features are now relies on rte_flow,
> what is your comment on getting this in rc5?

We need to know exactly what is broken.
If nothing serious, it can wait 18.08.

Adrien, please can you describe the use case, the issue and the impact?
Adrien Mazarguil May 21, 2018, 8:24 a.m. UTC | #3 
On Sat, May 19, 2018 at 04:25:15PM +0200, Thomas Monjalon wrote:
> 18/05/2018 19:06, Ferruh Yigit:
> > On 5/16/2018 4:41 PM, Adrien Mazarguil wrote:
> > > Like original commit mentioned below, this fix synchronizes flow rule copy
> > > function with testpmd's own implementation following "app/testpmd: fix copy
> > > of raw flow item (revisited)".
> > > 
> > > Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
> > > Cc: stable@dpdk.org
> > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > 
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > 
> > Hi Thomas,
> > 
> > What do you suggest about this one?
> > Scope is limited to rte_flow but still many features are now relies on rte_flow,
> > what is your comment on getting this in rc5?
> 
> We need to know exactly what is broken.
> If nothing serious, it can wait 18.08.
> 
> Adrien, please can you describe the use case, the issue and the impact?

A prior patch [1] (applied as "app/testpmd: fix copy of raw flow item"),
addresses a crash in testpmd's flow copy function.

The first patch of the present series [2] addresses remaining issues with
its behavior which is, in fact, what caused the original issue.

While both patches focus on testpmd, rte_flow also exposes its own public
copy function with the exact same code that breaks when encountering a RAW
pattern item. Primary users for this function are bonding and failsafe
PMDs. 

This patch therefore addresses both [1] and [2] at once for rte_flow_copy().

[1] "app/testpmd: fix invalid memory access"
    http://dpdk.org/ml/archives/dev/2018-May/100364.html

[2] "app/testpmd: fix copy of raw flow item (revisited)"
    http://dpdk.org/ml/archives/dev/2018-May/101994.html
Ferruh Yigit May 21, 2018, 10:44 a.m. UTC | #4 
On 5/21/2018 9:24 AM, Adrien Mazarguil wrote:
> On Sat, May 19, 2018 at 04:25:15PM +0200, Thomas Monjalon wrote:
>> 18/05/2018 19:06, Ferruh Yigit:
>>> On 5/16/2018 4:41 PM, Adrien Mazarguil wrote:
>>>> Like original commit mentioned below, this fix synchronizes flow rule copy
>>>> function with testpmd's own implementation following "app/testpmd: fix copy
>>>> of raw flow item (revisited)".
>>>>
>>>> Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
>>>> Cc: stable@dpdk.org
>>>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>>>>
>>>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>
>>> Hi Thomas,
>>>
>>> What do you suggest about this one?
>>> Scope is limited to rte_flow but still many features are now relies on rte_flow,
>>> what is your comment on getting this in rc5?
>>
>> We need to know exactly what is broken.
>> If nothing serious, it can wait 18.08.
>>
>> Adrien, please can you describe the use case, the issue and the impact?
> 
> A prior patch [1] (applied as "app/testpmd: fix copy of raw flow item"),
> addresses a crash in testpmd's flow copy function.
> 
> The first patch of the present series [2] addresses remaining issues with
> its behavior which is, in fact, what caused the original issue.
> 
> While both patches focus on testpmd, rte_flow also exposes its own public
> copy function with the exact same code that breaks when encountering a RAW
> pattern item. Primary users for this function are bonding and failsafe
> PMDs. 
> 
> This patch therefore addresses both [1] and [2] at once for rte_flow_copy().

Hi Adrien,

What is the effect of _not_ getting this patch, just trying to understand if
this is something to get for this release or postpone to next one.

Thanks,
ferruh

> 
> [1] "app/testpmd: fix invalid memory access"
>     http://dpdk.org/ml/archives/dev/2018-May/100364.html
> 
> [2] "app/testpmd: fix copy of raw flow item (revisited)"
>     http://dpdk.org/ml/archives/dev/2018-May/101994.html
>
Adrien Mazarguil May 21, 2018, 11:18 a.m. UTC | #5 
On Mon, May 21, 2018 at 11:44:33AM +0100, Ferruh Yigit wrote:
> On 5/21/2018 9:24 AM, Adrien Mazarguil wrote:
> > On Sat, May 19, 2018 at 04:25:15PM +0200, Thomas Monjalon wrote:
> >> 18/05/2018 19:06, Ferruh Yigit:
> >>> On 5/16/2018 4:41 PM, Adrien Mazarguil wrote:
> >>>> Like original commit mentioned below, this fix synchronizes flow rule copy
> >>>> function with testpmd's own implementation following "app/testpmd: fix copy
> >>>> of raw flow item (revisited)".
> >>>>
> >>>> Fixes: d0ad8648b1c5 ("ethdev: fix shallow copy of flow API RSS action")
> >>>> Cc: stable@dpdk.org
> >>>> Cc: Qi Zhang <qi.z.zhang@intel.com>
> >>>>
> >>>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>>
> >>> Hi Thomas,
> >>>
> >>> What do you suggest about this one?
> >>> Scope is limited to rte_flow but still many features are now relies on rte_flow,
> >>> what is your comment on getting this in rc5?
> >>
> >> We need to know exactly what is broken.
> >> If nothing serious, it can wait 18.08.
> >>
> >> Adrien, please can you describe the use case, the issue and the impact?
> > 
> > A prior patch [1] (applied as "app/testpmd: fix copy of raw flow item"),
> > addresses a crash in testpmd's flow copy function.
> > 
> > The first patch of the present series [2] addresses remaining issues with
> > its behavior which is, in fact, what caused the original issue.
> > 
> > While both patches focus on testpmd, rte_flow also exposes its own public
> > copy function with the exact same code that breaks when encountering a RAW
> > pattern item. Primary users for this function are bonding and failsafe
> > PMDs. 
> > 
> > This patch therefore addresses both [1] and [2] at once for rte_flow_copy().
> 
> Hi Adrien,
> 
> What is the effect of _not_ getting this patch, just trying to understand if
> this is something to get for this release or postpone to next one.

Hi Ferruh,

*Not* getting this patch means rte_flow_copy() crashes when user creates a
flow rule that involves the RAW pattern item on top of either bonding or
failsafe PMDs.

Ditto for any external application that relies on rte_flow_copy() combined
with the RAW pattern item.

I'll send v2 to provide a bit more info and fix the wrong commit ID on the
"Fixes" line.

> > [1] "app/testpmd: fix invalid memory access"
> >     http://dpdk.org/ml/archives/dev/2018-May/100364.html
> > 
> > [2] "app/testpmd: fix copy of raw flow item (revisited)"
> >     http://dpdk.org/ml/archives/dev/2018-May/101994.html
diff mbox

Patch

diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 7947529da..b2afba089 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -300,17 +300,26 @@  flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
 		    enum item_spec_type type)
 {
 	size_t size = 0;
-	const void *item_spec =
+	const void *data =
 		type == ITEM_SPEC ? item->spec :
 		type == ITEM_LAST ? item->last :
 		type == ITEM_MASK ? item->mask :
 		NULL;
 
-	if (!item_spec)
+	if (!item->spec || !data)
 		goto empty;
 	switch (item->type) {
 		union {
 			const struct rte_flow_item_raw *raw;
+		} spec;
+		union {
+			const struct rte_flow_item_raw *raw;
+		} last;
+		union {
+			const struct rte_flow_item_raw *raw;
+		} mask;
+		union {
+			const struct rte_flow_item_raw *raw;
 		} src;
 		union {
 			struct rte_flow_item_raw *raw;
@@ -318,11 +327,21 @@  flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
 		size_t off;
 
 	case RTE_FLOW_ITEM_TYPE_RAW:
-		src.raw = item_spec;
+		spec.raw = item->spec;
+		last.raw = item->last ? item->last : item->spec;
+		mask.raw = item->mask ? item->mask : &rte_flow_item_raw_mask;
+		src.raw = data;
 		dst.raw = buf;
 		off = RTE_ALIGN_CEIL(sizeof(struct rte_flow_item_raw),
 				     sizeof(*src.raw->pattern));
-		size = off + src.raw->length * sizeof(*src.raw->pattern);
+		if (type == ITEM_SPEC ||
+		    (type == ITEM_MASK &&
+		     ((spec.raw->length & mask.raw->length) >=
+		      (last.raw->length & mask.raw->length))))
+			size = spec.raw->length & mask.raw->length;
+		else
+			size = last.raw->length & mask.raw->length;
+		size = off + size * sizeof(*src.raw->pattern);
 		if (dst.raw) {
 			memcpy(dst.raw, src.raw, sizeof(*src.raw));
 			dst.raw->pattern = memcpy((uint8_t *)dst.raw + off,
@@ -333,7 +352,7 @@  flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
 	default:
 		size = rte_flow_desc_item[item->type].size;
 		if (buf)
-			memcpy(buf, item_spec, size);
+			memcpy(buf, data, size);
 		break;
 	}
 empty: