netdev - Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5 fw

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: <20240726171013.00006e67@Huawei.com>
Date: Fri, 26 Jul 2024 17:10:13 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: Jonathan Corbet <corbet@....net>, Itay Avraham <itayavr@...dia.com>, Jakub
 Kicinski <kuba@...nel.org>, Leon Romanovsky <leon@...nel.org>,
	<linux-doc@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
	<netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>, Saeed Mahameed
	<saeedm@...dia.com>, Tariq Toukan <tariqt@...dia.com>, Andy Gospodarek
	<andrew.gospodarek@...adcom.com>, Aron Silverton <aron.silverton@...cle.com>,
	Dan Williams <dan.j.williams@...el.com>, David Ahern <dsahern@...nel.org>,
	Christoph Hellwig <hch@...radead.org>, Jiri Pirko <jiri@...dia.com>, Leonid
 Bloch <lbloch@...dia.com>, "Leon Romanovsky" <leonro@...dia.com>,
	<linux-cxl@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v2 7/8] fwctl/mlx5: Support for communicating with mlx5
 fw

On Mon, 24 Jun 2024 19:47:31 -0300
Jason Gunthorpe <jgg@...dia.com> wrote:

> From: Saeed Mahameed <saeedm@...dia.com>
> 
> mlx5's fw has long provided a User Context concept. This has a long
> history in RDMA as part of the devx extended verbs programming
> interface. A User Context is a security envelope that contains objects and
> controls access. It contains the Protection Domain object from the
> InfiniBand Architecture and both togther provide the OS with the necessary
> tools to bind a security context like a process to the device.
> 
> The security context is restricted to not be able to touch the kernel or
> other processes. In the RDMA verbs case it is also restricted to not touch
> global device resources.
> 
> The fwctl_mlx5 takes this approach and builds a User Context per fwctl
> file descriptor and uses a FW security capability on the User Context to
> enable access to global device resources. This makes the context useful
> for provisioning and debugging the global device state.
> 
> mlx5 already has a robust infrastructure for delivering RPC messages to
> fw. Trivially connect fwctl's RPC mechanism to mlx5_cmd_do(). Enforce the
> User Context ID in every RPC header so the FW knows the security context
> of the issuing ID.
> 
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>

A few minor comments + a reference counting question.

> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index 37147a695add9a..e5ee2d46d43126 100644
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -7,3 +7,17 @@ menuconfig FWCTL
>  	  support a wide range of lockdown compatible device behaviors including
>  	  manipulating device FLASH, debugging, and other activities that don't
>  	  fit neatly into an existing subsystem.
> +
> +if FWCTL

Why not use depends on FWCTL?

> +config FWCTL_MLX5
> +	tristate "mlx5 ConnectX control fwctl driver"
> +	depends on MLX5_CORE
> +	help
> +	  MLX5CTL provides interface for the user process to access the debug and
> +	  configuration registers of the ConnectX hardware family
> +	  (NICs, PCI switches and SmartNIC SoCs).
> +	  This will allow configuration and debug tools to work out of the box on
> +	  mainstream kernel.
> +
> +	  If you don't know what to do here, say N.
> +endif

> diff --git a/drivers/fwctl/mlx5/main.c b/drivers/fwctl/mlx5/main.c
> new file mode 100644
> index 00000000000000..5e64371d7e5508
> --- /dev/null
> +++ b/drivers/fwctl/mlx5/main.c



> +static void mlx5ctl_remove(struct auxiliary_device *adev)
> +{
> +	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);

So this is calling fwctl_put(&mcdev->fwctl) on scope exit.

Why do you need to drop a reference beyond the one fwctl_unregister() is dropping
in cdev_device_del()?  Where am I missing a reference get?

> +
> +	fwctl_unregister(&mcdev->fwctl);
> +}
> +
> +static const struct auxiliary_device_id mlx5ctl_id_table[] = {
> +	{.name = MLX5_ADEV_NAME ".fwctl",},
> +	{},

No point in comma after terminating entries

> +};
> +MODULE_DEVICE_TABLE(auxiliary, mlx5ctl_id_table);
> +
> +static struct auxiliary_driver mlx5ctl_driver = {
> +	.name = "mlx5_fwctl",
> +	.probe = mlx5ctl_probe,
> +	.remove = mlx5ctl_remove,
> +	.id_table = mlx5ctl_id_table,
> +};
> +
> +module_auxiliary_driver(mlx5ctl_driver);
> +
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("mlx5 ConnectX fwctl driver");
> +MODULE_AUTHOR("Saeed Mahameed <saeedm@...dia.com>");
> +MODULE_LICENSE("Dual BSD/GPL");

> +#endif