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

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

On Tue, 4 Jun 2024 13:58:44 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, Jun 04, 2024 at 05:50:23PM +0100, Jonathan Cameron wrote:
> 
> > > > >   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.   
> 
> I saw that discussion, but I thought it was talking about the macro
> behavior - ie guard() creates a variable hidden in the macro.
> 
> The point about order is interesting though - notice the above will
> free the uctx after unlocking (which is the slightly more preferred
> order here), but it is easy to imagine cases where that order would be
> wrong.
> 
> > 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(...);  
> 
> I don't recall that dramatic conclusion in the discussion, but it does
> make alot of sense to me.

I'll be less lazy (and today found the search foo to track it down).

https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@mail.gmail.com/
Linus:
> IOW, my current thinking is "let's always have the constructor and
> destructor together", and see how it ends up going.

Not set in stone but I've not yet seen a suggestion of the opposite.

The example from Bartosz that got that response was
Bartosz:
> void foo(void)
> {
>     char *s __free(kfree) = NULL;
> 
>     do_stuff();
>     s = kmalloc(42, GFP_KERNEL);
> }
> 
> Or does it always have to be:
> 
> void foo(void)
> {
>     do_stuff();
>     char *s __free(kfree) = kmalloc(42, GFP_KERNEL);
> }
So option 2.

Jonathan

> 
> Thanks,
> Jason