diff mbox series

ethdev: check if queue setupped in queue-related APIs

Message ID 20201010071212.24086-1-huwei013@chinasoftinc.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: check if queue setupped in queue-related APIs |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed

Commit Message

Wei Hu (Xavier) Oct. 10, 2020, 7:12 a.m. UTC 
  From: Chengchang Tang <tangchengchang@huawei.com>

This patch adds checking whether the related Tx or Rx queue has been
setupped in the queue-related API functions to avoid illegal address
access. And validity check of the queue_id is also added in the API
functions rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable.

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/librte_ethdev/rte_ethdev.c | 56 ++++++++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h |  3 ++-
 2 files changed, 58 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger Oct. 10, 2020, 3:24 p.m. UTC | #1 
On Sat, 10 Oct 2020 15:12:12 +0800
"Wei Hu (Xavier)" <huwei013@chinasoftinc.com> wrote:

> +	if (dev->data->rx_queues[rx_queue_id] == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device with port_id=%"
> +				    PRIu16" has not been setupped\n",
> +				    rx_queue_id, port_id);
> +		return -EINVAL;
> +

Please use correct spelling.

Your change follows the existing style in rte_eth_dev_rx_queue_start() but
my preference is that message strings are not split across
lines. That makes it easier to use tools like grep to find messages in the source.

Use of PRIu16 is not required. And recent compiler standards would require space
around its use.

Suggest:
		RTE_ETHDEV_LOG(ERR, 
			       "Queue %u of device with port_id=%u has not been setup\n",
				rx_queue_id, port_id);
Kalesh A P Oct. 10, 2020, 4:38 p.m. UTC | #2 
On Sat, Oct 10, 2020 at 12:42 PM Wei Hu (Xavier) <huwei013@chinasoftinc.com>
wrote:

> From: Chengchang Tang <tangchengchang@huawei.com>
>
> This patch adds checking whether the related Tx or Rx queue has been
> setupped in the queue-related API functions to avoid illegal address
> access. And validity check of the queue_id is also added in the API
> functions rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable.
>
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 56
> ++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h |  3 ++-
>  2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c
> b/lib/librte_ethdev/rte_ethdev.c
> index 892c246..31a8eb3 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -897,6 +897,13 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t
> rx_queue_id)
>                 return -EINVAL;
>         }
>
> +       if (dev->data->rx_queues[rx_queue_id] == NULL) {
> +               RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device with
> port_id=%"
> +                                   PRIu16" has not been setupped\n",
> +                                   rx_queue_id, port_id);
> +               return -EINVAL;
> +       }
> +
>

Hi Xavier,

How about having two common functions which validate RXQ/TXQ ids and
whether it has been set up or not like below. This helps avoiding lot of
duplicate code:

static inline int
rte_eth_dev_validate_rx_queue(uint16_t port_id, uint16_t rx_queue_id)
{
        struct rte_eth_dev *dev;

        RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

        dev = &rte_eth_devices[port_id];

        if (rx_queue_id >= dev->data->nb_rx_queues) {
                RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
rx_queue_id);
                return -EINVAL;
        }

       if (dev->data->rx_queues[rx_queue_id] == NULL) {
               RTE_ETHDEV_LOG(ERR,
                              "Queue %u of device with port_id=%u has not
been setup\n",
                              rx_queue_id, port_id);
               return -EINVAL;
       }

       return 0;
}

Regards,
Kalesh

> --
> 2.9.5
>
>
Wei Hu (Xavier) Oct. 12, 2020, 3:21 a.m. UTC | #3 
Hi, Stephen Hemminger

On 2020/10/10 23:24, Stephen Hemminger wrote:
> On Sat, 10 Oct 2020 15:12:12 +0800
> "Wei Hu (Xavier)" <huwei013@chinasoftinc.com> wrote:
>
>> +	if (dev->data->rx_queues[rx_queue_id] == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device with port_id=%"
>> +				    PRIu16" has not been setupped\n",
>> +				    rx_queue_id, port_id);
>> +		return -EINVAL;
>> +
> Please use correct spelling.
>
> Your change follows the existing style in rte_eth_dev_rx_queue_start() but
> my preference is that message strings are not split across
> lines. That makes it easier to use tools like grep to find messages in the source.
>
> Use of PRIu16 is not required. And recent compiler standards would require space
> around its use.
>
> Suggest:
> 		RTE_ETHDEV_LOG(ERR,
> 			       "Queue %u of device with port_id=%u has not been setup\n",
> 				rx_queue_id, port_id);

I fixed it in V2.

Thanks, xavier
Wei Hu (Xavier) Oct. 12, 2020, 3:22 a.m. UTC | #4 
Hi, Kalesh Anakkur Purayil

