From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id F2541A04B6; Tue, 13 Oct 2020 06:29:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 259B41D9F8; Tue, 13 Oct 2020 06:29:09 +0200 (CEST) Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) by dpdk.org (Postfix) with ESMTP id 50B881D9DE for ; Tue, 13 Oct 2020 06:29:07 +0200 (CEST) Received: by mail-io1-f67.google.com with SMTP id u19so20746538ion.3 for ; Mon, 12 Oct 2020 21:29:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=z6U/5UYFMhIEPY/pVhACvT10OKlFLJo1Xq0elHrOTV8=; b=Pcs1E0DeqcQGK8IasCUEwByrm9AxyXZdTplH4C48VgQUXz+g1AxIwrDtHkU/SQDwr3 5gmeN8wxrNFR46potQ+oob8jxDrlezIKWA0M9QFmNxmv9lYFJVu+JEL+SlrQwsirSxFw t0xuRMlvZgaFG9eOL5fSU1tRVN3/sKPEMhPIw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=z6U/5UYFMhIEPY/pVhACvT10OKlFLJo1Xq0elHrOTV8=; b=qnXDr5V19pvt+gzV4uKnyaoOXmOO7p8lRSJzZ/BU08CvJdU9mCPe3pDfApFcEf0hgD alUYZpS4GDOzcOjyAnTawaTmnsEvK08uMMac9IW/bYes7OpeUKHoIxy++2aP7E8/A/4R 0OIZzGMfVpiR0cF9fS8uvK1zJ0gOb+eQiHhtfjTHbmkhl1q/y41/VsHv66mYg7xKncg8 Yx8+sYV1C0nVw0g6d/bVkKkjcYwXF6Jdf7MvmVfw789k8Et/sUa3ANvymbhrFqCDCLzL 6jrC/l3109OBAvw86gj8I31U1K2ek+C7Yasner90UAuQNHZqD5lJdjgnF+3nZoaRFWPS pNpA== X-Gm-Message-State: AOAM533S0CDbPDuySEM2A60vsGNZzPThrIaPwGdLIH+Khjb52vmcpwtL sj5nwKKW1DVokG8wvAVZ4e8VtsUczhuzfTpbco8n8Q== X-Google-Smtp-Source: ABdhPJwHENJtHgqv0m7zA+wyIsVP6sZx9n6aGBS0XaE+jeT+2MPTgNSopYYjLsckU3VGm2KWzXjiapTMgbuXz7U8nTM= X-Received: by 2002:a6b:8e08:: with SMTP id q8mr18272336iod.83.1602563345378; Mon, 12 Oct 2020 21:29:05 -0700 (PDT) MIME-Version: 1.0 References: <20201010071212.24086-1-huwei013@chinasoftinc.com> <20201013024126.26492-1-huwei013@chinasoftinc.com> In-Reply-To: <20201013024126.26492-1-huwei013@chinasoftinc.com> From: Kalesh Anakkur Purayil Date: Tue, 13 Oct 2020 09:58:55 +0530 Message-ID: To: "Wei Hu (Xavier)" Cc: dev@dpdk.org, Stephen Hemminger , "Yigit, Ferruh" , Wei Hu Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v4] ethdev: check if queue setup in queue-related APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Xavier, Thanks for taking care of the comments. LGTM. Reviewed-by: Kalesh AP Regards, Kalesh On Tue, Oct 13, 2020 at 8:11 AM Wei Hu (Xavier) wrote: > From: Chengchang Tang > > 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 > Signed-off-by: Wei Hu (Xavier) > Signed-off-by: Chengwen Feng > Acked-by: Stephen Hemminger > --- > 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