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: <ebf275e7-f986-436d-b665-3320a04eb83e@moroto.mountain>
Date: Wed, 10 Apr 2024 16:24:19 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: 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 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