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 543ADA0548; Tue, 27 Apr 2021 04:44:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0E36940143; Tue, 27 Apr 2021 04:44:36 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id 927914003E for ; Tue, 27 Apr 2021 04:44:34 +0200 (CEST) Received: from DGGEMS412-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FTmH83bDjzmdsx for ; Tue, 27 Apr 2021 10:41:24 +0800 (CST) Received: from [127.0.0.1] (10.69.27.114) by DGGEMS412-HUB.china.huawei.com (10.3.19.212) with Microsoft SMTP Server id 14.3.498.0; Tue, 27 Apr 2021 10:44:23 +0800 To: Ferruh Yigit , "Min Hu (Connor)" , References: <1619075569-33619-1-git-send-email-humin29@huawei.com> From: Chengchang Tang Message-ID: <5871c7f5-58d7-e101-c544-f5374936f09c@huawei.com> Date: Tue, 27 Apr 2021 10:44:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.69.27.114] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check 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" On 2021/4/26 22:54, Ferruh Yigit wrote: > On 4/22/2021 8:12 AM, Min Hu (Connor) wrote: >> From: Chengchang Tang >> >> The socket ID entered by user is cast to an unsigned integer. However, >> the value may be an illegal negative value, which may cause some >> problems. In this case, an error should be returned. >> > > +1 to fix > >> In addition, the socket ID may be an invalid positive number, which is >> also processed in this patch. >> >> Fixes: 2efb58cbab6e ("bond: new link bonding library") >> Cc: stable@dpdk.org >> >> Signed-off-by: Chengchang Tang >> Signed-off-by: Min Hu (Connor) >> --- >> drivers/net/bonding/rte_eth_bond_args.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c >> index 8c5f90d..bcc0fe3 100644 >> --- a/drivers/net/bonding/rte_eth_bond_args.c >> +++ b/drivers/net/bonding/rte_eth_bond_args.c >> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused, >> return -1; >> >> errno = 0; >> - socket_id = (uint8_t)strtol(value, &endptr, 10); >> + socket_id = strtol(value, &endptr, 10); > > 'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is > if this cause a static analysis tool warning. > What do you think to have 'socket_id' type as 'long int'? > I think it would be better to cast to the 'int' here, for reasons below. >> if (*endptr != 0 || errno != 0) >> return -1; >> >> /* validate socket id value */ >> - if (socket_id >= 0) { >> + if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {> *(uint8_t *)extra_args = (uint8_t)socket_id; > > Here there is an assumption that RTE_MAX_NUMA_NODES will be less than > 'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption. Currently, it is unlikely that RTE_MAX_NUMA_NODES will be greater than 256. Therefore, adding such check will not cause any problems. But I don't think it's necessary to put such restrictions on it (i.e. RTE_MAX_NUMA_NODES should be less than UCHAR_MAX). I checked all references to RTE_MAX_NUMA_NODES, and usually socket_id is of type 'int' or 'unsigned int' (Only the efd, node, and bonding specify 'unsigned char' for socket IDs.). And for that, I think it will be better to change the socket id type to 'int' in this patch. For the type of socket id in efd and node, I will send new patches to modify it. > >> return 0; >> } >> > > > . >