[dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[Wei Hu (Xavier)]

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;
+	}
+
 	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);
-- 
2.9.5

Re: [dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[Stephen Hemminger]

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);

Re: [dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[Kalesh Anakkur Purayil]

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
>
>

-- 
Regards,
Kalesh A P

[dpdk-dev] [PATCH v2] ethdev: check if queue setupped in queue-related APIs

[Wei Hu (Xavier)]

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>
---
v1 -> v2:
	 1. replace %"PRIu16" with %u.
	 2. extact two common functions which validate RXQ/TXQ ids and
	    whether it has been set up or not.
---
 lib/librte_ethdev/rte_ethdev.c | 92 ++++++++++++++++++++++++++++++++++--------
 lib/librte_ethdev/rte_ethdev.h |  3 +-
 2 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 892c246..cb55863 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -877,10 +877,59 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 	return 0;
 }
 
+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;
+}
+
+static inline int
+rte_eth_dev_validate_tx_queue(uint16_t port_id, uint16_t tx_queue_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+
+	dev = &rte_eth_devices[port_id];
+
+	if (tx_queue_id >= dev->data->nb_tx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
+		return -EINVAL;
+	}
+
+	if (dev->data->tx_queues[tx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR,
+			       "Queue %u of device with port_id=%u has not been"
+			       " setup\n", tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -892,10 +941,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
-		return -EINVAL;
-	}
+	ret = rte_eth_dev_validate_rx_queue(port_id, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
 
@@ -922,14 +970,15 @@ int
 rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	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;
-	}
+
+	ret = rte_eth_dev_validate_rx_queue(port_id, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
 
@@ -955,6 +1004,7 @@ int
 rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -966,10 +1016,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+	ret = rte_eth_dev_validate_tx_queue(port_id, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
 
@@ -994,14 +1043,15 @@ int
 rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+
+	ret = rte_eth_dev_validate_tx_queue(port_id, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
 
@@ -4458,11 +4508,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
 			   uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_dev_validate_rx_queue(port_id, queue_id);
+	if (ret)
+		return ret;
+
 	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));
@@ -4473,11 +4528,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
 			    uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_dev_validate_rx_queue(port_id, queue_id);
+	if (ret)
+		return ret;
+
 	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);
-- 
2.9.5

Re: [dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[Wei Hu (Xavier)]

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

Re: [dpdk-dev] [PATCH] ethdev: check if queue setupped in queue-related APIs

[Wei Hu (Xavier)]

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

Re: [dpdk-dev] [PATCH v2] ethdev: check if queue setupped in queue-related APIs

[Stephen Hemminger]

On Mon, 12 Oct 2020 11:19:07 +0800
"Wei Hu (Xavier)" <huwei013@chinasoftinc.com> wrote:

> +		RTE_ETHDEV_LOG(ERR,
> +			       "Queue %u of device with port_id=%u has not been"
> +			       " setup\n", rx_queue_id, port_id);

Please do not break lines in format strings.
If checkpatch complains (which it shouldn't) ignore the warning.

[dpdk-dev] [PATCH v3] ethdev: check if queue setupped in queue-related APIs

[Wei Hu (Xavier)]

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>
---
v2 -> v3:
	 don't break lines in format strings.
v1 -> v2:
	 1. replace %"PRIu16" with %u.
	 2. extact two common functions which validate RXQ/TXQ ids and
	    whether it has been set up or not.
---
 lib/librte_ethdev/rte_ethdev.c | 92 ++++++++++++++++++++++++++++++++++--------
 lib/librte_ethdev/rte_ethdev.h |  3 +-
 2 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 892c246..34eec97 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -877,10 +877,59 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 	return 0;
 }
 
+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;
+}
+
+static inline int
+rte_eth_dev_validate_tx_queue(uint16_t port_id, uint16_t tx_queue_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+
+	dev = &rte_eth_devices[port_id];
+
+	if (tx_queue_id >= dev->data->nb_tx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
+		return -EINVAL;
+	}
+
+	if (dev->data->tx_queues[tx_queue_id] == NULL) {
+		RTE_ETHDEV_LOG(ERR,
+			       "Queue %u of device with port_id=%u has not been setup\n",
+			       tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -892,10 +941,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
-		return -EINVAL;
-	}
+	ret = rte_eth_dev_validate_rx_queue(port_id, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
 
@@ -922,14 +970,15 @@ int
 rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	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;
-	}
+
+	ret = rte_eth_dev_validate_rx_queue(port_id, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
 
@@ -955,6 +1004,7 @@ int
 rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -966,10 +1016,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+	ret = rte_eth_dev_validate_tx_queue(port_id, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
 
@@ -994,14 +1043,15 @@ int
 rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+
+	ret = rte_eth_dev_validate_tx_queue(port_id, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
 
@@ -4458,11 +4508,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
 			   uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_dev_validate_rx_queue(port_id, queue_id);
+	if (ret)
+		return ret;
+
 	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));
@@ -4473,11 +4528,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
 			    uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = rte_eth_dev_validate_rx_queue(port_id, queue_id);
+	if (ret)
+		return ret;
+
 	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);
-- 
2.9.5

Re: [dpdk-dev] [PATCH v3] ethdev: check if queue setupped in queue-related APIs

[Ferruh Yigit]

On 10/12/2020 8:32 AM, Wei Hu (Xavier) 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>
> ---
> v2 -> v3:
> 	 don't break lines in format strings.
> v1 -> v2:
> 	 1. replace %"PRIu16" with %u.
> 	 2. extact two common functions which validate RXQ/TXQ ids and
> 	    whether it has been set up or not.
> ---
>   lib/librte_ethdev/rte_ethdev.c | 92 ++++++++++++++++++++++++++++++++++--------
>   lib/librte_ethdev/rte_ethdev.h |  3 +-
>   2 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 892c246..34eec97 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -877,10 +877,59 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>   	return 0;
>   }
>   
> +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];
> +

Since these are static (internal) functions, why not get "struct rte_eth_dev 
*dev" as parameter and drop the port_id validation?

Also for same reason, although there is not convention around it, what do you 
think dropping the 'rte_' prefix from the funcitons names, to prevent them 
confused by ethdev APIs?

<...>

> @@ -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;

This will bring additional check for the datapath, but I guess this check is 
reasonable since this is an API.

Re: [dpdk-dev] [PATCH v3] ethdev: check if queue setupped in queue-related APIs

[Stephen Hemminger]

On Mon, 12 Oct 2020 15:32:44 +0800
"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>

Looks good. If you make another version (or when committing)
Lets change the subject line and commit text to replace "setupped" 
since that is not an English word; the proper word would be setup.

https://www.quora.com/What-is-the-past-tense-of-to-set-up-What-are-some-examples?share=1

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[dpdk-dev] [PATCH v4] ethdev: check if queue setup in queue-related APIs

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

This patch adds checking whether the related Tx or Rx queue has been setup
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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
---
v3 -> v4:
	 1. dropping the 'rte_' prefix from the funcitons names.
	 2. get "struct rte_eth_dev *dev" as parameter and drop the port_id
	    validation in the function named eth_dev_validate_rx_queue and
	    eth_dev_validate_tx_queue.
	 3. replace "setupped" with "setup" in the commit log and titile.
v2 -> v3:
	 don't break lines in format strings.
v1 -> v2:
	 1. replace %"PRIu16" with %u.
	 2. extact two common functions which validate RXQ/TXQ ids and
	    whether it has been set up or not.
---
 lib/librte_ethdev/rte_ethdev.c | 86 ++++++++++++++++++++++++++++++++++--------
 lib/librte_ethdev/rte_ethdev.h |  3 +-
 2 files changed, 72 insertions(+), 17 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 892c246..04d30f9 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -877,10 +877,53 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 	return 0;
 }
 
+static inline int
+eth_dev_validate_rx_queue(struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+	uint16_t 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) {
+		port_id = dev->data->port_id;
+		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;
+}
+
+static inline int
+eth_dev_validate_tx_queue(struct rte_eth_dev *dev, uint16_t tx_queue_id)
+{
+	uint16_t port_id;
+
+	if (tx_queue_id >= dev->data->nb_tx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
+		return -EINVAL;
+	}
+
+	if (dev->data->tx_queues[tx_queue_id] == NULL) {
+		port_id = dev->data->port_id;
+		RTE_ETHDEV_LOG(ERR,
+			       "Queue %u of device with port_id=%u has not been setup\n",
+			       tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -892,10 +935,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
-		return -EINVAL;
-	}
+	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
 
@@ -922,14 +964,15 @@ int
 rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	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;
-	}
+
+	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
 
@@ -955,6 +998,7 @@ int
 rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -966,10 +1010,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
 
@@ -994,14 +1037,15 @@ int
 rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+
+	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
+	if (ret)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
 
@@ -4458,11 +4502,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
 			   uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = eth_dev_validate_rx_queue(dev, queue_id);
+	if (ret)
+		return ret;
+
 	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));
