[dpdk-dev] [DPDK] net/ice: fix hash flow segmentation fault

[dpdk-dev] [DPDK] net/ice: fix hash flow segmentation fault

[taox.zhu]

From: Zhu Tao <taox.zhu@intel.com>

Macro rte_errno is not a static value, so it needs to be updated in all
error handling code.

Patch 'dc36bd5dfd' mistakenly consider that rte_errno is a constant, which
causes the unrecognized flow rule to be marked as recognition success.
Later, when the code tried to parse the flow rule, a null pointer caused
a segmentation fault.

Fixes: dc36bd5dfd ("net/ice: fix flow FDIR/switch memory leak")
Cc: stable@dpdk.org

Signed-off-by: Zhu Tao <taox.zhu@intel.com>
---
 drivers/net/ice/ice_hash.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index d891538bd..e5fb0f344 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 			void **meta,
 			struct rte_flow_error *error)
 {
-	int ret = -rte_errno;
+	int ret = 0;
 	struct ice_pattern_match_item *pattern_match_item;
 	struct rss_meta *rss_meta_ptr;
 
@@ -424,12 +424,16 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 	/* Check rss supported pattern and find matched pattern. */
 	pattern_match_item = ice_search_pattern_match_item(pattern,
 					array, array_len, error);
-	if (!pattern_match_item)
+	if (!pattern_match_item) {
+		ret = -rte_errno;
 		goto error;
+	}
 
 	ret = ice_hash_check_inset(pattern, error);
-	if (ret)
+	if (ret) {
+		ret = -rte_errno;
 		goto error;
+	}
 
 	/* Save protocol header to rss_meta. */
 	*meta = rss_meta_ptr;
@@ -439,8 +443,10 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 	/* Check rss action. */
 	ret = ice_hash_parse_action(pattern_match_item, actions, meta, error);
 error:
-	if (ret)
+	if (ret) {
+		ret = -rte_errno;
 		rte_free(rss_meta_ptr);
+	}
 	rte_free(pattern_match_item);
 
 	return ret;
-- 
2.17.1

Re: [dpdk-dev] [DPDK] net/ice: fix hash flow segmentation fault

[Ye Xiaolong]

On 03/03, taox.zhu@intel.com wrote:
>From: Zhu Tao <taox.zhu@intel.com>
>
>Macro rte_errno is not a static value, so it needs to be updated in all
>error handling code.
>
>Patch 'dc36bd5dfd' mistakenly consider that rte_errno is a constant, which
>causes the unrecognized flow rule to be marked as recognition success.
>Later, when the code tried to parse the flow rule, a null pointer caused
>a segmentation fault.
>
>Fixes: dc36bd5dfd ("net/ice: fix flow FDIR/switch memory leak")

It's recommended to have 12 chars length of commit SHA in Fixes line.
You can set below git alias for convenience.

git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'"

>Cc: stable@dpdk.org
>
>Signed-off-by: Zhu Tao <taox.zhu@intel.com>
>---
> drivers/net/ice/ice_hash.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
>index d891538bd..e5fb0f344 100644
>--- a/drivers/net/ice/ice_hash.c
>+++ b/drivers/net/ice/ice_hash.c
>@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
> 			void **meta,
> 			struct rte_flow_error *error)
> {
>-	int ret = -rte_errno;
>+	int ret = 0;
> 	struct ice_pattern_match_item *pattern_match_item;
> 	struct rss_meta *rss_meta_ptr;
> 
>@@ -424,12 +424,16 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
> 	/* Check rss supported pattern and find matched pattern. */
> 	pattern_match_item = ice_search_pattern_match_item(pattern,
> 					array, array_len, error);
>-	if (!pattern_match_item)
>+	if (!pattern_match_item) {
>+		ret = -rte_errno;
> 		goto error;
>+	}
> 
> 	ret = ice_hash_check_inset(pattern, error);
>-	if (ret)
>+	if (ret) {
>+		ret = -rte_errno;

This seems redundant, since ice_hash_check_inset would return -rte_errno directly.

> 		goto error;
>+	}
> 
> 	/* Save protocol header to rss_meta. */
> 	*meta = rss_meta_ptr;
>@@ -439,8 +443,10 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
> 	/* Check rss action. */
> 	ret = ice_hash_parse_action(pattern_match_item, actions, meta, error);
> error:
>-	if (ret)
>+	if (ret) {
>+		ret = -rte_errno;

Ditto.

> 		rte_free(rss_meta_ptr);
>+	}
> 	rte_free(pattern_match_item);
> 
> 	return ret;
>-- 
>2.17.1
>

Re: [dpdk-dev] [DPDK] net/ice: fix hash flow segmentation fault

[Zhu, TaoX]

Hi Xiaolong,

Commit SHA length non-compliance will be modified in V2 patch.
The reason why they did not return immediately after the error is that they need to release the allocated memory after the error, which is released uniformly in error processing, so they did not return directly.

BR,
Zhu, Tao


> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, March 3, 2020 11:32 AM
> To: Zhu, TaoX <taox.zhu@intel.com>
> Cc: Yang, Qiming <qiming.yang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org; Su, Simei <simei.su@intel.com>;
> Cao, Yahui <yahui.cao@intel.com>; stable@dpdk.org
> Subject: Re: [DPDK] net/ice: fix hash flow segmentation fault
> 
> On 03/03, taox.zhu@intel.com wrote:
> >From: Zhu Tao <taox.zhu@intel.com>
> >
> >Macro rte_errno is not a static value, so it needs to be updated in all
> >error handling code.
> >
> >Patch 'dc36bd5dfd' mistakenly consider that rte_errno is a constant,
> >which causes the unrecognized flow rule to be marked as recognition
> success.
> >Later, when the code tried to parse the flow rule, a null pointer
> >caused a segmentation fault.
> >
> >Fixes: dc36bd5dfd ("net/ice: fix flow FDIR/switch memory leak")
> 
> It's recommended to have 12 chars length of commit SHA in Fixes line.
> You can set below git alias for convenience.
> 
> git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h
> (\"%s\")%nCc: %ae'"
> 
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Zhu Tao <taox.zhu@intel.com>
> >---
> > drivers/net/ice/ice_hash.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
> >index d891538bd..e5fb0f344 100644
> >--- a/drivers/net/ice/ice_hash.c
> >+++ b/drivers/net/ice/ice_hash.c
> >@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused
> struct ice_adapter *ad,
> > 			void **meta,
> > 			struct rte_flow_error *error)
> > {
> >-	int ret = -rte_errno;
> >+	int ret = 0;
> > 	struct ice_pattern_match_item *pattern_match_item;
> > 	struct rss_meta *rss_meta_ptr;
> >
> >@@ -424,12 +424,16 @@ ice_hash_parse_pattern_action(__rte_unused
> struct ice_adapter *ad,
> > 	/* Check rss supported pattern and find matched pattern. */
> > 	pattern_match_item = ice_search_pattern_match_item(pattern,
> > 					array, array_len, error);
> >-	if (!pattern_match_item)
> >+	if (!pattern_match_item) {
> >+		ret = -rte_errno;
> > 		goto error;
> >+	}
> >
> > 	ret = ice_hash_check_inset(pattern, error);
> >-	if (ret)
> >+	if (ret) {
> >+		ret = -rte_errno;
> 
> This seems redundant, since ice_hash_check_inset would return -rte_errno
> directly.
> 
> > 		goto error;
> >+	}
> >
> > 	/* Save protocol header to rss_meta. */
> > 	*meta = rss_meta_ptr;
> >@@ -439,8 +443,10 @@ ice_hash_parse_pattern_action(__rte_unused
> struct ice_adapter *ad,
> > 	/* Check rss action. */
> > 	ret = ice_hash_parse_action(pattern_match_item, actions, meta,
> >error);
> > error:
> >-	if (ret)
> >+	if (ret) {
> >+		ret = -rte_errno;
> 
> Ditto.
> 
> > 		rte_free(rss_meta_ptr);
> >+	}
> > 	rte_free(pattern_match_item);
> >
> > 	return ret;
> >--
> >2.17.1
> >

[dpdk-dev] [PATCH v2] net/ice: fix hash flow segmentation fault

[taox.zhu]

From: Zhu Tao <taox.zhu@intel.com>

Macro rte_errno is not a static value, so it needs to be updated in all
error handling code.

Patch 'dc36bd5dfdeb' mistakenly consider that rte_errno is a constant, which
causes the unrecognized flow rule to be marked as recognition success.
Later, when the code tried to parse the flow rule, a null pointer caused
a segmentation fault.

Fixes: dc36bd5dfdeb ("net/ice: fix flow FDIR/switch memory leak")
Cc: stable@dpdk.org

Signed-off-by: Zhu Tao <taox.zhu@intel.com>
---
v2 Changes:

 Commit message: Use 12 chars length of commit SHA in Fixes line.

 drivers/net/ice/ice_hash.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index d891538bd..e5fb0f344 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 			void **meta,
 			struct rte_flow_error *error)
 {
-	int ret = -rte_errno;
+	int ret = 0;
 	struct ice_pattern_match_item *pattern_match_item;
 	struct rss_meta *rss_meta_ptr;
 
@@ -424,12 +424,16 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 	/* Check rss supported pattern and find matched pattern. */
 	pattern_match_item = ice_search_pattern_match_item(pattern,
 					array, array_len, error);
-	if (!pattern_match_item)
+	if (!pattern_match_item) {
+		ret = -rte_errno;
 		goto error;
+	}
 
 	ret = ice_hash_check_inset(pattern, error);
-	if (ret)
+	if (ret) {
+		ret = -rte_errno;
 		goto error;
+	}
 
 	/* Save protocol header to rss_meta. */
 	*meta = rss_meta_ptr;
@@ -439,8 +443,10 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 	/* Check rss action. */
 	ret = ice_hash_parse_action(pattern_match_item, actions, meta, error);
 error:
-	if (ret)
+	if (ret) {
+		ret = -rte_errno;
 		rte_free(rss_meta_ptr);
+	}
 	rte_free(pattern_match_item);
 
 	return ret;
-- 
2.17.1

[dpdk-dev] [PATCH v3] net/ice: fix hash flow segmentation fault

[taox.zhu]

From: Zhu Tao <taox.zhu@intel.com>

Macro rte_errno is not a static value, so it needs to be updated in all
error handling code.

Patch 'dc36bd5dfdeb' mistakenly consider that rte_errno is a constant,
which causes the unrecognized flow rule to be marked as recognition
success. Later, when the code tried to parse the flow rule, a null pointer
caused a segmentation fault.

Fixes: dc36bd5dfdeb ("net/ice: fix flow FDIR/switch memory leak")
Cc: stable@dpdk.org

Signed-off-by: Zhu Tao <taox.zhu@intel.com>
---
v3 Changes:

 Commit message: Use 12 chars length of commit SHA in Fixes line.
 Code: Delete redundant assignment codes.

v2 Changes:

 Commit message: Use 12 chars length of commit SHA in Fixes line.

 drivers/net/ice/ice_hash.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index d891538bd..0e9c3c4e5 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 			void **meta,
 			struct rte_flow_error *error)
 {
-	int ret = -rte_errno;
+	int ret = 0;
 	struct ice_pattern_match_item *pattern_match_item;
 	struct rss_meta *rss_meta_ptr;
 
@@ -424,8 +424,10 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
 	/* Check rss supported pattern and find matched pattern. */
 	pattern_match_item = ice_search_pattern_match_item(pattern,
 					array, array_len, error);
-	if (!pattern_match_item)
+	if (!pattern_match_item) {
+		ret = -rte_errno;
 		goto error;
+	}
 
 	ret = ice_hash_check_inset(pattern, error);
 	if (ret)
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3] net/ice: fix hash flow segmentation fault

[Ye Xiaolong]

On 03/03, taox.zhu@intel.com wrote:
>From: Zhu Tao <taox.zhu@intel.com>
>
>Macro rte_errno is not a static value, so it needs to be updated in all
>error handling code.
>
>Patch 'dc36bd5dfdeb' mistakenly consider that rte_errno is a constant,
>which causes the unrecognized flow rule to be marked as recognition
>success. Later, when the code tried to parse the flow rule, a null pointer
>caused a segmentation fault.
>
>Fixes: dc36bd5dfdeb ("net/ice: fix flow FDIR/switch memory leak")
>Cc: stable@dpdk.org
>
>Signed-off-by: Zhu Tao <taox.zhu@intel.com>
>---
>v3 Changes:
>
> Commit message: Use 12 chars length of commit SHA in Fixes line.
> Code: Delete redundant assignment codes.
>
>v2 Changes:
>
> Commit message: Use 12 chars length of commit SHA in Fixes line.
>
> drivers/net/ice/ice_hash.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
>index d891538bd..0e9c3c4e5 100644
>--- a/drivers/net/ice/ice_hash.c
>+++ b/drivers/net/ice/ice_hash.c
>@@ -409,7 +409,7 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
> 			void **meta,
> 			struct rte_flow_error *error)
> {
>-	int ret = -rte_errno;
>+	int ret = 0;
> 	struct ice_pattern_match_item *pattern_match_item;
> 	struct rss_meta *rss_meta_ptr;
> 
>@@ -424,8 +424,10 @@ ice_hash_parse_pattern_action(__rte_unused struct ice_adapter *ad,
> 	/* Check rss supported pattern and find matched pattern. */
> 	pattern_match_item = ice_search_pattern_match_item(pattern,
> 					array, array_len, error);
>-	if (!pattern_match_item)
>+	if (!pattern_match_item) {
>+		ret = -rte_errno;
> 		goto error;
>+	}
> 
> 	ret = ice_hash_check_inset(pattern, error);
> 	if (ret)
>-- 
>2.17.1
>

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel, Thanks.