netdev - Re: [PATCH 1/8] fwctl: Add basic structure for a class subsystem with a cdev

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]

Date: Tue, 4 Jun 2024 12:50:09 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Leon Romanovsky <leon@...nel.org>
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 Tue, Jun 04, 2024 at 12:32:19PM +0300, Leon Romanovsky wrote:
> > +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.

I'm trying it here. One of the most common bugs I end up fixing is
error unwind and cleanup.h has successfully removed all of it. Let's
find out, others thought it was a good idea to add the infrastructure.

One thing that seems clear in my work here is that you should not use
cleanup.h if you don't have simple memory lifetime, like the above
case where the memory is freed if the function fails.

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

Oops, yes

> > +	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?

No, the put_device() does it in the release function. The __free
always calls fwctl_put()/put_device() on failure, and within all
functions except _alloc_device() the put_device() is the correct way
to free this memory.

Thanks,
Jason