Re: [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL

[Olivier Matz <olivier.matz@6wind.com>]

Hi Stephen,

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.
> 
> Bug was introduced by commit d4fef8b0d5e5
> ("ethdev: expose generic and driver specific stats in xstats")
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_ether/rte_ethdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index db35102..8721a6b 100644
> --- 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.


Thanks,
Olivier