@@ -4473,11 +4522,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
 			    uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = eth_dev_validate_rx_queue(dev, queue_id);
+	if (ret)
+		return ret;
+
 	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);
-- 
2.9.5

Re: [dpdk-dev] [PATCH v4] ethdev: check if queue setup in queue-related APIs

[Kalesh Anakkur Purayil]

Hi Xavier,

Thanks for taking care of the comments. LGTM.

Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Regards,
Kalesh

On Tue, Oct 13, 2020 at 8:11 AM 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 setup
> 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>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 -> v4:
>          1. dropping the 'rte_' prefix from the funcitons names.
>          2. get "struct rte_eth_dev *dev" as parameter and drop the port_id
>             validation in the function named eth_dev_validate_rx_queue and
>             eth_dev_validate_tx_queue.
>          3. replace "setupped" with "setup" in the commit log and titile.
> v2 -> v3:
>          don't break lines in format strings.
> v1 -> v2:
>          1. replace %"PRIu16" with %u.
>          2. extact two common functions which validate RXQ/TXQ ids and
>             whether it has been set up or not.
> ---
>  lib/librte_ethdev/rte_ethdev.c | 86
> ++++++++++++++++++++++++++++++++++--------
>  lib/librte_ethdev/rte_ethdev.h |  3 +-
>  2 files changed, 72 insertions(+), 17 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c
> b/lib/librte_ethdev/rte_ethdev.c
> index 892c246..04d30f9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -877,10 +877,53 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev,
> uint16_t nb_queues)
>         return 0;
>  }
>
> +static inline int
> +eth_dev_validate_rx_queue(struct rte_eth_dev *dev, uint16_t rx_queue_id)
> +{
> +       uint16_t 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) {
> +               port_id = dev->data->port_id;
> +               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;
> +}
> +
> +static inline int
> +eth_dev_validate_tx_queue(struct rte_eth_dev *dev, uint16_t tx_queue_id)
> +{
> +       uint16_t port_id;
> +
> +       if (tx_queue_id >= dev->data->nb_tx_queues) {
> +               RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> tx_queue_id);
> +               return -EINVAL;
> +       }
> +
> +       if (dev->data->tx_queues[tx_queue_id] == NULL) {
> +               port_id = dev->data->port_id;
> +               RTE_ETHDEV_LOG(ERR,
> +                              "Queue %u of device with port_id=%u has not
> been setup\n",
> +                              tx_queue_id, port_id);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  int
>  rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
> @@ -892,10 +935,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t
> rx_queue_id)
>                 return -EINVAL;
>         }
>
> -       if (rx_queue_id >= dev->data->nb_rx_queues) {
> -               RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
> rx_queue_id);
> -               return -EINVAL;
> -       }
> +       ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
> +       if (ret)
> +               return ret;
>
>         RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
>
> @@ -922,14 +964,15 @@ int
>  rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         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;
> -       }
> +
> +       ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
> +       if (ret)
> +               return ret;
>
>         RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
>
> @@ -955,6 +998,7 @@ int
>  rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
> @@ -966,10 +1010,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id,
> uint16_t tx_queue_id)
>                 return -EINVAL;
>         }
>
> -       if (tx_queue_id >= dev->data->nb_tx_queues) {
> -               RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> tx_queue_id);
> -               return -EINVAL;
> -       }
> +       ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
> +       if (ret)
> +               return ret;
>
>         RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
>
> @@ -994,14 +1037,15 @@ int
>  rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
>         dev = &rte_eth_devices[port_id];
> -       if (tx_queue_id >= dev->data->nb_tx_queues) {
> -               RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> tx_queue_id);
> -               return -EINVAL;
> -       }
> +
> +       ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
> +       if (ret)
> +               return ret;
>
>         RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
>
> @@ -4458,11 +4502,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
>                            uint16_t queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
>         dev = &rte_eth_devices[port_id];
>
> +       ret = eth_dev_validate_rx_queue(dev, queue_id);
> +       if (ret)
> +               return ret;
> +
>         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));
> @@ -4473,11 +4522,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
>                             uint16_t queue_id)
>  {
>         struct rte_eth_dev *dev;
> +       int ret;
>
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
>         dev = &rte_eth_devices[port_id];
>
> +       ret = eth_dev_validate_rx_queue(dev, queue_id);
> +       if (ret)
> +               return ret;
> +
>         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);
> --
> 2.9.5
>
>

