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: Wed, 5 Jun 2024 12:07:37 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: Leon Romanovsky <leon@...nel.org>, Zhu Yanjun <zyjzyj2000@...il.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 13:58:44 -0300
Jason Gunthorpe <jgg@...dia.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