On Tue, 4 Jun 2024 13:58:44 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/
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
