diff mbox

[dpdk-dev,RFC,2/9] ethdev: move queue id check in generic layer

Message ID 1479981261-19512-3-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
checkpatch/checkpatch success coding style OK

Commit Message

Olivier Matz Nov. 24, 2016, 9:54 a.m. UTC 
  The check of queue_id is done in all drivers implementing
rte_eth_rx_queue_count(). Factorize this check in the generic function.

Note that the nfp driver was doing the check differently, which could
induce crashes if the queue index was too big.

By the way, also move the is_supported test before the port valid and
queue valid test.

PR=52423
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Ivan Boule <ivan.boule@6wind.com>
---
 drivers/net/e1000/em_rxtx.c    | 5 -----
 drivers/net/e1000/igb_rxtx.c   | 5 -----
 drivers/net/i40e/i40e_rxtx.c   | 5 -----
 drivers/net/ixgbe/ixgbe_rxtx.c | 5 -----
 drivers/net/nfp/nfp_net.c      | 6 ------
 lib/librte_ether/rte_ethdev.h  | 6 ++++--
 6 files changed, 4 insertions(+), 28 deletions(-)

Comments

Ferruh Yigit Nov. 24, 2016, 10:59 a.m. UTC | #1 
On 11/24/2016 9:54 AM, Olivier Matz wrote:
> The check of queue_id is done in all drivers implementing
> rte_eth_rx_queue_count(). Factorize this check in the generic function.
> 
> Note that the nfp driver was doing the check differently, which could
> induce crashes if the queue index was too big.
> 
> By the way, also move the is_supported test before the port valid and
> queue valid test.
> 
> PR=52423
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Acked-by: Ivan Boule <ivan.boule@6wind.com>
> ---

<...>

> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index c3edc23..9551cfd 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2693,7 +2693,7 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>   *  The queue id on the specific port.
>   * @return
>   *  The number of used descriptors in the specific queue, or:
> - *     (-EINVAL) if *port_id* is invalid
> + *     (-EINVAL) if *port_id* or *queue_id* is invalid
>   *     (-ENOTSUP) if the device does not support this function
>   */
>  static inline int
> @@ -2701,8 +2701,10 @@ rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, -ENOTSUP);
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

Doing port validity check before accessing dev->dev_ops->rx_queue_count
can be good idea.

What about validating port_id even before accessing
rte_eth_devices[port_id]?

> +	if (queue_id >= dev->data->nb_rx_queues)
> +		return -EINVAL;
>  
>  	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
>  }
>
Olivier Matz Nov. 24, 2016, 1:05 p.m. UTC | #2 
Hi Ferruh,

On Thu, 2016-11-24 at 10:59 +0000, Ferruh Yigit wrote:
> On 11/24/2016 9:54 AM, Olivier Matz wrote:
> > The check of queue_id is done in all drivers implementing
> > rte_eth_rx_queue_count(). Factorize this check in the generic
> > function.
> > 
> > Note that the nfp driver was doing the check differently, which
> > could
> > induce crashes if the queue index was too big.
> > 
> > By the way, also move the is_supported test before the port valid
> > and
> > queue valid test.
> > 
> > PR=52423
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > Acked-by: Ivan Boule <ivan.boule@6wind.com>
> > ---
> 
> <...>
> 
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h
> > index c3edc23..9551cfd 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2693,7 +2693,7 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t
> > queue_id,
> >   *  The queue id on the specific port.
> >   * @return
> >   *  The number of used descriptors in the specific queue, or:
> > - *     (-EINVAL) if *port_id* is invalid
> > + *     (-EINVAL) if *port_id* or *queue_id* is invalid
> >   *     (-ENOTSUP) if the device does not support this function
> >   */
> >  static inline int
> > @@ -2701,8 +2701,10 @@ rte_eth_rx_queue_count(uint8_t port_id,
> > uint16_t queue_id)
> >  {
> >  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >  
> > -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count,
> > -ENOTSUP);
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> 
> Doing port validity check before accessing dev->dev_ops-
> >rx_queue_count
> can be good idea.
> 
> What about validating port_id even before accessing
> rte_eth_devices[port_id]?
> 

oops right, we should not move this line, it's stupid...

Thanks for the feedback,
Olivier
diff mbox

Patch

diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
index 41f51c0..c1c724b 100644
--- a/drivers/net/e1000/em_rxtx.c
+++ b/drivers/net/e1000/em_rxtx.c
@@ -1390,11 +1390,6 @@  eth_em_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct em_rx_queue *rxq;
 	uint32_t desc = 0;
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		PMD_RX_LOG(DEBUG, "Invalid RX queue_id=%d", rx_queue_id);
-		return 0;
-	}
-
 	rxq = dev->data->rx_queues[rx_queue_id];
 	rxdp = &(rxq->rx_ring[rxq->rx_tail]);
 
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index dbd37ac..e9aa356 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1512,11 +1512,6 @@  eth_igb_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct igb_rx_queue *rxq;
 	uint32_t desc = 0;
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		PMD_RX_LOG(ERR, "Invalid RX queue id=%d", rx_queue_id);
-		return 0;
-	}
-
 	rxq = dev->data->rx_queues[rx_queue_id];
 	rxdp = &(rxq->rx_ring[rxq->rx_tail]);
 
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 7ae7d9f..79a72f0 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1793,11 +1793,6 @@  i40e_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct i40e_rx_queue *rxq;
 	uint16_t desc = 0;
 
-	if (unlikely(rx_queue_id >= dev->data->nb_rx_queues)) {
-		PMD_DRV_LOG(ERR, "Invalid RX queue id %u", rx_queue_id);
-		return 0;
-	}
-
 	rxq = dev->data->rx_queues[rx_queue_id];
 	rxdp = &(rxq->rx_ring[rxq->rx_tail]);
 	while ((desc < rxq->nb_rx_desc) &&
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index b2d9f45..1a8ea5f 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2857,11 +2857,6 @@  ixgbe_dev_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 	struct ixgbe_rx_queue *rxq;
 	uint32_t desc = 0;
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		PMD_RX_LOG(ERR, "Invalid RX queue id=%d", rx_queue_id);
-		return 0;
-	}
-
 	rxq = dev->data->rx_queues[rx_queue_id];
 	rxdp = &(rxq->rx_ring[rxq->rx_tail]);
 
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index e315dd8..f1d00fb 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -1084,12 +1084,6 @@  nfp_net_rx_queue_count(struct rte_eth_dev *dev, uint16_t queue_idx)
 	uint32_t count;
 
 	rxq = (struct nfp_net_rxq *)dev->data->rx_queues[queue_idx];
-
-	if (rxq == NULL) {
-		PMD_INIT_LOG(ERR, "Bad queue: %u\n", queue_idx);
-		return 0;
-	}
-
 	idx = rxq->rd_p % rxq->rx_count;
 	rxds = &rxq->rxds[idx];
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index c3edc23..9551cfd 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2693,7 +2693,7 @@  rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
  *  The queue id on the specific port.
  * @return
  *  The number of used descriptors in the specific queue, or:
- *     (-EINVAL) if *port_id* is invalid
+ *     (-EINVAL) if *port_id* or *queue_id* is invalid
  *     (-ENOTSUP) if the device does not support this function
  */
 static inline int
@@ -2701,8 +2701,10 @@  rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, -ENOTSUP);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	if (queue_id >= dev->data->nb_rx_queues)
+		return -EINVAL;
 
 	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
 }