Re: [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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