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

["Su, Simei" <simei.su@intel.com>]

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
>