diff mbox

[dpdk-dev] ethdev: fix rte_eth_dev_xstats_get with NULL

Message ID 1458684557-15378-1-git-send-email-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Stephen Hemminger March 22, 2016, 10:09 p.m. UTC 
  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(-)

Comments

Olivier Matz March 23, 2016, 8:51 a.m. UTC | #1 
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
Thomas Monjalon March 23, 2016, 10:19 a.m. UTC | #2 
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
Stephen Hemminger March 23, 2016, 4:31 p.m. UTC | #3 
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.
diff mbox

Patch

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;