-- 
Regards,
Kalesh A P

Re: [dpdk-dev] [PATCH v4] ethdev: check if queue setup in queue-related APIs

[Andrew Rybchenko]

On 10/13/20 5:41 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> This patch adds checking whether the related Tx or Rx queue has been setup

"This patch adds checking" -> "Add checking"

> 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.

Thanks for the patch. LGTM with few minor notes.

Based on the "And ..." in the description I'd consider to split
it into two patches since it has few logical changes.
 * Addition of helper without extra check for queue setup in
    it. I.e. just code restructuring without any changes.
    Definitely harmless.
 * Additoin of queue setup check to helpers
 * Addition of queue ID checks to Rx interrupt control routines

> 
> 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>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 -> v4:
> 	 1. dropping the 'rte_' prefix from the funcitons names.
> 	 2. get "struct rte_eth_dev *dev" as parameter and drop the port_id
> 	    validation in the function named eth_dev_validate_rx_queue and
> 	    eth_dev_validate_tx_queue.
> 	 3. replace "setupped" with "setup" in the commit log and titile.
> v2 -> v3:
> 	 don't break lines in format strings.
> v1 -> v2:
> 	 1. replace %"PRIu16" with %u.
> 	 2. extact two common functions which validate RXQ/TXQ ids and
> 	    whether it has been set up or not.
> ---
>  lib/librte_ethdev/rte_ethdev.c | 86 ++++++++++++++++++++++++++++++++++--------
>  lib/librte_ethdev/rte_ethdev.h |  3 +-
>  2 files changed, 72 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 892c246..04d30f9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -877,10 +877,53 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>  	return 0;
>  }
>  
> +static inline int

