[DPDK] net/ice: fix hash flow segmentation fault
diff mbox series

Message ID 20200303015557.63621-1-taox.zhu@intel.com
State Superseded, archived
Delegated to: xiaolong ye
Headers show
Series
  • [DPDK] net/ice: fix hash flow segmentation fault
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Zhu, TaoX March 3, 2020, 1:55 a.m. UTC
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(-)

Comments

Xiaolong Ye March 3, 2020, 3:32 a.m. UTC | #1
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
>
Zhu, TaoX March 3, 2020, 5:07 a.m. UTC | #2
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
> >

Patch
diff mbox series

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;