Hi Xavier, Thanks for taking care of the comments. LGTM.
Reviewed-by: Kalesh AP <kalesh-anakkur.pura...@broadcom.com> Regards, Kalesh On Tue, Oct 13, 2020 at 8:11 AM Wei Hu (Xavier) <huwei...@chinasoftinc.com> wrote: > From: Chengchang Tang <tangchengch...@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 <tangchengch...@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.hu...@huawei.com> > Signed-off-by: Chengwen Feng <fengcheng...@huawei.com> > Acked-by: Stephen Hemminger <step...@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