Pleae, remove inline. The compiler should be wise enought to do
the job. Also it is control path helpers, so there is no point
to blow binaries size.

> +eth_dev_validate_rx_queue(struct rte_eth_dev *dev, uint16_t rx_queue_id)

May be add 'const' to dev to highlight that it changes nothing.

> +{
> +	uint16_t port_id;
> +
> +	if (rx_queue_id >= dev->data->nb_rx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);

RX -> Rx
Please, log port ID as well.
I see that the log is simply copies from previous code,
but let's improve it on the way.

> +		return -EINVAL;
> +	}
> +
> +	if (dev->data->rx_queues[rx_queue_id] == NULL) {
> +		port_id = dev->data->port_id;
> +		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;
> +}
> +
> +static inline int

same as above

> +eth_dev_validate_tx_queue(struct rte_eth_dev *dev, uint16_t tx_queue_id)

same as above

> +{
> +	uint16_t port_id;
> +
> +	if (tx_queue_id >= dev->data->nb_tx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);

TX -> Tx
plus port_id logging

> +		return -EINVAL;
> +	}
> +
> +	if (dev->data->tx_queues[tx_queue_id] == NULL) {
> +		port_id = dev->data->port_id;
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Queue %u of device with port_id=%u has not been setup\n",
> +			       tx_queue_id, port_id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  
> @@ -892,10 +935,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
>  		return -EINVAL;
>  	}
>  
> -	if (rx_queue_id >= dev->data->nb_rx_queues) {
> -		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
> -		return -EINVAL;
> -	}
> +	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
> +	if (ret)

DPDK coding style requires explicit comparion to 0.
Please, add != 0

> +		return ret;
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
>  
> @@ -922,14 +964,15 @@ int
>  rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	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;
> -	}
> +
> +	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
> +	if (ret)

