netdev - Re: [PATCH net 1/6] net: hns3: fix side effects passed to min

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ee5706c6-c841-24c0-8f65-60dbbc3cbdf8@huawei.com>
Date: Sat, 29 Jul 2023 10:57:10 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: David Laight <David.Laight@...LAB.COM>, "yisen.zhuang@...wei.com"
	<yisen.zhuang@...wei.com>, "salil.mehta@...wei.com" <salil.mehta@...wei.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>
CC: "shenjian15@...wei.com" <shenjian15@...wei.com>, "wangjie125@...wei.com"
	<wangjie125@...wei.com>, "liuyonglong@...wei.com" <liuyonglong@...wei.com>,
	"wangpeiyang1@...wei.com" <wangpeiyang1@...wei.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "stable@...r.kernel.org" <stable@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 1/6] net: hns3: fix side effects passed to min_t()

Hi David：

Yes, you're right, min_t() evaluates the arguments only once.

In the actual scenario, the number of cpu is far less than 65535. 
Therefore, the minimum value will not convert to zero.

Thanks for your advice, this patch will be withdrawn.

   Jijie Shao

on 2023/7/28 16:29, David Laight wrote:
> From: Jijie Shao
>> Sent: 28 July 2023 08:59
>>
>> num_online_cpus() may call more than once when passing to min_t(),
>> between calls, it may return different values, so move num_online_cpus()
>> out of min_t().
> Nope, wrong bug:
> min() (and friends) are careful to only evaluate their arguments once.
> The bug is using min_t() - especially with a small type.
>
> If/when the number of cpu hits 65536 the (u16) cast will convert
> it to zero.
>
> Looking at the code a lot of the local variables should be
> 'unsigned int' not 'u16.
> Just because the domain of a value is small doesn't mean
> you should use a small type (unless you are saving space in
> a structure).
>
> 	David
>
>> Signed-off-by: Yonglong Liu <liuyonglong@...wei.com>
>> Signed-off-by: Jijie Shao <shaojijie@...wei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 9f6890059666..823e6d2e85f5 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>>   {
>>   	struct hnae3_handle *h = priv->ae_handle;
>>   	struct hns3_enet_tqp_vector *tqp_vector;
>> +	u32 online_cpus = num_online_cpus();
>>   	struct hnae3_vector_info *vector;
>>   	struct pci_dev *pdev = h->pdev;
>>   	u16 tqp_num = h->kinfo.num_tqps;
>> @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>>
>>   	/* RSS size, cpu online and vector_num should be the same */
>>   	/* Should consider 2p/4p later */
>> -	vector_num = min_t(u16, num_online_cpus(), tqp_num);
>> +	vector_num = min_t(u16, online_cpus, tqp_num);
>>
>>   	vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector),
>>   			      GFP_KERNEL);
>> --
>> 2.30.0
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>