Re: [dpdk-dev] [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 23 Mar 2016 11:19:12 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2016-03-23 09:51, Olivier Matz:
> > On 03/22/2016 11:09 PM, Stephen Hemminger wrote:
> > > Normal usage of rte_eth_dev_xstats_get is to call twice. The
> > > first time the function is called with portid, xstats = NULL
> > > and n = 0; this returns the number of entries in the statistics
> > > table that need to be allocated.
> > > 
> > > The problem is that the routine adds a count value to NULL (0)
> > > and assumes that this is a valid pointer (it isn't). Device drivers
> > > all have a check for NULL, and this no longer matches.
> 
> This check for NULL should be done after the check (n == 0).
> 
> > > Bug was introduced by commit d4fef8b0d5e5
> > > ("ethdev: expose generic and driver specific stats in xstats")
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> [...]
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1495,8 +1495,9 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
> > >  		/* Retrieve the xstats from the driver at the end of the
> > >  		 * xstats struct.
> > >  		 */
> > > -		xcount = (*dev->dev_ops->xstats_get)(dev, &xstats[count],
> > > -			 (n > count) ? n - count : 0);
> > > +		xcount = (*dev->dev_ops->xstats_get)(dev,
> > > +				     xstats ? xstats + count : NULL,
> > > +				     (n > count) ? n - count : 0);
> > >  
> > >  		if (xcount < 0)
> > >  			return xcount;
> > 
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> > 
> > Just one comment: I think the driver should not rely on the pointer
> > value, but on the count. From the documentation of rte_eth_xstats_get(),
> > the function returns a:
> > 
> >   "positive value higher than n: error, the given statistics table
> >    is too small. The return value corresponds to the size that should
> >    be given to succeed. The entries in the table are not valid and
> >    shall not be used by the caller."
> > 
> > So maybe it should be also fixed in the driver you are talking about.
> 
> In all known drivers, n is checked before xstats pointer.
> So there is no issue but the patch is still valid for unknown drivers.
> 
> Applied, thanks

Thanks, yes I was testing with a still not yet upstreamed driver.