same as above, if (ret != 0)

> +		return ret;
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
>  
> @@ -955,6 +998,7 @@ int
>  rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  
> @@ -966,10 +1010,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
>  		return -EINVAL;
>  	}
>  
> -	if (tx_queue_id >= dev->data->nb_tx_queues) {
> -		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
> -		return -EINVAL;
> -	}
> +	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
> +	if (ret)

same as above, if (ret != 0)

> +		return ret;
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
>  
> @@ -994,14 +1037,15 @@ int
>  rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>  
>  	dev = &rte_eth_devices[port_id];
> -	if (tx_queue_id >= dev->data->nb_tx_queues) {
> -		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
> -		return -EINVAL;
> -	}
> +
> +	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
> +	if (ret)

same as above, if (ret != 0)

> +		return ret;
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
>  
> @@ -4458,11 +4502,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
>  			   uint16_t queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
>  	dev = &rte_eth_devices[port_id];
>  
> +	ret = eth_dev_validate_rx_queue(dev, queue_id);
> +	if (ret)

same as above, if (ret != 0)

> +		return ret;
> +
>  	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));
> @@ -4473,11 +4522,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
>  			    uint16_t queue_id)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  
>  	dev = &rte_eth_devices[port_id];
>  
> +	ret = eth_dev_validate_rx_queue(dev, queue_id);
> +	if (ret)

same as above, if (ret != 0)

> +		return ret;
> +
>  	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);
> 

[dpdk-dev] [PATCH v5 0/3] check queue id in queue-related APIs

[Wei Hu (Xavier)]

From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>

This series check queue id in queue-related API functions.

Wei Hu (Xavier) (3):
  ethdev: extract checking queue id into common functions
  ethdev: check if queue setup in queue-related APIs
  ethdev: check queue id in Rx interrupt control routine

 lib/librte_ethdev/rte_ethdev.c | 92 ++++++++++++++++++++++++++++++++++--------
 lib/librte_ethdev/rte_ethdev.h |  3 +-
 2 files changed, 78 insertions(+), 17 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v5 1/3] ethdev: extract checking queue id into common functions

[Wei Hu (Xavier)]

From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>

This patch extact checking rx_queue_id or tx_queue_id into two separate
common functions named eth_dev_validate_rx_queue and
eth_dev_validate_tx_queue.

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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
---
v4 -> v5:
	 1. remove inline.
	 2. add 'const' to dev to highlight that it changes nothing.
	 3. RX -> Rx, TX -> Tx.
	 4. log port ID in the error branch.
	 5. 'if (ret)'  -> 'if (ret != 0)'
	 6. split the original patch into three separate patches.
v3 -> v4:
	 1. dropping the 'rte_' prefix from the funcitons names.
	 2. get "struct rte_eth_dev *dev" as parameter and drop the port_id
	    validation in the function named eth_dev_validate_rx_queue and
	    eth_dev_validate_tx_queue.
	 3. replace "setupped" with "setup" in the commit log and titile.
v2 -> v3:
	 don't break lines in format strings.
v1 -> v2:
	 1. replace %"PRIu16" with %u.
	 2. extact two common functions which validate RXQ/TXQ ids and
	    whether it has been set up or not.
---
 lib/librte_ethdev/rte_ethdev.c | 66 ++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 892c246..11e54d9 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -877,10 +877,43 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 	return 0;
 }
 
