[dpdk-dev] [PATCH] net/ice: fix segmentation fault for RSS init

[dpdk-dev] [PATCH] net/ice: fix segmentation fault for RSS init

[Simei Su]

This patch fixes core dump issue when entering safe mode
with a wrong package file.

Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")

Signed-off-by: Simei Su <simei.su@intel.com>
---
 drivers/net/ice/ice_hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index 3381b45..04ec30f 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -236,7 +236,7 @@ struct ice_hash_match_type ice_hash_type_list[] = {
 static int
 ice_hash_init(struct ice_adapter *ad)
 {
-	struct ice_flow_parser *parser = NULL;
+	struct ice_flow_parser *parser;
 
 	if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT)
 		parser = &ice_hash_parser_os;
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init

[Simei Su]

This patch fixes core dump issue when entering safe mode
with a wrong package file. This patch also fixes build
failure issue.

Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")

Signed-off-by: Simei Su <simei.su@intel.com>
---
 drivers/net/ice/ice_hash.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
index 3381b45..08b2502 100644
--- a/drivers/net/ice/ice_hash.c
+++ b/drivers/net/ice/ice_hash.c
@@ -236,12 +236,12 @@ struct ice_hash_match_type ice_hash_type_list[] = {
 static int
 ice_hash_init(struct ice_adapter *ad)
 {
-	struct ice_flow_parser *parser = NULL;
+	struct ice_flow_parser *parser;
 
-	if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT)
-		parser = &ice_hash_parser_os;
-	else if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
+	if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
 		parser = &ice_hash_parser_comms;
+	else
+		parser = &ice_hash_parser_os;
 
 	return ice_register_parser(parser, ad);
 }
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/ice: fix segmentation fault for RSS init

[Stillwell Jr, Paul M]

Can you provide more details on how this fixes the issue? Looking at the code, the pointer *parser should get set by the code so I'm not sure how this change does anything.

Paul

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su
> Sent: Tuesday, November 5, 2019 11:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ice: fix segmentation fault for RSS init
> 
> This patch fixes core dump issue when entering safe mode with a wrong
> package file.
> 
> Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
>  drivers/net/ice/ice_hash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index
> 3381b45..04ec30f 100644
> --- a/drivers/net/ice/ice_hash.c
> +++ b/drivers/net/ice/ice_hash.c
> @@ -236,7 +236,7 @@ struct ice_hash_match_type ice_hash_type_list[] = {
> static int  ice_hash_init(struct ice_adapter *ad)  {
> -	struct ice_flow_parser *parser = NULL;
> +	struct ice_flow_parser *parser;
> 
>  	if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT)
>  		parser = &ice_hash_parser_os;
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init

[Stillwell Jr, Paul M]

Disregard my previous email, I didn't see this one. See my comments inline.

Paul

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su
> Sent: Wednesday, November 6, 2019 12:26 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init
> 
> This patch fixes core dump issue when entering safe mode with a wrong
> package file. This patch also fixes build failure issue.
> 
> Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
>  drivers/net/ice/ice_hash.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index
> 3381b45..08b2502 100644
> --- a/drivers/net/ice/ice_hash.c
> +++ b/drivers/net/ice/ice_hash.c
> @@ -236,12 +236,12 @@ struct ice_hash_match_type ice_hash_type_list[] = {
> static int  ice_hash_init(struct ice_adapter *ad)  {
> -	struct ice_flow_parser *parser = NULL;
> +	struct ice_flow_parser *parser;
> 
> -	if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT)
> -		parser = &ice_hash_parser_os;
> -	else if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
> +	if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
>  		parser = &ice_hash_parser_comms;
> +	else
> +		parser = &ice_hash_parser_os;

This doesn't make sense to me. Why set up a parser at all if we are in safe mode? Safe mode means 1 queue so there isn't anything to RSS to. It seems like if we are in safe mode we should just return an error here with a message that we are in safe mode.

> 
>  	return ice_register_parser(parser, ad);  }
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init

[Su, Simei]

Hi, Paul

> -----Original Message-----
> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>
> Sent: Thursday, November 7, 2019 12:10 AM
> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye,
> Xiaolong <xiaolong.ye@intel.com>; Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init
> 
> Disregard my previous email, I didn't see this one. See my comments inline.
> 
> Paul
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su
> > Sent: Wednesday, November 6, 2019 12:26 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong
> > <xiaolong.ye@intel.com>; Yang, Qiming <qiming.yang@intel.com>
> > Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> > Subject: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS
> > init
> >
> > This patch fixes core dump issue when entering safe mode with a wrong
> > package file. This patch also fixes build failure issue.
> >
> > Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")
> >
> > Signed-off-by: Simei Su <simei.su@intel.com>
> > ---
> >  drivers/net/ice/ice_hash.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
> > index
> > 3381b45..08b2502 100644
> > --- a/drivers/net/ice/ice_hash.c
> > +++ b/drivers/net/ice/ice_hash.c
> > @@ -236,12 +236,12 @@ struct ice_hash_match_type
> ice_hash_type_list[]
> > = { static int  ice_hash_init(struct ice_adapter *ad)  { -struct
> > ice_flow_parser *parser = NULL;
> > +struct ice_flow_parser *parser;
> >
> > -if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT) -parser =
> > &ice_hash_parser_os; -else if (ad->active_pkg_type ==
> > ICE_PKG_TYPE_COMMS)
> > +if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
> >  parser = &ice_hash_parser_comms;
> > +else
> > +parser = &ice_hash_parser_os;
> 
> This doesn't make sense to me. Why set up a parser at all if we are in safe
> mode? Safe mode means 1 queue so there isn't anything to RSS to. It seems
> like if we are in safe mode we should just return an error here with a message
> that we are in safe mode.
> 

  When application starts with a wrong ice.pkg, the output shows:
    ice_load_pkg(): failed to allocate buf of size 0 for package
    ice_dev_init(): Failed to load the DDP package,Entering Safe Mode
    Segmentation fault (core dumped)

  I used gdb to track this issue and found it cored dump at ice_hash_init(). 
  It is caused by the pointer *parser is set NULL. If I don't set NULL, the output shows normally:
    ice_load_pkg(): failed to allocate buf of size 0 for package
    ice_dev_init(): Failed to load the DDP package,Entering Safe Mode
    ice_init_rss(): RSS is not supported in safe mode

  As to build failure issue, I used "if, else if" previously and didn't consider all cases other than "if, else if". So the test report shows:
  error: 'parser' may be used uninitialized in this function. I changed it to "if, else". Thanks!

Br
Simei

> >
> >  return ice_register_parser(parser, ad);  }
> > --
> > 1.8.3.1
> 

[dpdk-dev] [PATCH v3] net/ice: fix segmentation fault with a wrong package

[Simei Su]

This patch fixes core dump issue when entering safe mode with a
wrong ice.pkg. In safe mode, rte_flow is not supported and it
won't initialize any flow engine.

Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")

Signed-off-by: Simei Su <simei.su@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index d81eb5e..2a28d8e 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 	/* get base queue pairs index  in the device */
 	ice_base_queue_get(pf);
 
-	ret = ice_flow_init(ad);
-	if (ret) {
-		PMD_INIT_LOG(ERR, "Failed to initialize flow");
-		return ret;
+	if (!ad->is_safe_mode) {
+		ret = ice_flow_init(ad);
+		if (ret) {
+			PMD_INIT_LOG(ERR, "Failed to initialize flow");
+			return ret;
+		}
 	}
 
 	ret = ice_reset_fxp_resource(hw);
@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 
 	ice_dev_stop(dev);
 
-	ice_flow_uninit(ad);
+	if (!ad->is_safe_mode)
+		ice_flow_uninit(ad);
 
 	/* release all queue resource */
 	ice_free_queues(dev);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] net/ice: fix segmentation fault with a wrong package

[Zhang, Qi Z]

> -----Original Message-----
> From: Su, Simei <simei.su@intel.com>
> Sent: Thursday, November 7, 2019 1:47 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: [PATCH v3] net/ice: fix segmentation fault with a wrong package
> 
> This patch fixes core dump issue when entering safe mode with a wrong
> ice.pkg. In safe mode, rte_flow is not supported and it won't initialize any flow
> engine.
> 
> Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> 
> Signed-off-by: Simei Su <simei.su@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Re: [dpdk-dev] [PATCH v3] net/ice: fix segmentation fault with a wrong package

[Ye Xiaolong]

Hi, simei

On 11/07, Simei Su wrote:
>This patch fixes core dump issue when entering safe mode with a
>wrong ice.pkg. In safe mode, rte_flow is not supported and it
>won't initialize any flow engine.
>
>Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
>
>Signed-off-by: Simei Su <simei.su@intel.com>
>---
> drivers/net/ice/ice_ethdev.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
>index d81eb5e..2a28d8e 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> 	/* get base queue pairs index  in the device */
> 	ice_base_queue_get(pf);
> 
>-	ret = ice_flow_init(ad);
>-	if (ret) {
>-		PMD_INIT_LOG(ERR, "Failed to initialize flow");
>-		return ret;
>+	if (!ad->is_safe_mode) {
>+		ret = ice_flow_init(ad);
>+		if (ret) {
>+			PMD_INIT_LOG(ERR, "Failed to initialize flow");
>+			return ret;
>+		}

Do we need to print out some message indicates that now ice is in safe mode? 

Thanks,
Xiaolong

> 	}
> 
> 	ret = ice_reset_fxp_resource(hw);
>@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> 
> 	ice_dev_stop(dev);
> 
>-	ice_flow_uninit(ad);
>+	if (!ad->is_safe_mode)
>+		ice_flow_uninit(ad);
> 
> 	/* release all queue resource */
> 	ice_free_queues(dev);
>-- 
>1.8.3.1
>

Re: [dpdk-dev] [PATCH v3] net/ice: fix segmentation fault with a wrong package

[Su, Simei]

Hi, xiaolong

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Monday, November 11, 2019 5:06 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH v3] net/ice: fix segmentation fault with a wrong package
> 
> Hi, simei
> 
> On 11/07, Simei Su wrote:
> >This patch fixes core dump issue when entering safe mode with a wrong
> >ice.pkg. In safe mode, rte_flow is not supported and it won't
> >initialize any flow engine.
> >
> >Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
> >
> >Signed-off-by: Simei Su <simei.su@intel.com>
> >---
> > drivers/net/ice/ice_ethdev.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/net/ice/ice_ethdev.c
> >b/drivers/net/ice/ice_ethdev.c index d81eb5e..2a28d8e 100644
> >--- a/drivers/net/ice/ice_ethdev.c
> >+++ b/drivers/net/ice/ice_ethdev.c
> >@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev
> *dev)
> > 	/* get base queue pairs index  in the device */
> > 	ice_base_queue_get(pf);
> >
> >-	ret = ice_flow_init(ad);
> >-	if (ret) {
> >-		PMD_INIT_LOG(ERR, "Failed to initialize flow");
> >-		return ret;
> >+	if (!ad->is_safe_mode) {
> >+		ret = ice_flow_init(ad);
> >+		if (ret) {
> >+			PMD_INIT_LOG(ERR, "Failed to initialize flow");
> >+			return ret;
> >+		}
> 
> Do we need to print out some message indicates that now ice is in safe mode?
> 

  We already have print info when ice is in safe mode. Thanks!

  Br
  Simei

> Thanks,
> Xiaolong
> 
> > 	}
> >
> > 	ret = ice_reset_fxp_resource(hw);
> >@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev
> >*dev)
> >
> > 	ice_dev_stop(dev);
> >
> >-	ice_flow_uninit(ad);
> >+	if (!ad->is_safe_mode)
> >+		ice_flow_uninit(ad);
> >
> > 	/* release all queue resource */
> > 	ice_free_queues(dev);
> >--
> >1.8.3.1
> >

Re: [dpdk-dev] [PATCH v3] net/ice: fix segmentation fault with a wrong package

[Ye Xiaolong]

On 11/07, Simei Su wrote:
>This patch fixes core dump issue when entering safe mode with a
>wrong ice.pkg. In safe mode, rte_flow is not supported and it
>won't initialize any flow engine.
>
>Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
>
>Signed-off-by: Simei Su <simei.su@intel.com>
>---
> drivers/net/ice/ice_ethdev.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
>index d81eb5e..2a28d8e 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> 	/* get base queue pairs index  in the device */
> 	ice_base_queue_get(pf);
> 
>-	ret = ice_flow_init(ad);
>-	if (ret) {
>-		PMD_INIT_LOG(ERR, "Failed to initialize flow");
>-		return ret;
>+	if (!ad->is_safe_mode) {
>+		ret = ice_flow_init(ad);
>+		if (ret) {
>+			PMD_INIT_LOG(ERR, "Failed to initialize flow");
>+			return ret;
>+		}
> 	}
> 
> 	ret = ice_reset_fxp_resource(hw);
>@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> 
> 	ice_dev_stop(dev);
> 
>-	ice_flow_uninit(ad);
>+	if (!ad->is_safe_mode)
>+		ice_flow_uninit(ad);
> 
> 	/* release all queue resource */
> 	ice_free_queues(dev);
>-- 
>1.8.3.1
>

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