netdev - Re: [PATCH 2/8] fwctl: Basic ioctl dispatch for the character 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]

Date: Tue, 4 Jun 2024 17:50:23 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Leon Romanovsky <leon@...nel.org>
CC: Zhu Yanjun <zyjzyj2000@...il.com>, Jason Gunthorpe <jgg@...dia.com>,
	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 2/8] fwctl: Basic ioctl dispatch for the character
 device

On Tue, 4 Jun 2024 15:22:21 +0300
Leon Romanovsky <leon@...nel.org> wrote:

> On Tue, Jun 04, 2024 at 02:16:12PM +0200, Zhu Yanjun wrote:
> > On 03.06.24 17:53, Jason Gunthorpe wrote:  
> > > Each file descriptor gets a chunk of per-FD driver specific context that
> > > allows the driver to attach a device specific struct to. The core code
> > > takes care of the memory lifetime for this structure.
> > > 
> > > The ioctl dispatch and design is based on what was built for iommufd. The
> > > ioctls have a struct which has a combined in/out behavior with a typical
> > > 'zero pad' scheme for future extension and backwards compatibility.
> > > 
> > > Like iommufd some shared logic does most of the ioctl marshalling and
> > > compatibility work and tables diatches to some function pointers for
> > > each unique iotcl.
> > > 
> > > This approach has proven to work quite well in the iommufd and rdma
> > > subsystems.
> > > 
> > > Allocate an ioctl number space for the subsystem.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> > > ---
> > >   .../userspace-api/ioctl/ioctl-number.rst      |   1 +
> > >   MAINTAINERS                                   |   1 +
> > >   drivers/fwctl/main.c                          | 124 +++++++++++++++++-
> > >   include/linux/fwctl.h                         |  31 +++++
> > >   include/uapi/fwctl/fwctl.h                    |  41 ++++++
> > >   5 files changed, 196 insertions(+), 2 deletions(-)
> > >   create mode 100644 include/uapi/fwctl/fwctl.h  
> 
> <...>
> 
> > >   static int fwctl_fops_open(struct inode *inode, struct file *filp)
> > >   {
> > >   	struct fwctl_device *fwctl =
> > >   		container_of(inode->i_cdev, struct fwctl_device, cdev);
> > > +	struct fwctl_uctx *uctx __free(kfree) = NULL;
> > > +	int ret;
> > > +
> > > +	guard(rwsem_read)(&fwctl->registration_lock);
> > > +	if (!fwctl->ops)
> > > +		return -ENODEV;
> > > +
> > > +	uctx = kzalloc(fwctl->ops->uctx_size, GFP_KERNEL |  GFP_KERNEL_ACCOUNT);
> > > +	if (!uctx)
> > > +		return -ENOMEM;
> > > +
> > > +	uctx->fwctl = fwctl;
> > > +	ret = fwctl->ops->open_uctx(uctx);
> > > +	if (ret)
> > > +		return ret;  
> > 
> > When something is wrong, uctx is freed in "fwctl->ops->open_uctx(uctx);"?
> > 
> > If not, the allocated memory uctx leaks here.  
> 
> See how uctx is declared:
> struct fwctl_uctx *uctx __free(kfree) = NULL;
> 
> It will be released automatically.
> See include/linux/cleanup.h for more details.

I'm lazy so not finding the discussion now, but Linus has been pretty clear
that he doesn't like this pattern because of possibility of additional cleanup
magic getting introduced and then the cleanup happening in an order that
causes problems. 

Preferred option is drag the declaration to where is initialized so break
with our tradition of declarations all at the top

struct fwctl_uctx *uctx __free(kfree) =
	kzalloc(...);
etc


> 
> Thanks
>