From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL
Date: Wed, 23 Mar 2016 11:19:12 +0100
Message-ID: <2833753.Klr9nq0nAo@xps13>
References: <1458684557-15378-1-git-send-email-stephen@networkplumber.org>
 <56F25925.60405@6wind.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, Olivier Matz <olivier.matz@6wind.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54])
 by dpdk.org (Postfix) with ESMTP id 9E80C29B6
 for <dev@dpdk.org>; Wed, 23 Mar 2016 11:20:54 +0100 (CET)
Received: by mail-wm0-f54.google.com with SMTP id l68so188699742wml.0
 for <dev@dpdk.org>; Wed, 23 Mar 2016 03:20:54 -0700 (PDT)
In-Reply-To: <56F25925.60405@6wind.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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