On Wed, 23 Mar 2016 11:19:12 +0100
Thomas Monjalon <thomas.monjalon at 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 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
Thanks, yes I was testing with a still not yet upstreamed driver.
