From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL Date: Wed, 23 Mar 2016 09:51:49 +0100 Message-ID: <56F25925.60405@6wind.com> References: <1458684557-15378-1-git-send-email-stephen@networkplumber.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit To: Stephen Hemminger , dev@dpdk.org Return-path: Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 976B62949 for ; Wed, 23 Mar 2016 09:51:51 +0100 (CET) Received: by mail-wm0-f49.google.com with SMTP id l68so13877684wml.1 for ; Wed, 23 Mar 2016 01:51:51 -0700 (PDT) In-Reply-To: <1458684557-15378-1-git-send-email-stephen@networkplumber.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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 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