From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stephen@networkplumber.org>
Received: from mail-pf0-f176.google.com (mail-pf0-f176.google.com
[209.85.192.176]) by dpdk.org (Postfix) with ESMTP id 33CCE2BDD
for <dev@dpdk.org>; Wed, 23 Mar 2016 17:31:24 +0100 (CET)
Received: by mail-pf0-f176.google.com with SMTP id x3so34727510pfb.1
for <dev@dpdk.org>; Wed, 23 Mar 2016 09:31:24 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=networkplumber-org.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:in-reply-to:references
:mime-version:content-transfer-encoding;
bh=rjuQjiqy//4uZxG88noL3jtZnW6+RcIC2FkTPfbtfhY=;
b=APLoz/bQD0RpqZX2u8H5Ogm1TihrxDu1D8A82qdO4MtKcKGS0u7sWtHN7f7R7unIN4
PSsQBr2wA666AwoOy0/qtVegPfNvHBKoctOxOcruH4ePTO0D9wKCzweZujt+OcTG0wPH
xdINJ8CbMNAGDkFO74X9qc+uzSFNlwhNr3U1RhtwRF3VbNrJc4+21MBnRVLtwUQRKE2J
4lSw8KCF5G9IY19dm/l9b3KD1vyq4unUC3IUBxUVuYGFB5R57xHU6dTu3LLZTkdo/2FB
+Nt1H+M+K0aCuiyCe4eGr8PbxzR6Skv2PlS7UiZ4DTulNUcLALbGxqHC2SwAj2XYSlD6
ZUNQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20130820;
h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to
:references:mime-version:content-transfer-encoding;
bh=rjuQjiqy//4uZxG88noL3jtZnW6+RcIC2FkTPfbtfhY=;
b=G+VlK9/ruzPHzzJroAcfxYgqFS9P+ERy2UPe5cvBLvhD/8HZDgtM4OhjQ2aSTIgB/5
Kw5RG+gNlIyuyOqiE+r7GIqKiY1U/nf6nTZ1+a7enrC8rpzuNGf32ldoF+tOkFaVNm5G
6fJf/edCd43JJ4LREcdzRQho9xsWe0fWWrTItjQmRz2omo6pAPKAavxyEKXBIBQtkrNU
8hvdcwuJ7NF4h20gaqeEWe94w46k7Yrg7fz1rD6k465c7bdaBV/swZ5GLdD6IXkgohyR
kphGdbCoRXP9TSZfwUJuHOgw/1wVdYNWc/V6InPZotydOoYh8QRX+tGxMuH1pFss1gGm
Wgsw==
X-Gm-Message-State: AD7BkJK8pfeAVhoTYJgrjNn/fCL34DCzV3h2IaEs2CHPZlJzRkGD0PVN2J7soSbbHUNxZQ==
X-Received: by 10.66.248.198 with SMTP id yo6mr5558355pac.54.1458750683586;
Wed, 23 Mar 2016 09:31:23 -0700 (PDT)
Received: from xeon-e3 (static-50-53-65-230.bvtn.or.frontiernet.net.
[50.53.65.230])
by smtp.gmail.com with ESMTPSA id o185sm5314271pfo.36.2016.03.23.09.31.23
(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Wed, 23 Mar 2016 09:31:23 -0700 (PDT)
Date: Wed, 23 Mar 2016 09:31:39 -0700
From: Stephen Hemminger <stephen@networkplumber.org>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: dev@dpdk.org, Olivier Matz <olivier.matz@6wind.com>
Message-ID: <20160323093139.2b990787@xeon-e3>
In-Reply-To: <2833753.Klr9nq0nAo@xps13>
References: <1458684557-15378-1-git-send-email-stephen@networkplumber.org>
<56F25925.60405@6wind.com> <2833753.Klr9nq0nAo@xps13>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH] ethdev: fix rte_eth_dev_xstats_get with NULL
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
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>
X-List-Received-Date: Wed, 23 Mar 2016 16:31:24 -0000
On Wed, 23 Mar 2016 11:19:12 +0100
Thomas Monjalon <thomas.monjalon@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@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
Thanks, yes I was testing with a still not yet upstreamed driver.