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 at 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 at 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