+static int
+eth_dev_validate_rx_queue(const struct rte_eth_dev *dev, uint16_t rx_queue_id)
+{
+	uint16_t port_id;
+
+	if (rx_queue_id >= dev->data->nb_rx_queues) {
+		port_id = dev->data->port_id;
+		RTE_ETHDEV_LOG(ERR,
+			       "Invalid Rx queue_id=%u of device with port_id=%u\n",
+			       rx_queue_id, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+eth_dev_validate_tx_queue(const struct rte_eth_dev *dev, uint16_t tx_queue_id)
+{
+	uint16_t port_id;
+
+	if (tx_queue_id >= dev->data->nb_tx_queues) {
+		port_id = dev->data->port_id;
+		RTE_ETHDEV_LOG(ERR,
+			       "Invalid Tx queue_id=%u of device with port_id=%u\n",
+			       tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int
 rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -892,10 +925,9 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
-	if (rx_queue_id >= dev->data->nb_rx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
-		return -EINVAL;
-	}
+	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
+	if (ret != 0)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_start, -ENOTSUP);
 
@@ -922,14 +954,15 @@ int
 rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	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;
-	}
+
+	ret = eth_dev_validate_rx_queue(dev, rx_queue_id);
+	if (ret != 0)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_stop, -ENOTSUP);
 
@@ -955,6 +988,7 @@ int
 rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -966,10 +1000,9 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
+	if (ret != 0)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_start, -ENOTSUP);
 
@@ -994,14 +1027,15 @@ int
 rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
-	if (tx_queue_id >= dev->data->nb_tx_queues) {
-		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
-		return -EINVAL;
-	}
+
+	ret = eth_dev_validate_tx_queue(dev, tx_queue_id);
+	if (ret != 0)
+		return ret;
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_stop, -ENOTSUP);
 
-- 
2.9.5

[dpdk-dev] [PATCH v5 2/3] ethdev: check if queue setup in queue-related APIs

[Wei Hu (Xavier)]

From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>

This patch adds checking whether the related Tx or Rx queue has been setup
in the queue-related API functions to avoid illegal address access.

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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.c | 16 ++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h |  3 ++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 11e54d9..4acf858 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -890,6 +890,14 @@ eth_dev_validate_rx_queue(const struct rte_eth_dev *dev, uint16_t rx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->rx_queues[rx_queue_id] == NULL) {
+		port_id = dev->data->port_id;
+		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;
 }
 
@@ -906,6 +914,14 @@ eth_dev_validate_tx_queue(const struct rte_eth_dev *dev, uint16_t tx_queue_id)
 		return -EINVAL;
 	}
 
+	if (dev->data->tx_queues[tx_queue_id] == NULL) {
+		port_id = dev->data->port_id;
+		RTE_ETHDEV_LOG(ERR,
+			       "Queue %u of device with port_id=%u has not been setup\n",
+			       tx_queue_id, port_id);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
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);
-- 
2.9.5

[dpdk-dev] [PATCH v5 3/3] ethdev: check queue id in Rx interrupt control routine

[Wei Hu (Xavier)]

From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>

This patch add queue ID checks to Rx interrupt control routines.

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>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 4acf858..5b7979a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4508,11 +4508,16 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id,
 			   uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = eth_dev_validate_rx_queue(dev, queue_id);
+	if (ret != 0)
+		return ret;
+
 	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));
@@ -4523,11 +4528,16 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id,
 			    uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
+	ret = eth_dev_validate_rx_queue(dev, queue_id);
+	if (ret != 0)
+		return ret;
+
 	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));
-- 
2.9.5

Re: [dpdk-dev] [PATCH v5 1/3] ethdev: extract checking queue id into common functions

[Andrew Rybchenko]

On 10/13/20 2:50 PM, Wei Hu (Xavier) wrote:
> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
> 
> This patch extact checking rx_queue_id or tx_queue_id into two separate

"This patch extact ..." -> "Extract ..."

> common functions named eth_dev_validate_rx_queue and
> eth_dev_validate_tx_queue.
> 
> 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>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v5 0/3] check queue id in queue-related APIs

[Ferruh Yigit]

On 10/13/2020 12:50 PM, Wei Hu (Xavier) wrote:
> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
> 
> This series check queue id in queue-related API functions.
> 
> Wei Hu (Xavier) (3):
>    ethdev: extract checking queue id into common functions
>    ethdev: check if queue setup in queue-related APIs
>    ethdev: check queue id in Rx interrupt control routine
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v5 0/3] check queue id in queue-related APIs

[Ferruh Yigit]

On 10/13/2020 2:17 PM, Ferruh Yigit wrote:
> On 10/13/2020 12:50 PM, Wei Hu (Xavier) wrote:
>> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com>
>>
>> This series check queue id in queue-related API functions.
>>
>> Wei Hu (Xavier) (3):
>>    ethdev: extract checking queue id into common functions
>>    ethdev: check if queue setup in queue-related APIs
>>    ethdev: check queue id in Rx interrupt control routine
>>
> 
> For series,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/main, thanks.