From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9DA31A0562; Thu, 15 Apr 2021 02:52:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6C7C4161DC7; Thu, 15 Apr 2021 02:52:42 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id 8B094161DC3 for ; Thu, 15 Apr 2021 02:52:41 +0200 (CEST) Received: from DGGEMS406-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FLLNb1rk1z16KDr; Thu, 15 Apr 2021 08:50:23 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS406-HUB.china.huawei.com (10.3.19.206) with Microsoft SMTP Server id 14.3.498.0; Thu, 15 Apr 2021 08:52:39 +0800 To: Andrew Rybchenko , CC: , References: <1618046334-39857-1-git-send-email-humin29@huawei.com> <1618398704-15759-1-git-send-email-humin29@huawei.com> <535dc494-bf3a-f213-3d9e-e5174c1baccc@oktetlabs.ru> From: "Min Hu (Connor)" Message-ID: Date: Thu, 15 Apr 2021 08:52:39 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <535dc494-bf3a-f213-3d9e-e5174c1baccc@oktetlabs.ru> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add sanity checks in control APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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, Andrew, All has been fixed in v4, check it out, thanks. 在 2021/4/14 20:00, Andrew Rybchenko 写道: > On 4/14/21 2:11 PM, Min Hu (Connor) wrote: >> This patch adds more sanity checks in control path APIs. >> >> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input") >> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables") >> Fixes: 0366137722a0 ("ethdev: check for invalid device name") >> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model") >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership") >> Fixes: f8244c6399d9 ("ethdev: increase port id range") >> Cc: stable@dpdk.org > > Please, see below. Error logging is missing in few cases and > I'd like to understand why. > >> Signed-off-by: Min Hu (Connor) >> --- >> v3: >> * set port_id checked first. >> * add error logging. >> >> v2: >> * Removed unnecessary checks. >> * Deleted checks in internal API. >> * Added documentation in the header file. >> --- >> lib/librte_ethdev/rte_ethdev.c | 274 ++++++++++++++++++++++++++++++++++++++--- >> lib/librte_ethdev/rte_ethdev.h | 20 ++- >> 2 files changed, 271 insertions(+), 23 deletions(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index 6b5cfd6..dfebcc9 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -199,6 +199,9 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) >> char *cls_str = NULL; >> int str_size; >> >> + if (iter == NULL || devargs_str == NULL) >> + return -EINVAL; >> + > > Is error logging skipped here intentially? Why? > >> memset(iter, 0, sizeof(*iter)); >> >> /* >> @@ -293,7 +296,7 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) >> uint16_t >> rte_eth_iterator_next(struct rte_dev_iterator *iter) >> { >> - if (iter->cls == NULL) /* invalid ethdev iterator */ >> + if (iter == NULL || iter->cls == NULL) /* invalid ethdev iterator */ >> return RTE_MAX_ETHPORTS; > > Is error logging skipped here intentially? Why? > >> >> do { /* loop to try all matching rte_device */ >> @@ -322,7 +325,7 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter) >> void >> rte_eth_iterator_cleanup(struct rte_dev_iterator *iter) >> { >> - if (iter->bus_str == NULL) >> + if (iter == NULL || iter->bus_str == NULL) >> return; /* nothing to free in pure class filter */ > > Is error logging skipped here intentially? Why? > >> free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */ >> free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */ > > [snip] > >> @@ -2491,6 +2536,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt) >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP); >> >> + if (queue_id >= dev->data->nb_tx_queues) { >> + RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.", >> + dev->data->nb_tx_queues); >> + return -EINVAL; >> + } >> + > > Again, it is not always a control path. So, I'm not sure that we should > add the check there. > >> /* Call driver to free pending mbufs. */ >> ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id], >> free_cnt); > > [snip] > >> @@ -2667,6 +2732,9 @@ rte_eth_link_speed_to_str(uint32_t link_speed) >> int >> rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link) >> { >> + if (str == NULL || eth_link == NULL) >> + return -EINVAL; >> + > > Is error logging skipped here intentionally? Why? > >> if (eth_link->link_status == ETH_LINK_DOWN) >> return snprintf(str, len, "Link down"); >> else > > [snip] > >> @@ -4602,6 +4784,9 @@ rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name, >> const struct rte_memzone *mz; >> int rc = 0; >> >> + if (dev == NULL || ring_name == NULL) >> + return -EINVAL; >> + > > Same question about logging here. > >> rc = eth_dev_dma_mzone_name(z_name, sizeof(z_name), dev->data->port_id, >> queue_id, ring_name); >> if (rc >= RTE_MEMZONE_NAMESIZE) { > > [snip] > >> @@ -5629,6 +5861,8 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev, >> struct rte_eth_representor_info *info = NULL; >> size_t size; >> >> + if (ethdev == NULL) >> + return -EINVAL; > > Question about logging here as well. > >> if (type == RTE_ETH_REPRESENTOR_NONE) >> return 0; >> if (repr_id == NULL) > > [snip] > . >