Re: [PATCH 2/8] fwctl: Basic ioctl dispatch for the character device

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 4 Jun 2024 15:22:21 +0300
Leon Romanovsky <leon@kernel.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@nvidia.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
>