[dpdk-dev] librte_ether: fix crashes in rte_ethdev functions.

  The nb_rx_queues and nb_tx_queues are initialised before
the tx_queue and rx_queue arrays are allocated. The arrays
are allocated when the ethdev port is started.

If any of the following functions are called before the ethdev
port is started there is a segmentation fault:

rte_eth_stats_get
rte_eth_stats_reset
rte_eth_xstats_get
rte_eth_xstats_reset

Fixes: af75078fece3 ("first public release")
Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats in xstats")
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
  

On 2015/12/18 1:24, Bernard Iremonger wrote:
> The nb_rx_queues and nb_tx_queues are initialised before
> the tx_queue and rx_queue arrays are allocated. The arrays
> are allocated when the ethdev port is started.
>
> If any of the following functions are called before the ethdev
> port is started there is a segmentation fault:
>
> rte_eth_stats_get
> rte_eth_stats_reset
> rte_eth_xstats_get
> rte_eth_xstats_reset
>
> Fixes: af75078fece3 ("first public release")
> Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended statistics")
> Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats in xstats")
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ed971b4..a0ee84d 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)
>  	memset(stats, 0, sizeof(*stats));
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
> -	(*dev->dev_ops->stats_get)(dev, stats);
> +
> +	if (dev->data->dev_started)
> +		(*dev->dev_ops->stats_get)(dev, stats);
> +

My question is should we mark an error or a warning here and return an
error so that the caller knows what happens?

Thanks,
Michael

>  	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
>  	return 0;
>  }
> @@ -1455,7 +1458,10 @@ rte_eth_stats_reset(uint8_t port_id)
>  	dev = &rte_eth_devices[port_id];
>  
>  	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset);
> -	(*dev->dev_ops->stats_reset)(dev);
> +
> +	if (dev->data->dev_started)
> +		(*dev->dev_ops->stats_reset)(dev);
> +
>  	dev->data->rx_mbuf_alloc_failed = 0;
>  }
>  
> @@ -1479,7 +1485,8 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
>  		(dev->data->nb_tx_queues * RTE_NB_TXQ_STATS);
>  
>  	/* implemented by the driver */
> -	if (dev->dev_ops->xstats_get != NULL) {
> +	if ((dev->dev_ops->xstats_get != NULL) &&
> +		(dev->data->dev_started)) {
>  		/* Retrieve the xstats from the driver at the end of the
>  		 * xstats struct.
>  		 */
> @@ -1548,7 +1555,8 @@ rte_eth_xstats_reset(uint8_t port_id)
>  	dev = &rte_eth_devices[port_id];
>  
>  	/* implemented by the driver */
> -	if (dev->dev_ops->xstats_reset != NULL) {
> +	if ((dev->dev_ops->xstats_reset != NULL) &&
> +		(dev->data->dev_started)) {
>  		(*dev->dev_ops->xstats_reset)(dev);
>  		return;
>  	}
  

Hi Michael,

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, December 21, 2015 9:03 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev
> functions.
> 
> On 2015/12/18 1:24, Bernard Iremonger wrote:
> > The nb_rx_queues and nb_tx_queues are initialised before the tx_queue
> > and rx_queue arrays are allocated. The arrays are allocated when the
> > ethdev port is started.
> >
> > If any of the following functions are called before the ethdev port is
> > started there is a segmentation fault:
> >
> > rte_eth_stats_get
> > rte_eth_stats_reset
> > rte_eth_xstats_get
> > rte_eth_xstats_reset
> >
> > Fixes: af75078fece3 ("first public release")
> > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended
> > statistics")
> > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats
> > in xstats")
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct
> rte_eth_stats *stats)
> >  	memset(stats, 0, sizeof(*stats));
> >
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -
> ENOTSUP);
> > -	(*dev->dev_ops->stats_get)(dev, stats);
> > +
> > +	if (dev->data->dev_started)
> > +		(*dev->dev_ops->stats_get)(dev, stats);
> > +
> 
> My question is should we mark an error or a warning here and return an
> error so that the caller knows what happens?
> 
> Thanks,
> Michael

<snip>

In other cases in rte_ethdev.c where there is a check on "dev->data->dev_started" there is a RTE_PMD_DEBUG_TRACE()  line. I will add a RTE_PMD_DEBUG_TRACE()  line.
The rte_eth_stats_reset() and rte_eth_xstats_reset() functions return void.
Not sure if an error is required for the rte_eth_stats_get() and rte_eth_xstats_get() functions as the stats information returned is all zero's at present.

Regards,

Bernard.
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Monday, December 21, 2015 11:40 AM
> To: Qiu, Michael; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev functions.
> 
> Hi Michael,
> 
> > -----Original Message-----
> > From: Qiu, Michael
> > Sent: Monday, December 21, 2015 9:03 AM
> > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev
> > functions.
> >
> > On 2015/12/18 1:24, Bernard Iremonger wrote:
> > > The nb_rx_queues and nb_tx_queues are initialised before the tx_queue
> > > and rx_queue arrays are allocated. The arrays are allocated when the
> > > ethdev port is started.
> > >
> > > If any of the following functions are called before the ethdev port is
> > > started there is a segmentation fault:
> > >
> > > rte_eth_stats_get
> > > rte_eth_stats_reset
> > > rte_eth_xstats_get
> > > rte_eth_xstats_reset
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended
> > > statistics")
> > > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific stats
> > > in xstats")
> > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct
> > rte_eth_stats *stats)
> > >  	memset(stats, 0, sizeof(*stats));
> > >
> > >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -
> > ENOTSUP);
> > > -	(*dev->dev_ops->stats_get)(dev, stats);
> > > +
> > > +	if (dev->data->dev_started)
> > > +		(*dev->dev_ops->stats_get)(dev, stats);
> > > +

