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

["Zhu, TaoX" <taox.zhu@intel.com>]

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
> >