On 2020/10/11 0:38, Kalesh Anakkur Purayil wrote:
>
>
> On Sat, Oct 10, 2020 at 12:42 PM Wei Hu (Xavier) 
> <huwei013@chinasoftinc.com <mailto:huwei013@chinasoftinc.com>> wrote:
>
>     From: Chengchang Tang <tangchengchang@huawei.com
>     <mailto:tangchengchang@huawei.com>>
>
>     This patch adds checking whether the related Tx or Rx queue has been
>     setupped in the queue-related API functions to avoid illegal address
>     access. And validity check of the queue_id is also added in the API
>     functions rte_eth_dev_rx_intr_enable and rte_eth_dev_rx_intr_disable.
>
>     Signed-off-by: Chengchang Tang <tangchengchang@huawei.com
>     <mailto:tangchengchang@huawei.com>>
>     Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com
>     <mailto:xavier.huwei@huawei.com>>
>     Signed-off-by: Chengwen Feng <fengchengwen@huawei.com
>     <mailto:fengchengwen@huawei.com>>
>     ---
>      lib/librte_ethdev/rte_ethdev.c | 56
>     ++++++++++++++++++++++++++++++++++++++++++
>      lib/librte_ethdev/rte_ethdev.h |  3 ++-
>      2 files changed, 58 insertions(+), 1 deletion(-)
>
>     diff --git a/lib/librte_ethdev/rte_ethdev.c
>     b/lib/librte_ethdev/rte_ethdev.c
>     index 892c246..31a8eb3 100644
>     --- a/lib/librte_ethdev/rte_ethdev.c
>     +++ b/lib/librte_ethdev/rte_ethdev.c
>     @@ -897,6 +897,13 @@ rte_eth_dev_rx_queue_start(uint16_t port_id,
>     uint16_t rx_queue_id)
>                     return -EINVAL;
>             }
>
>     +       if (dev->data->rx_queues[rx_queue_id] == NULL) {
>     +               RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device
>     with port_id=%"
>     +                                   PRIu16" has not been setupped\n",
>     +                                   rx_queue_id, port_id);
>     +               return -EINVAL;
>     +       }
>     +
>
>
> Hi Xavier,
>
> How about having two common functions which validate RXQ/TXQ ids and 
> whether it has been set up or not like below. This helps avoiding lot 
> of duplicate code:
>
> static inline int
> rte_eth_dev_validate_rx_queue(uint16_t port_id, uint16_t rx_queue_id)
> {
>         struct rte_eth_dev *dev;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
>         dev = &rte_eth_devices[port_id];
>
>         if (rx_queue_id >= dev->data->nb_rx_queues) {
>                 RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", 
> rx_queue_id);
>                 return -EINVAL;
>         }
>
>        if (dev->data->rx_queues[rx_queue_id] == NULL) {
>                RTE_ETHDEV_LOG(ERR,
>                               "Queue %u of device with port_id=%u has 
> not been setup\n",
>                               rx_queue_id, port_id);
>                return -EINVAL;
>        }
>
>        return 0;
> }
>
I fixed it in V2.

Thanks, xavier

> Regards,
> Kalesh
>
>     -- 
>     2.9.5
>
>
>
> -- 
> Regards,
> Kalesh A P
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 892c246..31a8eb3 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -897,6 +897,13 @@  rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->rx_queues[rx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device with port_id=%"
+				    PRIu16" has not been setupped\n",
+				    rx_queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
 
 	if (rte_eth_dev_is_rx_hairpin_queue(dev, rx_queue_id)) {
@@ -931,6 +938,13 @@  rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->rx_queues[rx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Rx queue %"PRIu16" of device with port_id=%"
+				    PRIu16" has not been setupped\n",
+				    rx_queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
 
 	if (rte_eth_dev_is_rx_hairpin_queue(dev, rx_queue_id)) {
@@ -971,6 +985,13 @@  rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->rx_queues[tx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Tx queue %"PRIu16" of device with port_id=%"
+				    PRIu16" has not been setupped\n",
+				    tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
 
 	if (rte_eth_dev_is_tx_hairpin_queue(dev, tx_queue_id)) {
@@ -1003,6 +1024,13 @@  rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->rx_queues[tx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Tx queue %"PRIu16" of device with port_id=%"
+				    PRIu16" has not been setupped\n",
+				    tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
 
 	if (rte_eth_dev_is_tx_hairpin_queue(dev, tx_queue_id)) {
@@ -4463,6 +4491,20 @@  rte_eth_dev_rx_intr_enable(uint16_t port_id,
 
 	dev = &rte_eth_devices[port_id];
 
+	if (queue_id >= dev->data->nb_rx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%"PRIu16"\n",
+			       queue_id);
+		return -EINVAL;
+	}
+
+	if (dev->data->rx_queues[queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR,
+			       "Rx queue %"PRIu16" of device with port_id=%"
+			       PRIu16" has not been setupped\n",
+			       queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_intr_enable, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rx_queue_intr_enable)(dev,
 								queue_id));
@@ -4478,6 +4520,20 @@  rte_eth_dev_rx_intr_disable(uint16_t port_id,
 
 	dev = &rte_eth_devices[port_id];
 
+	if (queue_id >= dev->data->nb_rx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%"PRIu16"\n",
+			       queue_id);
+		return -EINVAL;
+	}
+
+	if (dev->data->rx_queues[queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR,
+			       "Rx queue %"PRIu16" of device with port_id=%"
+			       PRIu16" has not been setupped\n",
+			       queue_id, port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_intr_disable, -ENOTSUP);
 	return eth_err(port_id, (*dev->dev_ops->rx_queue_intr_disable)(dev,
 								queue_id));
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 5bcfbb8..f4cc591 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4721,7 +4721,8 @@  rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_queue_count, -ENOTSUP);
-	if (queue_id >= dev->data->nb_rx_queues)
+	if (queue_id >= dev->data->nb_rx_queues ||
+	    dev->data->rx_queues[queue_id] == NULL)
 		return -EINVAL;
 
 	return (int)(*dev->rx_queue_count)(dev, queue_id);