netdev - Re: [PATCH net V2 12/12] net/mlx5: SD, Handle possible devcom ERR

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: <43f4a10f-b5d1-4a1e-a765-c2324d03c6a7@gmail.com>
Date: Wed, 10 Apr 2024 21:16:37 +0300
From: Tariq Toukan <ttoukan.linux@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, Tariq Toukan <tariqt@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
 Saeed Mahameed <saeedm@...dia.com>, Gal Pressman <gal@...dia.com>,
 Leon Romanovsky <leonro@...dia.com>
Subject: Re: [PATCH net V2 12/12] net/mlx5: SD, Handle possible devcom ERR_PTR

On 10/04/2024 16:24, Dan Carpenter wrote:
> On Tue, Apr 09, 2024 at 10:08:20PM +0300, Tariq Toukan wrote:
>> Check if devcom holds an error pointer and return immediately.
>>
>> This fixes Smatch static checker warning:
>> drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:221 sd_register()
>> error: 'devcom' dereferencing possible ERR_PTR()
>>
>> Fixes: d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary election")
>> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
>> Link: https://lore.kernel.org/all/f09666c8-e604-41f6-958b-4cc55c73faf9@gmail.com/T/
>> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
>> Reviewed-by: Gal Pressman <gal@...dia.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> index 5b28084e8a03..adbafed44ce7 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
>> @@ -213,8 +213,8 @@ static int sd_register(struct mlx5_core_dev *dev)
>>   	sd = mlx5_get_sd(dev);
>>   	devcom = mlx5_devcom_register_component(dev->priv.devc, MLX5_DEVCOM_SD_GROUP,
>>   						sd->group_id, NULL, dev);
>> -	if (!devcom)
>> -		return -ENOMEM;
>> +	if (IS_ERR_OR_NULL(devcom))
>> +		return devcom ? PTR_ERR(devcom) : -ENOMEM;
> 
> Why not just change mlx5_devcom_register_component() to return
> ERR_PTR(-EINVAL); instead of NULL?  Then the callers could just do:
> 
> 	if (IS_ERR(devcom))
> 		return PTR_ERR(devcom);
> 
> We only have a sample size of 4 callers but doing it in this
> non-standard way seems to introduce bugs in 25% of the callers.
> 
> regards,
> dan carpenter
> 
> 

Hi Dan,

Touching mlx5_devcom_register_component() as a fix for commit 
d3d057666090 ("net/mlx5: SD, Implement devcom communication and primary 
election") doesn't look right to me.
In addition, I prefer minimal fixes to net.

After this one is accepted to net, we can enhance the code in a followup 
patch to net-next.

Regards,
Tariq