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: <20240604093219.GN3884@unreal>
Date: Tue, 4 Jun 2024 12:32:19 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Jonathan Corbet <corbet@....net>, Itay Avraham <itayavr@...dia.com>,
	Jakub Kicinski <kuba@...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>, linux-cxl@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH 1/8] fwctl: Add basic structure for a class subsystem
 with a cdev

On Mon, Jun 03, 2024 at 12:53:17PM -0300, Jason Gunthorpe wrote:
> Create the class, character device and functions for a fwctl driver to
> un/register to the subsystem.
> 
> A typical fwctl driver has a sysfs presence like:
> 
> $ ls -l /dev/fwctl/fwctl0
> crw------- 1 root root 250, 0 Apr 25 19:16 /dev/fwctl/fwctl0
> 
> $ ls /sys/class/fwctl/fwctl0
> dev  device  power  subsystem  uevent
> 
> $ ls /sys/class/fwctl/fwctl0/device/infiniband/
> ibp0s10f0
> 
> $ ls /sys/class/infiniband/ibp0s10f0/device/fwctl/
> fwctl0/
> 
> $ ls /sys/devices/pci0000:00/0000:00:0a.0/fwctl/fwctl0
> dev  device  power  subsystem  uevent
> 
> Which allows userspace to link all the multi-subsystem driver components
> together and learn the subsystem specific names for the device's
> components.
> 
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> ---
>  MAINTAINERS            |   8 ++
>  drivers/Kconfig        |   2 +
>  drivers/Makefile       |   1 +
>  drivers/fwctl/Kconfig  |   9 +++
>  drivers/fwctl/Makefile |   4 +
>  drivers/fwctl/main.c   | 174 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/fwctl.h  |  68 ++++++++++++++++
>  7 files changed, 266 insertions(+)
>  create mode 100644 drivers/fwctl/Kconfig
>  create mode 100644 drivers/fwctl/Makefile
>  create mode 100644 drivers/fwctl/main.c
>  create mode 100644 include/linux/fwctl.h

<...>

> +static struct fwctl_device *
> +_alloc_device(struct device *parent, const struct fwctl_ops *ops, size_t size)
> +{
> +	struct fwctl_device *fwctl __free(kfree) = kzalloc(size, GFP_KERNEL);
> +
> +	if (!fwctl)
> +		return NULL;

<...>

> +/* Drivers use the fwctl_alloc_device() wrapper */
> +struct fwctl_device *_fwctl_alloc_device(struct device *parent,
> +					 const struct fwctl_ops *ops,
> +					 size_t size)
> +{
> +	struct fwctl_device *fwctl __free(fwctl) =
> +		_alloc_device(parent, ops, size);

I'm not a big fan of cleanup.h pattern as it hides important to me
information about memory object lifetime and by "solving" one class of
problems it creates another one.

You didn't check if fwctl is NULL before using it.

> +	int devnum;
> +
> +	devnum = ida_alloc_max(&fwctl_ida, FWCTL_MAX_DEVICES - 1, GFP_KERNEL);
> +	if (devnum < 0)
> +		return NULL;
> +	fwctl->dev.devt = fwctl_dev + devnum;
> +
> +	cdev_init(&fwctl->cdev, &fwctl_fops);
> +	fwctl->cdev.owner = THIS_MODULE;
> +
> +	if (dev_set_name(&fwctl->dev, "fwctl%d", fwctl->dev.devt - fwctl_dev))

Did you miss ida_free() here?

> +		return NULL;
> +
> +	fwctl->ops = ops;
> +	return_ptr(fwctl);
> +}
> +EXPORT_SYMBOL_NS_GPL(_fwctl_alloc_device, FWCTL);

Thanks

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