So why it would be no possible now to get statistics on the stopped device?
Konstantin

> >
> > My question is should we mark an error or a warning here and return an
> > error so that the caller knows what happens?
> >
> > Thanks,
> > Michael
> 
> <snip>
> 
> In other cases in rte_ethdev.c where there is a check on "dev->data->dev_started" there is a RTE_PMD_DEBUG_TRACE()  line. I will
> add a RTE_PMD_DEBUG_TRACE()  line.
> The rte_eth_stats_reset() and rte_eth_xstats_reset() functions return void.
> Not sure if an error is required for the rte_eth_stats_get() and rte_eth_xstats_get() functions as the stats information returned is all
> zero's at present.
> 
> Regards,
> 
> Bernard.
  

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, December 21, 2015 12:02 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Qiu, Michael
> <michael.qiu@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev
> functions.
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger,
> > Bernard
> > Sent: Monday, December 21, 2015 11:40 AM
> > To: Qiu, Michael; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in rte_ethdev
> functions.
> >
> > Hi Michael,
> >
> > > -----Original Message-----
> > > From: Qiu, Michael
> > > Sent: Monday, December 21, 2015 9:03 AM
> > > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] librte_ether: fix crashes in
> > > rte_ethdev functions.
> > >
> > > On 2015/12/18 1:24, Bernard Iremonger wrote:
> > > > The nb_rx_queues and nb_tx_queues are initialised before the
> > > > tx_queue and rx_queue arrays are allocated. The arrays are
> > > > allocated when the ethdev port is started.
> > > >
> > > > If any of the following functions are called before the ethdev
> > > > port is started there is a segmentation fault:
> > > >
> > > > rte_eth_stats_get
> > > > rte_eth_stats_reset
> > > > rte_eth_xstats_get
> > > > rte_eth_xstats_reset
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Fixes: ce757f5c9a4d ("ethdev: new method to retrieve extended
> > > > statistics")
> > > > Fixes: d4fef8b0d5e5 ("ethdev: expose generic and driver specific
> > > > stats in xstats")
> > > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > > > ---
> > > >  lib/librte_ether/rte_ethdev.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > > b/lib/librte_ether/rte_ethdev.c index ed971b4..a0ee84d 100644
> > > > --- a/lib/librte_ether/rte_ethdev.c
> > > > +++ b/lib/librte_ether/rte_ethdev.c
> > > > @@ -1441,7 +1441,10 @@ rte_eth_stats_get(uint8_t port_id, struct
> > > rte_eth_stats *stats)
> > > >  	memset(stats, 0, sizeof(*stats));
> > > >
> > > >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -
> > > ENOTSUP);
> > > > -	(*dev->dev_ops->stats_get)(dev, stats);
> > > > +
> > > > +	if (dev->data->dev_started)
> > > > +		(*dev->dev_ops->stats_get)(dev, stats);
> > > > +
> 
> So why it would be no possible now to get statistics on the stopped device?
> Konstantin

<snip>

I did not consider this scenario.
I need to rethink this patch.

Self NAK

Regards,

Bernard.