From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Wed, 23 Mar 2016 17:31:24 +0100 (CET) Received: by mail-pf0-f176.google.com with SMTP id x3so34727510pfb.1 for ; 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 To: Thomas Monjalon Cc: dev@dpdk.org, Olivier Matz 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Mar 2016 16:31:24 -0000 On Wed, 23 Mar 2016 11:19:12 +0100 Thomas Monjalon 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 > [...] > > > --- 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. > > 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.