[dpdk-dev] net/i40e: fix VF reset stats crash

  Calling i40evf_dev_xstats_reset can sometimes crash.

Fixed issue by adding a pstats NULL check.

Fixes: da61cd0849766 ("i40evf: add extended stats")
       8210e9e0d805e ("net/i40e: fix clear xstats bug in VF")
Cc: wei.zhao1@intel.com

Signed-off-by: David C Harton <dharton@cisco.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David C Harton
> Sent: Thursday, January 11, 2018 1:24 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>
> Cc: dev@dpdk.org; David C Harton <dharton@cisco.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [dpdk-dev] [PATCH] net/i40e: fix VF reset stats crash
> 
> Calling i40evf_dev_xstats_reset can sometimes crash.
> 
> Fixed issue by adding a pstats NULL check.

Perhaps add a note here, that the root cause of (pstats == NULL)
is a valid error return from i40evf_query_stats(), if the vf
command fails to execute.

Hence, we are not hiding symptoms of a different issue here,
this NULL check is required to validate the returned values.


> Fixes: da61cd0849766 ("i40evf: add extended stats")
>        8210e9e0d805e ("net/i40e: fix clear xstats bug in VF")
> Cc: wei.zhao1@intel.com
> 
> Signed-off-by: David C Harton <dharton@cisco.com>


Acked-by: Harry van Haaren <harry.van.haaren@intel.com>


> ---
>  drivers/net/i40e/i40e_ethdev_vf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index b96d77a..9c773fe 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -923,7 +923,8 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
>  	i40evf_query_stats(dev, &pstats);
> 
>  	/* set stats offset base on current values */
> -	vf->vsi.eth_stats_offset = *pstats;
> +	if (pstats != NULL)
> +		vf->vsi.eth_stats_offset = *pstats;
>  }
> 
>  static int i40evf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> *dev,
> --
> 2.10.3.dirty
  

> -----Original Message-----
> From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David C Harton
> >
> > Calling i40evf_dev_xstats_reset can sometimes crash.
> >
> > Fixed issue by adding a pstats NULL check.
> 
> Perhaps add a note here, that the root cause of (pstats == NULL) is a
> valid error return from i40evf_query_stats(), if the vf command fails to
> execute.
> 
> Hence, we are not hiding symptoms of a different issue here, this NULL
> check is required to validate the returned values.
>
 
NULL is returned if the call fails.  
If desired I can capture the return code and base the check off the rc instead.
That's the pattern used in a couple other places.

Thanks,
Dave
  

> From: David Harton (dharton) [mailto:dharton@cisco.com]
> Sent: Thursday, January 11, 2018 2:59 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: fix VF reset stats crash
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> >
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David C Harton
> > >
> > > Calling i40evf_dev_xstats_reset can sometimes crash.
> > >
> > > Fixed issue by adding a pstats NULL check.
> >
> > Perhaps add a note here, that the root cause of (pstats == NULL) is a
> > valid error return from i40evf_query_stats(), if the vf command fails to
> > execute.
> >
> > Hence, we are not hiding symptoms of a different issue here, this NULL
> > check is required to validate the returned values.
> >
> 
> NULL is returned if the call fails.
> If desired I can capture the return code and base the check off the rc
> instead.
> That's the pattern used in a couple other places.

If you prefer that implementation of the check, you can keep my Ack, I don't mind much.

I reviewed as in my experience a NULL pointer can often be detected earlier, but that wasn't the case here.

PS: Apologies I didn't reply to v2, was reading "up" and didn't check for newer..