netdev - Re: [PATCH v2 3/8] fwctl: FWCTL_INFO to return basic information about the device

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: <20240726161503.00001c85@Huawei.com>
Date: Fri, 26 Jul 2024 16:15:03 +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 3/8] fwctl: FWCTL_INFO to return basic information
 about the device

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

> Userspace will need to know some details about the fwctl interface being
> used to locate the correct userspace code to communicate with the
> kernel. Provide a simple device_type enum indicating what the kernel
> driver is.

As below - maybe consider a UUID?
Would let you decouple allocating those with upstreaming drivers.
We'll just get annoying races on the enum otherwise as multiple
drivers get upstreamed that use this.

> 
> Allow the device to provide a device specific info struct that contains
> any additional information that the driver may need to provide to
> userspace.
> 
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> ---
>  drivers/fwctl/main.c       | 53 ++++++++++++++++++++++++++++++++++++++
>  include/linux/fwctl.h      |  8 ++++++
>  include/uapi/fwctl/fwctl.h | 29 +++++++++++++++++++++
>  3 files changed, 90 insertions(+)
> 
> diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c
> index 6872c01d5c62e8..f1dec0b590aee4 100644
> --- a/drivers/fwctl/main.c
> +++ b/drivers/fwctl/main.c
> @@ -17,6 +17,8 @@ enum {
>  static dev_t fwctl_dev;
>  static DEFINE_IDA(fwctl_ida);
>  
> +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));

Why need for a new one?  That's the same as the one in slab.h from
6.9 onwards. Before that it was
if (_T)

I was going to suggest promoting this to slab.h and then found
the normal implementation had been improved since I last checked.


>  
>  /**
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 0bdce95b6d69d9..39db9f09f8068e 100644
> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -36,6 +36,35 @@
>   */
>  enum {
>  	FWCTL_CMD_BASE = 0,
> +	FWCTL_CMD_INFO = 0,
> +	FWCTL_CMD_RPC = 1,
>  };
>  
> +enum fwctl_device_type {
> +	FWCTL_DEVICE_TYPE_ERROR = 0,
> +};
> +
> +/**
> + * struct fwctl_info - ioctl(FWCTL_INFO)
> + * @size: sizeof(struct fwctl_info)
> + * @flags: Must be 0
> + * @out_device_type: Returns the type of the device from enum fwctl_device_type

Maybe a UUID?  Avoid need to synchronize that list for ever.

> + * @device_data_len: On input the length of the out_device_data memory. On
> + *	output the size of the kernel's device_data which may be larger or
> + *	smaller than the input. Maybe 0 on input.
> + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will
> + *	fill the entire memory, zeroing as required.

Why do we need device in names of these two?

> + *
> + * Returns basic information about this fwctl instance, particularly what driver
> + * is being used to define the device_data format.
> + */
> +struct fwctl_info {
> +	__u32 size;
> +	__u32 flags;
> +	__u32 out_device_type;
> +	__u32 device_data_len;
> +	__aligned_u64 out_device_data;
> +};
> +#define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO)
> +
>  #endif